Re: [PATCH 13/24] pnfs_submit: read path changeover

[Boaz Harrosh <bharrosh@panasas.com>]

On 06/08/2010 07:19 AM, Fred Isaman wrote:
> Change readpages path to only call LAYOUTGET once.
> 
> Signed-off-by: Fred Isaman <iisaman@netapp.com>
> ---
>  fs/nfs/pagelist.c |    2 ++
>  fs/nfs/pnfs.c     |   37 +++++++------------------------------
>  fs/nfs/pnfs.h     |   25 ++++++++++++++++---------
>  3 files changed, 25 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index ed647b9..c3e5a1f 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -253,6 +253,8 @@ static int nfs_can_coalesce_requests(struct nfs_page *prev,
>  		return 0;
>  	if (prev->wb_pgbase + prev->wb_bytes != PAGE_CACHE_SIZE)
>  		return 0;
> +	if (req->wb_lseg != prev->wb_lseg)
> +		return 0;
>  #ifdef CONFIG_NFS_V4_1
>  	if (pgio->pg_test && !pgio->pg_test(pgio, prev, req))
>  		return 0;
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 2b5f6fc..692a18e 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -1689,7 +1689,7 @@ pnfs_readpages(struct nfs_read_data *rdata)
>  {
>  	struct nfs_readargs *args = &rdata->args;
>  	struct inode *inode = rdata->inode;
> -	int numpages, status, pgcount, temp;
> +	int numpages, pgcount, temp;
>  	struct nfs_server *nfss = NFS_SERVER(inode);
>  	struct nfs_inode *nfsi = NFS_I(inode);
>  	struct pnfs_layout_segment *lseg;
> @@ -1701,19 +1701,8 @@ pnfs_readpages(struct nfs_read_data *rdata)
>  		args->count,
>  		args->offset);
>  
> -	/* Retrieve and set layout if not allready cached */
> -	status = _pnfs_update_layout(inode,
> -				    args->context,
> -				    args->count,
> -				    args->offset,
> -				    IOMODE_READ,
> -				    &lseg);
> -	if (status) {
> -		dprintk("%s: Updating layout failed (%d), retry with NFS \n",
> -			__func__, status);
> -		trypnfs = PNFS_NOT_ATTEMPTED;
> -		goto out;
> -	}
> +	lseg = rdata->req->wb_lseg;
> +	get_lseg(lseg);
>  
>  	/* Determine number of pages. */
>  	pgcount = args->pgbase + args->count;
> @@ -1740,7 +1729,6 @@ pnfs_readpages(struct nfs_read_data *rdata)
>  		rdata->pdata.lseg = NULL;
>  		put_lseg(lseg);
>  	}
> - out:
>  	dprintk("%s End (trypnfs:%d)\n", __func__, trypnfs);
>  	return trypnfs;
>  }
> @@ -1749,21 +1737,10 @@ enum pnfs_try_status
>  _pnfs_try_to_read_data(struct nfs_read_data *data,
>  		       const struct rpc_call_ops *call_ops)
>  {
> -	struct inode *ino = data->inode;
> -	struct nfs_server *nfss = NFS_SERVER(ino);
> -
> -	dprintk("--> %s\n", __func__);
> -	/* Only create an rpc request if utilizing NFSv4 I/O */
> -	if (!pnfs_enabled_sb(nfss) ||
> -	    !nfss->pnfs_curr_ld->ld_io_ops->read_pagelist) {
> -		dprintk("<-- %s: not using pnfs\n", __func__);
> -		return PNFS_NOT_ATTEMPTED;
> -	} else {
> -		dprintk("%s: Utilizing pNFS I/O\n", __func__);
> -		data->pdata.call_ops = call_ops;
> -		data->pdata.pnfs_error = 0;
> -		return pnfs_readpages(data);
> -	}

Wahoo, nice stuff Ha

By now this can go into it's only caller. 
(And caller de-inlined, what was that all about)

> +	dprintk("%s: Utilizing pNFS I/O\n", __func__);
> +	data->pdata.call_ops = call_ops;
> +	data->pdata.pnfs_error = 0;
> +	return pnfs_readpages(data);
>  }
>  
>  enum pnfs_try_status
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index 6326ed5..816ebe1 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -94,22 +94,29 @@ static inline int pnfs_enabled_sb(struct nfs_server *nfss)
>  	return nfss->pnfs_curr_ld != NULL;
>  }
>  
> +static inline void _pnfs_clear_lseg_from_pages(struct list_head *head)
> +{
> +	struct nfs_page *req;
> +
> +	list_for_each_entry(req, head, wb_list) {
> +		put_lseg(req->wb_lseg);
> +		req->wb_lseg = NULL;
> +	}
> +}
> +
>  static inline enum pnfs_try_status
>  pnfs_try_to_read_data(struct nfs_read_data *data,
>  		      const struct rpc_call_ops *call_ops)

Don't think this needs to be inline, whats the point?

>  {
> -	struct inode *inode = data->inode;
> -	struct nfs_server *nfss = NFS_SERVER(inode);
>  	enum pnfs_try_status ret;
>  
> -	/* FIXME: read_pagelist should probably be mandated */
> -	if (PNFS_EXISTS_LDIO_OP(nfss, read_pagelist))
> -		ret = _pnfs_try_to_read_data(data, call_ops);
> -	else
> -		ret = PNFS_NOT_ATTEMPTED;
> -
> +	if (!data->req->wb_lseg)
> +		return PNFS_NOT_ATTEMPTED;
> +	ret = _pnfs_try_to_read_data(data, call_ops);
>  	if (ret == PNFS_ATTEMPTED)
> -		nfs_inc_stats(inode, NFSIOS_PNFS_READ);
> +		nfs_inc_stats(data->inode, NFSIOS_PNFS_READ);
> +	else
> +		_pnfs_clear_lseg_from_pages(&data->pages);
>  	return ret;
>  }
>  

Boaz