[pnfs] [PATCH 10/11] pnfs: remove error return from nfs_read_rpcsetup

[Benny Halevy]

On Apr. 10, 2008, 17:05 +0300, Fred Isaman <iisaman at citi.umich.edu> wrote:
> This is expected to be an async call, in fact nfs_pagein_multi relies
> on this.  Further, if my understanding of the code is correct, "passing
> the error up" is pointless, since they are eventually ignored.
> do_generic_file_read calls nfs_readpages through the readahead code,
> ignoring any error there because it is just readahead.  Later, a cache
> miss will cause nfs_readpage to be called, but nfs_readpage_async ignores
> any errors and always returns 0
> 
> Signed-off-by: Fred Isaman <iisaman at citi.umich.edu>

NACK for now.
As mentioned on previous patch, I'm looking into this
and will propose a generic fix to Trond.

Benny

> ---
>  fs/nfs/internal.h |    2 +-
>  fs/nfs/read.c     |   42 ++++++++++++++----------------------------
>  2 files changed, 15 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 07eea8e..00afd05 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -203,7 +203,7 @@ extern int nfs4_path_walk(struct nfs_server *server,
>  #endif
>  
>  /* read.c */
> -extern int nfs_initiate_read(struct nfs_read_data *data, struct rpc_clnt *clnt,
> +extern void nfs_initiate_read(struct nfs_read_data *data, struct rpc_clnt *clnt,
>  			     const struct rpc_call_ops *call_ops);
>  extern int nfs_read_validate(struct rpc_task *task, void *calldata);
>  extern void nfs_readdata_release(void *data);
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index 019abb9..7381951 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -159,7 +159,7 @@ static void nfs_readpage_release(struct nfs_page *req)
>  	nfs_release_request(req);
>  }
>  
> -int nfs_initiate_read(struct nfs_read_data *data, struct rpc_clnt *clnt,
> +void nfs_initiate_read(struct nfs_read_data *data, struct rpc_clnt *clnt,
>  		      const struct rpc_call_ops *call_ops)
>  {
>  	struct inode *inode = data->inode;
> @@ -190,22 +190,19 @@ int nfs_initiate_read(struct nfs_read_data *data, struct rpc_clnt *clnt,
>  			(unsigned long long)data->args.offset);
>  
>  	task = rpc_run_task(&task_setup_data);
> -	if (unlikely(IS_ERR(task)))
> -		return PTR_ERR(task);
> -	rpc_put_task(task);
> -	return 0;
> +	if (!IS_ERR(task))
> +		rpc_put_task(task);
>  }
>  EXPORT_SYMBOL(nfs_initiate_read);
>  
>  /*
>   * Set up the NFS read request struct
>   */
> -static int nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data,
> +static void nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data,
>  		const struct rpc_call_ops *call_ops,
>  		unsigned int count, unsigned int offset)
>  {
>  	struct inode *inode = req->wb_context->path.dentry->d_inode;
> -	int ret;
>  
>  	data->req	  = req;
>  	data->inode	  = inode;
> @@ -223,11 +220,9 @@ static int nfs_read_rpcsetup(struct nfs_page *req, struct nfs_read_data *data,
>  	data->res.eof     = 0;
>  	nfs_fattr_init(&data->fattr);
>  
> -	ret = pnfs_try_to_read_data(data, call_ops);
> -	if (ret == 0)
> -		return data->pnfs_error;
> -
> -	return nfs_initiate_read(data, NFS_CLIENT(inode), call_ops);
> +	if (pnfs_try_to_read_data(data, call_ops) == 0)
> +		return;
> +	nfs_initiate_read(data, NFS_CLIENT(inode), call_ops);
>  }
>  
>  static void
> @@ -263,7 +258,7 @@ static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigne
>  	struct nfs_read_data *data;
>  	size_t rsize = NFS_SERVER(inode)->rsize, nbytes;
>  	unsigned int offset;
> -	int requests = 0, status = -ENOMEM;
> +	int requests = 0;
>  	LIST_HEAD(list);
>  
>  	nfs_list_remove_request(req);
> @@ -294,15 +289,8 @@ static int nfs_pagein_multi(struct inode *inode, struct list_head *head, unsigne
>  
>  		if (nbytes < rsize)
>  			rsize = nbytes;
> -		status = nfs_read_rpcsetup(req, data, &nfs_read_partial_ops,
> -					   rsize, offset);
> -		if (status) {
> -			/* FIXME - this is broken. Cleanup code assumes
> -			 * nothing  outstanding.  Further, if triggered on
> -			 * last list entry, req will be double-freed.
> -			 */
> -			goto out_bad;
> -		}
> +		nfs_read_rpcsetup(req, data, &nfs_read_partial_ops,
> +				  rsize, offset);
>  		offset += rsize;
>  		nbytes -= rsize;
>  	} while (nbytes != 0);
> @@ -317,7 +305,7 @@ out_bad:
>  	}
>  	SetPageError(page);
>  	nfs_readpage_release(req);
> -	return status;
> +	return -ENOMEM;
>  }
>  
>  static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned int npages, size_t count, int flags)
> @@ -325,7 +313,6 @@ static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned
>  	struct nfs_page		*req;
>  	struct page		**pages;
>  	struct nfs_read_data	*data;
> -	int			status = -ENOMEM;
>  
>  	data = nfs_readdata_alloc(npages);
>  	if (!data)
> @@ -343,12 +330,11 @@ static int nfs_pagein_one(struct inode *inode, struct list_head *head, unsigned
>  	}
>  	req = nfs_list_entry(data->pages.next);
>  
> -	status = nfs_read_rpcsetup(req, data, &nfs_read_full_ops, count, 0);
> -	if (!status)
> -		return 0;
> +	nfs_read_rpcsetup(req, data, &nfs_read_full_ops, count, 0);
> +	return 0;
>  out_bad:
>  	nfs_async_read_error(head);
> -	return status;
> +	return -ENOMEM;
>  }
>  
>  /*