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

[Alex Elder <aelder@sgi.com>]

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.

>  	/* 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().

>  {
> -	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