[Top] [All Lists]

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

To: aelder@xxxxxxx
Subject: Re: [PATCH v2 7/9] xfsrestore: make node lookup more efficient
From: Bill Kendall <wkendall@xxxxxxx>
Date: Mon, 15 Nov 2010 16:06:34 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1289853517.2199.226.camel@doink>
References: <20101105163500.747192954@xxxxxxx> <20101105163644.371326494@xxxxxxx> <1289853517.2199.226.camel@doink>
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20100915 Thunderbird/3.0.8
On 11/15/2010 02:38 PM, Alex Elder wrote:
On Fri, 2010-11-05 at 11:35 -0500, wkendall@xxxxxxx 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@xxxxxxx>

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@xxxxxxx>

  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 */
  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 );

. . .

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,

-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

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.


-       size_t offwithinseg;
-       size_t segix;
        off64_t segoff;
        win_t *winp;


-       /* 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
-            "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);
        /* resize the array if necessary */
        if ( segix>= tranp->t_segmaplen )

. . .

<Prev in Thread] Current Thread [Next in Thread>