Re: [PATCH v2 7/9] xfsrestore: make node lookup more efficient

[Bill Kendall <wkendall@sgi.com>]

On 11/15/2010 02:38 PM, Alex Elder wrote:
> On Fri, 2010-11-05 at 11:35 -0500, wkendall@sgi.com wrote:
>> plain text document attachment (window_lookup_performance)
>> When converting an nh_t to a segment index and relative node index,
>> use shift and bitwise-and instead of division and modulo. Results show
>> this is about 50% faster than the current method.
>>
>>
>> Signed-off-by: Bill Kendall<wkendall@sgi.com>
>
> I have a few comments below.  They are suggestions and
> don't indicate that I've found any real problems in your
> code.  If you think that updating things in response to
> my suggestions is warranted, but want to do it later (as
> a separate change) that's OK with me.
>
> Reviewed-by: Alex Elder<aelder@sgi.com>
>
>> ---
>>   restore/node.c |  150 ++++++++++++++++++++++++++++-----------------------------
>>   restore/win.c  |   36 +++----------
>>   restore/win.h  |    6 +-
>>   3 files changed, 87 insertions(+), 105 deletions(-)
>>
>> Index: xfsdump-kernel.org/restore/node.c
>> ===================================================================
>> --- xfsdump-kernel.org.orig/restore/node.c
>> +++ xfsdump-kernel.org/restore/node.c
>
> . . .
>
>> @@ -173,6 +166,12 @@ typedef struct node_hdr node_hdr_t;
>>   static node_hdr_t *node_hdrp;
>>   static intgen_t node_fd;
>
> The following forward declarations could be eliminated
> if you just move their definitions up in the file.
>
>> +/* forward declarations of locally defined static functions ******************/
>> +static inline size_t nh2segix( nh_t nh );
>> +static inline nh_t nh2relnix( nh_t nh );
>> +static inline void node_map_internal( nh_t nh, void **pp );
>> +static inline void node_unmap_internal( nh_t nh, void **pp, bool_t freepr );
>> +
>>   /* ARGSUSED */
>>   bool_t
>>   node_init( intgen_t fd,
>
> . . .
>
>> @@ -281,6 +280,8 @@ node_init( intgen_t fd,
>>   		winmapmax = min( vmsz / segsz, max_segments );
>>   	}
>>
>> +	relnixmask = nodesperseg - 1;
>> +
>
> Is nodesperseg guaranteed to be a power of 2?  It may
> be, but it's not obvious to me from glancing at this
> block of code.

Yes, just above this hunk nodesperseg is set using a bitshift.

>
>>   	/* map the abstraction header
>>   	 */
>>   	ASSERT( ( NODE_HDRSZ&  pgmask ) == 0 );
>
> . . .
>
>>   void
>> Index: xfsdump-kernel.org/restore/win.c
>> ===================================================================
>> --- xfsdump-kernel.org.orig/restore/win.c
>> +++ xfsdump-kernel.org/restore/win.c
>> @@ -177,31 +177,16 @@ win_init( intgen_t fd,
>>   }
>>
>>   void
>> -win_map( off64_t off, void **pp )
>> +win_map( size_t segix, void **pp )
>
> It might be nice to have a segix_t or something.
> The point being that what you are passing in here
> is an index value (not a size), which is computed
> from a node handle by figuring out which segment
> that node handle resides in.
>
> I was a little puzzled that nh2segix() took a
> node handle and returned a size_t.
>
> Similarly, a relnix appears to be an index
> value within a segment; an integral typedef
> could be used to clear that up. (and make
> it clear it's different from just a nh_t).
> Again, this suggestion comes out of my
> not understanding the return type of
> nh2relnix().

I agree, these were bad choices. I based them off
the fact that the old code unnecessarily passed
a size64_t to the winmap interface for one of
the params, then just changed it to size_t.

I'll rework this one and resubmit the series.

Bill

>
>>   {
>> -	size_t offwithinseg;
>> -	size_t segix;
>>   	off64_t segoff;
>>   	win_t *winp;
>>
>>   	CRITICAL_BEGIN();
>>
>> -	/* calculate offset within segment
>> -	 */
>> -	offwithinseg = ( size_t )( off % ( off64_t )tranp->t_segsz );
>> -
>> -	/* calculate segment index
>> -	 */
>> -	segix = (size_t)( off / ( off64_t )tranp->t_segsz );
>> -
>> -	/* calculate offset of segment
>> -	 */
>> -	segoff = off - ( off64_t )offwithinseg;
>> -
>>   #ifdef TREE_DEBUG
>>   	mlog(MLOG_DEBUG | MLOG_TREE | MLOG_NOLOCK,
>> -	     "win_map(off=%lld,addr=%x): off within = %llu, segoff = %lld\n",
>> -	      off, pp, offwithinseg, segoff);
>> +	     "win_map(segix=%lu,addr=%p)\n", segix, pp);
>>   #endif
>>   	/* resize the array if necessary */
>>   	if ( segix>= tranp->t_segmaplen )
>
>
> . . .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs