Re: [PATCH] NFS: filelayout should use nfs_generic_pg_test

[Benny Halevy <bhalevy@panasas.com>]

On 2011-06-06 12:47, William A. (Andy) Adamson wrote:
> On Wed, Jun 1, 2011 at 4:09 PM, Benny Halevy <bhalevy@panasas.com> wrote:
>> 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!
> 
> Is anyone coding this fix?
> 

I started working on this but switched to porting forward spnfs and
spnfs-block (which I've just pushed out).

Benny

> -->Andy
> 
>>
>>>> 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.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html