Re: [PATCH 21/41] CacheFiles: Permit the page lock state to be monitored [ver #48]

[Nick Piggin <nickpiggin@yahoo.com.au>]

On Saturday 04 April 2009 22:31:40 David Howells wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> > I would really like to know exactly why it *has* to be asynchronous,
> > and see what the diff looks like between a simple synchronous design
> > and what you have here.
> 
> Okay.  See attached patch.
> 
> Three benchmarks, all with data preloaded into the cache, and all with all the
> data in memory on the server:
> 
>  (1) dd of a 100MB file.
> 
>  (2) dd of a 200MB file.
> 
>  (3) Eight parallel tars of 340MB kernel trees.
> 
> 	Benchmark	Without Patch	With Patch
> 	1		1.971s		3.371s
> 	2		3.750s		7.093s
> 	3		18 mins		22 mins
> 
> With this patch, we wait for the pages to be read from the backing fs and copy
> them in fscache_read_or_alloc_pages().

Thanks. Pretty good numbers, really, for a first cut.


> I'm sure there are ways to optimise things, but whilst doing a synchronous
> read at this in fscache_read_or_alloc_pages() might help an start-to-end read
> as done by most programs, it's less likely to help random-access reads, such
> as page-in for mmapped sections.

Why is this the case? ->readpage is always a synchronous call, whether it
is via read(2) or page fault.

->readpages is asynchonous, for readahead, but in that case the caller
isn't locking the page and waiting on them anyway, so do you really need
an asynchronous callback? You could still kick off asynchronous reads to
the backing file.


> Note also: (1) the caller of the netfs's readpages() has already done
> readahead speculation, and we should probably make best use of that, rather
> than doing it again; and

The caller *is* the readahead speculation. But if you're hooking into
the backing filesystem at the ->readpage level, then you don't get
extra readahead there anyway.


> (2) all the metadata for where the data blocks are on
> the backing fs has already been read by dint of calling bmap() to determine
> whether the data blocks exist or not.
> 
> What might be cute is to have cachefiles_read_or_alloc_pages() dentry_open()
> the backing file with O_DIRECT|O_NOHOLE, use ->read() to load the data into
> the netfs pages directly, and then fput() the file once it has finished.

It would definitely be nicer to move this thing higher up to look as
much like userspace syscalls as possible. You could probably use AIO
with your DIO too.