Re: [PATCH 06/18] pnfs_submit: nfs4_layoutreturn_release should not reference results

[Benny Halevy <bhalevy@panasas.com>]

On 2010-11-04 17:22, Fred Isaman wrote:
> Since the release function may be called without sending any RPC,
> it must should not refer to any of the result fields.  This is
          ^^^^^^

must not

> better accomplished in the rpc_done function.
> 
> In the process, this basically reverts the commit
> "pnfs: do not change layout stateid when dropping layouts."

Not exactly, as the !lrp->res.valid noop case is now handled with
the same outcome, just implemented differently.

Otherwise, this patch looks good.

Benny
> 
> Signed-off-by: Fred Isaman <iisaman@netapp.com>
> ---
>  fs/nfs/nfs4proc.c       |   17 ++++++++++++++---
>  fs/nfs/nfs4xdr.c        |    1 -
>  fs/nfs/pnfs.c           |   23 ++---------------------
>  fs/nfs/pnfs.h           |    3 ++-
>  include/linux/nfs_xdr.h |    1 -
>  5 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 8d3965c..de3ed2f 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5596,9 +5596,20 @@ static void nfs4_layoutreturn_done(struct rpc_task *task, void *calldata)
>  		server = NFS_SERVER(lrp->args.inode);
>  	else
>  		server = NULL;
> -	if (nfs4_async_handle_error(task, server, NULL, lrp->clp) == -EAGAIN)
> +	if (nfs4_async_handle_error(task, server, NULL, lrp->clp) == -EAGAIN) {
>  		nfs_restart_rpc(task, lrp->clp);
> +		return;
> +	}
> +	if ((task->tk_status == 0) && (lrp->args.return_type == RETURN_FILE)) {
> +		struct pnfs_layout_hdr *lo = NFS_I(lrp->args.inode)->layout;
>  
> +		spin_lock(&lo->inode->i_lock);
> +		if (lrp->res.lrs_present)
> +			pnfs_set_layout_stateid(lo, &lrp->res.stateid);
> +		else
> +			pnfs_invalidate_layout_stateid(lo);
> +		spin_unlock(&lo->inode->i_lock);
> +	}
>  	dprintk("<-- %s\n", __func__);
>  }
>  
> @@ -5607,8 +5618,8 @@ static void nfs4_layoutreturn_release(void *calldata)
>  	struct nfs4_layoutreturn *lrp = calldata;
>  
>  	dprintk("--> %s return_type %d\n", __func__, lrp->args.return_type);
> -
> -	pnfs_layoutreturn_release(lrp);
> +	if (lrp->args.return_type == RETURN_FILE)
> +		put_layout_hdr(lrp->args.inode);
>  	kfree(calldata);
>  	dprintk("<-- %s\n", __func__);
>  }
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index b71a482..10a6f4a 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -5286,7 +5286,6 @@ static int decode_layoutreturn(struct xdr_stream *xdr,
>  	p = xdr_inline_decode(xdr, 4);
>  	if (unlikely(!p))
>  		goto out_overflow;
> -	res->valid = true;
>  	res->lrs_present = be32_to_cpup(p);
>  	if (res->lrs_present)
>  		status = decode_stateid(xdr, &res->stateid);
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 34f6914..44f4f30 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -449,7 +449,7 @@ pnfs_destroy_all_layouts(struct nfs_client *clp)
>   *
>   * lo->stateid could be the open stateid, in which case we just use what given.
>   */
> -static void
> +void
>  pnfs_set_layout_stateid(struct pnfs_layout_hdr *lo,
>  			const nfs4_stateid *new)
>  {
> @@ -587,25 +587,6 @@ pnfs_return_layout_barrier(struct nfs_inode *nfsi,
>  	return ret;
>  }
>  
> -void
> -pnfs_layoutreturn_release(struct nfs4_layoutreturn *lrp)
> -{
> -	struct pnfs_layout_hdr *lo;
> -
> -	if (lrp->args.return_type != RETURN_FILE)
> -		return;
> -	lo = NFS_I(lrp->args.inode)->layout;
> -	spin_lock(&lrp->args.inode->i_lock);
> -	if (!lrp->res.valid)
> -		;	/* forgetful model internal release */
> -	else if (!lrp->res.lrs_present)
> -		pnfs_invalidate_layout_stateid(lo);
> -	else 
> -		pnfs_set_layout_stateid(lo, &lrp->res.stateid);
> -	put_layout_hdr_locked(lo); /* Matched in _pnfs_return_layout */
> -	spin_unlock(&lrp->args.inode->i_lock);
> -}
> -
>  static int
>  return_layout(struct inode *ino, struct pnfs_layout_range *range,
>  	      enum pnfs_layoutreturn_type type, struct pnfs_layout_hdr *lo,
> @@ -675,7 +656,7 @@ _pnfs_return_layout(struct inode *ino, struct pnfs_layout_range *range,
>  		list_for_each_entry_safe(lseg, tmp, &lo->segs, fi_list)
>  			if (should_free_lseg(lseg, &arg))
>  				mark_lseg_invalid(lseg, &tmp_list);
> -		/* Reference matched in pnfs_layoutreturn_release */
> +		/* Reference matched in nfs4_layoutreturn_release */
>  		get_layout_hdr_locked(lo);
>  		spin_unlock(&ino->i_lock);
>  		pnfs_free_lseg_list(&tmp_list);
> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
> index de4eaa8..f0232f5 100644
> --- a/fs/nfs/pnfs.h
> +++ b/fs/nfs/pnfs.h
> @@ -201,10 +201,11 @@ void pnfs_pageio_init_read(struct nfs_pageio_descriptor *, struct inode *,
>  			   struct nfs_open_context *, struct list_head *);
>  void pnfs_pageio_init_write(struct nfs_pageio_descriptor *, struct inode *);
>  int pnfs_layout_process(struct nfs4_layoutget *lgp);
> -void pnfs_layoutreturn_release(struct nfs4_layoutreturn *lpr);
>  void pnfs_destroy_layout(struct nfs_inode *);
>  void pnfs_destroy_all_layouts(struct nfs_client *);
>  void put_layout_hdr(struct inode *inode);
> +void pnfs_set_layout_stateid(struct pnfs_layout_hdr *lo,
> +			     const nfs4_stateid *new);
>  void pnfs_copy_layout_stateid(nfs4_stateid *dst, struct pnfs_layout_hdr *lo);
>  void pnfs_get_layout_stateid(nfs4_stateid *dst, struct pnfs_layout_hdr *lo,
>  			     struct nfs4_state *open_state);
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 0ee7cce..ebe11d3 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -270,7 +270,6 @@ struct nfs4_layoutreturn_args {
>  
>  struct nfs4_layoutreturn_res {
>  	struct nfs4_sequence_res seq_res;
> -	bool valid;	/* internal, true if received reply */
>  	u32 lrs_present;
>  	nfs4_stateid stateid;
>  };