Re: [PATCH] NFS: Revert pnfs ugliness from the generic NFS read code path

Benny Halevy
2011-11-14T16:01:26Z

On 2011-11-14 17:49, Trond Myklebust wrote:
> On Mon, 2011-11-14 at 13:12 +0200, Benny Halevy wrote:
>> On 2011-11-10 21:38, Trond Myklebust wrote:
>>> pNFS-specific code belongs in the pnfs layer. It should not be
>>> hijacking generic NFS read or write code paths.
>>>
>>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>>> ---
>>> fs/nfs/internal.h | 2 ++
>>> fs/nfs/pnfs.c | 26 +++++++++++++++++++++-----
>>> fs/nfs/read.c | 14 ++------------
>>> 3 files changed, 25 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
>>> index c1a1bd8..3f4d957 100644
>>> --- a/fs/nfs/internal.h
>>> +++ b/fs/nfs/internal.h
>>> @@ -299,6 +299,8 @@ extern void nfs_read_prepare(struct rpc_task *task, void *calldata);
>>> extern int nfs_generic_pagein(struct nfs_pageio_descriptor *desc,
>>> struct list_head *head);
>>>
>>> +extern void nfs_pageio_init_read_mds(struct nfs_pageio_descriptor *pgio,
>>> + struct inode *inode);
>>> extern void nfs_pageio_reset_read_mds(struct nfs_pageio_descriptor *pgio);
>>> extern void nfs_readdata_release(struct nfs_read_data *rdata);
>>>
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index baf7353..8e672a2 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -1260,6 +1260,25 @@ pnfs_generic_pg_writepages(struct nfs_pageio_descriptor *desc)
>>> }
>>> EXPORT_SYMBOL_GPL(pnfs_generic_pg_writepages);
>>>
>>> +static void pnfs_ld_handle_read_error(struct nfs_read_data *data)
>>> +{
>>> + struct nfs_pageio_descriptor pgio;
>>> +
>>> + put_lseg(data->lseg);
>>> + data->lseg = NULL;
>>> + dprintk("pnfs write error = %d\n", data->pnfs_error);
>>
>> should be "read error", might as well fix this while you're at it :)
>
> Sure...
>
>>> +
>>> + nfs_pageio_init_read_mds(&pgio, data->inode);
>>> +
>>> + while (!list_empty(&data->pages)) {
>>> + struct nfs_page *req = nfs_list_entry(data->pages.next);
>>> +
>>> + nfs_list_remove_request(req);
>>> + nfs_pageio_add_request(&pgio, req);
>>> + }
>>
>> What about pgio.pg_recoalesce?
>
> It was incorrect to set it in the first place. This is an ordinary
> coalesce...
>

I see. Please just document that in the commit message...

Thanks,

Benny