xfs
[Top] [All Lists]

Re: [PATCH v2 3/9] xfsrestore: cache path lookups

To: wkendall@xxxxxxx
Subject: Re: [PATCH v2 3/9] xfsrestore: cache path lookups
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 12 Nov 2010 17:25:11 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101105163643.571496225@xxxxxxx>
References: <20101105163500.747192954@xxxxxxx> <20101105163643.571496225@xxxxxxx>
Reply-to: aelder@xxxxxxx
On Fri, 2010-11-05 at 11:35 -0500, wkendall@xxxxxxx wrote:
> 
> In order to resolve a pathname, xfsrestore must work from an inode
> number (from the dump) and recurse up the directory entry tree that it
> has constructed. Each level of recursion requires a seek and read to
> get the name of the dirent, and possibly a mmap of a section of the
> directory entry tree if it is not already mapped (and in that case,
> possibly a munmap of another section). It's quite common to resolve
> pathnames in the same directory consecutively, so simply caching the
> parent directory pathname from the previous lookup saves quite a bit
> of overhead.
> 
> Signed-off-by: Bill Kendall <wkendall@xxxxxxx>

I think an assertion would do in a place I show below,
but it's nothing to worry about.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> ---
>  restore/tree.c |   41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
> 
> Index: xfsdump-kernel.org/restore/tree.c
> ===================================================================
> --- xfsdump-kernel.org.orig/restore/tree.c
> +++ xfsdump-kernel.org/restore/tree.c

. . .

> @@ -3475,6 +3486,13 @@ Node2path_recurse( nh_t nh, char *buf, i
>               return bufsz;
>       }
>  
> +     /* if we have a cache hit, no need to recurse any further
> +      */
> +     if ( nh == cache.nh && bufsz > cache.len ) {

Since a nh basically encodes the entire path,
the check that the buffer is big enough should
not be necessary, and could probably be inserted
instead:
                ASSERT(bufsz > cache.len);

> +             strcpy( buf, cache.buf );
> +             return bufsz - cache.len;
> +     }
> +
>       /* extract useful node members
>        */
>       np = Node_map( nh );

 . . .

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