[pnfs] [PATCH 01/25] pnfs: fix pnfs_{read|write}pages
Fredric Isaman
iisaman at citi.umich.edu
Fri Feb 15 11:51:52 EST 2008
On Fri, 15 Feb 2008, Benny Halevy wrote:
> On Feb. 14, 2008, 20:21 +0200, Fred Isaman <iisaman at citi.umich.edu> wrote:
>> THIS WILL BREAK OTHER LAYOUT DRIVERS that rely on old behavior.
>>
>> This patch fixes two basic problems:
>> 1. It is impossible to fall back to nfs from the layout driver's
>> read_pagelist or write_pagelist routines.
>> 2. The pnfs.c code is assuming that if these routines return a non-zero
>> status, that pnfs_writeback_done (or pnfs_read_done) was not called, which
>> is not consistent with them returning count of bytes read/written.
>>
>> This patch alters the call chain, and expects
>> ld_io_ops->{read|write}_pagelist to return either a 0 or 1, consistent
>> with its parents, putting any other information that needs to be returned
>> into the data.res and data.task structures.
>>
>> Signed-off-by: Fred Isaman <iisaman at citi.umich.edu>
>> ---
>> fs/nfs/pnfs.c | 31 ++++++++++++++++++++++---------
>> include/linux/nfs4_pnfs.h | 10 ++++++++--
>> 2 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 3f34f34..5e12cd1 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -1364,6 +1364,11 @@ pnfs_writepages(struct nfs_write_data *wdata, int how)
>> if (pnfs_get_type(inode) != LAYOUT_NFSV4_FILES)
>> wdata->pnfsflags |= PNFS_NO_RPC;
>> wdata->lseg = lseg;
>> + /* FRED - this should return just 0 (to indicate done for now)
>> + * or 1 (to indicate try normal nfs). It can indicate bytes
>> + * written in wdata->res.count. It can indicate error status in
>> + * wdata->task.tk_status.
>> + */
>
> This behavior makes sense to me but the comment better be in include/linux/nfs4_pnfs.h
> where the methods are defined rather than here.
>
OK
>> status = nfss->pnfs_curr_ld->ld_io_ops->write_pagelist(
>> nfsi->current_layout,
>> args->pages,
>> @@ -1374,13 +1379,15 @@ pnfs_writepages(struct nfs_write_data *wdata, int how)
>> how,
>> wdata);
>>
>> - if (status)
>> + if (status) {
>> put_lseg(lseg);
>> - if (status > 0) {
>> - dprintk("%s: LD write_pagelist returned status %d > 0\n", __FUNCTION__, status);
>> - pnfs_update_last_write(nfsi, args->offset, status);
>> + wdata->pnfsflags &= ~PNFS_NO_RPC;
>> + }
>> + if (status == 0 && wdata->res.count > 0) {
>
> how about "} else if (wdata->res.count > 0) {" instead?
>
I'm not sure I unserstand this comment. My intention is that status==0
implies the ld_op->write_pagelist has handled the write, calling
pnfs_writeback_done, and set wdata->res fields appropriately. On the
other hand status==1 implies the driver code has not altered anything (or
at least left it in a consistent state such that nfs_initiate_write will
work), not set any wdata->res codes, not called pnfs_writeback_done,
and now expects the code to fall through to nfs_initiate_write.
>> + dprintk("%s: LD write_pagelist returned count %d > 0\n",
>> + __FUNCTION__, wdata->res.count);
>
> minor nit: let's use __func__ rather than __FUNCTION__
> See http://lkml.org/lkml/2008/1/28/169
OK. this is an issue throughout the entire patchset that I'll fix.
>
>> + pnfs_update_last_write(nfsi, args->offset, wdata->res.count);
>> pnfs_need_layoutcommit(nfsi, wdata->args.context);
>
> Currently, in the sync completion case, where status > 0, we call put_lseg().
> You expect the layout driver to always call back pnfs_writeback_done
> (pnfs_client_ops->nfs_writelist_complete), right?
>
Only if is returns status==0
>> - status = 0;
>> }
>>
>> out:
>> @@ -1447,6 +1454,11 @@ pnfs_readpages(struct nfs_read_data *rdata)
>> if (pnfs_get_type(inode) != LAYOUT_NFSV4_FILES)
>> rdata->pnfsflags |= PNFS_NO_RPC;
>> rdata->lseg = lseg;
>> + /* FRED - this should return just 0 (to indicate done for now)
>> + * or 1 (to indicate try normal nfs). It can indicate bytes
>> + * read in rdata->res.count. It can indicate error status in
>> + * rdata->task.tk_status.
>> + */
>
> ditto
>
>> status = nfss->pnfs_curr_ld->ld_io_ops->read_pagelist(
>> nfsi->current_layout,
>> args->pages,
>> @@ -1455,12 +1467,13 @@ pnfs_readpages(struct nfs_read_data *rdata)
>> (loff_t)args->offset,
>> args->count,
>> rdata);
>> - if (status)
>> + if (status) {
>> put_lseg(lseg);
>> - if (status > 0) {
>> - dprintk("%s: LD read_pagelist returned status %d > 0\n", __FUNCTION__, status);
>> - status = 0;
>> + rdata->pnfsflags &= ~PNFS_NO_RPC;
>> }
>> + if (status == 0 && rdata->res.count > 0)
>
> ditto "} else if ()"
>
>> + dprintk("%s: LD read_pagelist returned count %d > 0\n",
>> + __FUNCTION__, status);
>
> ditto __FUNCTION__
> s/status/rdata->res.count/
>
> Thanks,
>
> Benny
>
>>
>> out:
>> dprintk("%s: End Status %d\n", __FUNCTION__, status);
>> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
>> index 8a720e5..655f1f7 100644
>> --- a/include/linux/nfs4_pnfs.h
>> +++ b/include/linux/nfs4_pnfs.h
>> @@ -115,8 +115,14 @@ struct layoutdriver_io_operations {
>> /* Functions that use the pagecache.
>> * If use_pagecache == 1, then these functions must be implemented.
>> */
>> - ssize_t (*read_pagelist) (struct pnfs_layout_type *layoutid, struct page **pages, unsigned int pgbase, unsigned nr_pages, loff_t offset, size_t count, struct nfs_read_data *nfs_data);
>> - ssize_t (*write_pagelist) (struct pnfs_layout_type *layoutid, struct page **pages, unsigned int pgbase, unsigned nr_pages, loff_t offset, size_t count, int sync, struct nfs_write_data *nfs_data);
>> + int (*read_pagelist) (struct pnfs_layout_type *layoutid,
>> + struct page **pages, unsigned int pgbase,
>> + unsigned nr_pages, loff_t offset, size_t count,
>> + struct nfs_read_data *nfs_data);
>> + int (*write_pagelist) (struct pnfs_layout_type *layoutid,
>> + struct page **pages, unsigned int pgbase,
>> + unsigned nr_pages, loff_t offset, size_t count,
>> + int sync, struct nfs_write_data *nfs_data);
>> int (*flush_one) (struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int how);
>> void (*free_request_data) (struct nfs_page *);
>>
>
>
More information about the pNFS
mailing list