Re: [PATCH] NFS: filelayout should use nfs_generic_pg_test

[Benny Halevy <bhalevy@panasas.com>]

On 2011-06-01 22:29, Trond Myklebust wrote:
> On Wed, 2011-06-01 at 22:13 +0300, Benny Halevy wrote: 
>> On 2011-06-01 21:07, Trond Myklebust wrote:
>>> On Wed, 2011-06-01 at 17:51 +0300, Benny Halevy wrote: 
>>>> I think the following should work:
>>>>
>>>> Benny
>>>>
>>>> git diff --stat -p -M
>>>>  fs/nfs/nfs4filelayout.c |   10 ++++++++++
>>>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>>>> index 4269088..9f1d445 100644
>>>> --- a/fs/nfs/nfs4filelayout.c
>>>> +++ b/fs/nfs/nfs4filelayout.c
>>>> @@ -661,6 +661,16 @@ filelayout_pg_test(struct nfs_pageio_descriptor
>>>> *pgio, struct nfs_page *prev,
>>>>  	u64 p_stripe, r_stripe;
>>>>  	u32 stripe_unit;
>>>>
>>>> +	/*
>>>> +	 * FIXME: ideally we should be able to coalesce all requests
>>>> +	 * that are not block boundary aligned, but currently this
>>>> +	 * is problematic for the case of bsize < PAGE_CACHE_SIZE,
>>>> +	 * since nfs_flush_multi and nfs_pagein_multi assume you
>>>> +	 * can have only one struct nfs_page.
>>>> +	 */
>>>> +	if (desc->pg_bsize < PAGE_SIZE)
>>>> +		return 0;
>>>> +
>>>>  	if (!pnfs_generic_pg_test(pgio, prev, req))
>>>>  		return 0;
>>>
>>> So, there are several things that bother me about pnfs_generic_pg_test()
>>> too now that I'm looking more closely at it:
>>>
>>>      1. If the intention is to coalesce 'prev' and 'req', shouldn't we
>>>         be asking for a layout with req_offset(prev) instead of
>>>         req_offset(req)? 
>>>      2. If we're only requesting a layout of length pg_count, don't we
>>>         still need to test the layout length that the server actually
>>>         returned before we can allow the coalescing? 
>>>      3. if (!pgio->lseg), shouldn't we be returning an error of some
>>>         sort? Right now we're returning 'true', and allowing the
>>>         coalesce to occur. 
>>>      4. Furthermore, shouldn't that test guarding the
>>>         pnfs_update_layout() call rather be an 'if (pgio->pg_lseg ==
>>>         NULL)' instead of looking at the values of pg_count and
>>>         prev->wb_bytes?
>>>
>>
>> or rather we get the layout for the first page in
>> nfs_pageio_do_add_request when desc->pg_count == 0?
> 
> I can live with a desc->pg_init() callback or rather, converting
> pg_test() and pg_doio() into a
> 
> struct nfs_pageio_ops {
> 	int (*pg_init)(struct nfs_pageio_descriptor *desc, struct nfs_page *req);
> 	bool (*pg_test)(struct nfs_pageio_descriptor *desc, struct nfs_page *prev, struct nfs_page *req);
> 	int (*pg_doio)(struct nfs_pageio_descriptor *desc);
> };
> 
> and then replacing the two callback functions in the existing struct
> nfs_pageio_descriptor with a single pointer to a 'const struct
> nfs_pageio_ops'...
> 

looks like a good way to do this!

>> Then, this lseg would be useful for nfs_flush_multi
>> if we failed to coalesce, or we failed to get a layout
>> altogether we go the nfs path and can reset pg_test to
>> nfs_generic_pg_test.
> 
> It would presumably also get rid of all those pnfs_update_layout() calls
> in read.c and write.c.
> 

Yup.