[Top] [All Lists]

Re: [PATCH v3 4/9] xfsrestore: mmap dirent names for faster lookups

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH v3 4/9] xfsrestore: mmap dirent names for faster lookups
From: Bill Kendall <wkendall@xxxxxxx>
Date: Wed, 17 Nov 2010 11:57:09 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101117093402.GG17317@xxxxxxxxxxxxx>
References: <20101116150502.179825893@xxxxxxx> <20101116150704.454578811@xxxxxxx> <20101117093402.GG17317@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20100915 Thunderbird/3.0.8
On 11/17/2010 03:34 AM, Christoph Hellwig wrote:
On Tue, Nov 16, 2010 at 09:05:06AM -0600, wkendall@xxxxxxx wrote:
Pathname resolution in xfsrestore is about 4x faster if the file
containing dirent names ("namreg") is memory mapped.  If xfsrestore is
unable to map the file (e.g., due to virtual memory constraints)
fallback to the existing seek-and-read approach.

The file is mapped after all directory entries have been written to
the "namreg" file. If the caller tries to add additional entries after
the file has been mapped, it will be unmapped and restore will resort
back to seek-and-read lookups.

Signed-off-by: Bill Kendall<wkendall@xxxxxxx>

Reviewed-by: Alex Elder<aelder@xxxxxxx>

Generally looks good to me, but I really hate how namreg_map/unmap
are hidden under namreg_add/flush.  As a start instead of adding the
done_adding argument we can easily move the explicit map to the one
caller wanting it, similarly namreg_unmap could be moved to namreg_add,
that is manage to mapping/unmapping explicitly.

I'll rework this.

In fact I'm not sure what the point of the unmap/map cycles is.  At
least in Linux concurrent buffer writes and mmap reads are coherent.

Adding an entry extends the file beyond what is mapped. The unmap
code was merely defensive, in case namreg_add is called somewhere
after the file is mapped. I spent some time looking at the callers,
and all namreg_adds occur before the file is mapped. namreg_del is
called after the file is mapped, but as it doesn't do anything this
is not a problem. I'll remove the unmap code.


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