[pnfs] [PATCH 01/25] pnfs: fix pnfs_{read|write}pages

[Benny Halevy]

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.

>  	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?

> +		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

> +		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?

> -		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 *);
>