Re: [PATCH 14/26] NFS: merge _full and _partial read rpc_ops

["Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx>]

On Mon, 2012-04-09 at 16:52 -0400, Fred Isaman wrote:
> Decouple nfs_pgio_header and nfs_read_data, and have (possibly
> multiple) nfs_read_datas each take a refcount on nfs_pgio_header.
> 
> For the moment keeps nfs_read_header as a way to preallocate a single
> nfs_read_data with the nfs_pgio_header.  The code doesn't need this,
> and would be prettier without, but given the amount of churn I am
> already introducing I didn't want to play with tuning new mempools.
> 
> This also fixes bug in pnfs_ld_handle_read_error.  In the case of
> desc->pg_bsize < PAGE_CACHE_SIZE, the pages list was empty, causing
> replay attempt to do nothing.
> 
> Signed-off-by: Fred Isaman <iisaman@xxxxxxxxxx>
> ---
<snip>
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index f6ab30b..ea844b2 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -30,29 +30,45 @@
>  #define NFSDBG_FACILITY		NFSDBG_PAGECACHE
>  
>  static const struct nfs_pageio_ops nfs_pageio_read_ops;
> -static const struct rpc_call_ops nfs_read_partial_ops;
> -static const struct rpc_call_ops nfs_read_full_ops;
> +static const struct rpc_call_ops nfs_read_common_ops;
>  
>  static struct kmem_cache *nfs_rdata_cachep;
>  
> -struct nfs_read_header *nfs_readhdr_alloc(unsigned int pagecount)
> +struct nfs_read_header *nfs_readhdr_alloc()
>  {
> -	struct nfs_read_header *p;
> +	struct nfs_read_header *rhdr;
>  
> -	p = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
> -	if (p) {
> -		struct nfs_pgio_header *hdr = &p->header;
> -		struct nfs_read_data *data = &p->rpc_data;
> +	rhdr = kmem_cache_zalloc(nfs_rdata_cachep, GFP_KERNEL);
> +	if (rhdr) {
> +		struct nfs_pgio_header *hdr = &rhdr->header;
>  
>  		INIT_LIST_HEAD(&hdr->pages);
> -		INIT_LIST_HEAD(&data->list);
> -		data->header = hdr;
> +		INIT_LIST_HEAD(&hdr->rpc_list);
> +		spin_lock_init(&hdr->lock);
> +		atomic_set(&hdr->refcnt, 0);
> +	}
> +	return rhdr;
> +}
> +
> +struct nfs_read_data *nfs_readdata_alloc(struct nfs_pgio_header *hdr,
> +					 unsigned int pagecount)
> +{
> +	struct nfs_read_data *data;
> +
> +	if (hdr) {
> +		/* Use data preallocated with the header */
> +		data = &container_of(hdr, struct nfs_read_header, header)->rpc_data;
> +	} else
> +		data = kzalloc(sizeof(*data), GFP_KERNEL);
> +
> +	if (data) {
>  		if (!nfs_pgarray_set(&data->pages, pagecount)) {
> -			kmem_cache_free(nfs_rdata_cachep, p);
> -			p = NULL;
> +			if (!hdr)
> +				kfree(data);
> +			data = NULL;
>  		}
>  	}
> -	return p;
> +	return data;
>  }

BTW: It makes me nervous that we can allocate a struct nfs_read_data
that does not have data->header set. Could we please always pass an
nfs_pgio_header argument?

If you make sure that the above function always bumps header->refcnt,
then you can check if header->rpc_data is allocated by seeing if
header->refcnt is zero.

BTW: The same applies to the "write" equivalents...

>  void nfs_readhdr_free(struct nfs_pgio_header *hdr)
> @@ -64,10 +80,16 @@ void nfs_readhdr_free(struct nfs_pgio_header *hdr)
>  
>  void nfs_readdata_release(struct nfs_read_data *rdata)
>  {
> +	struct nfs_pgio_header *hdr = rdata->header;
> +
>  	put_nfs_open_context(rdata->args.context);
>  	if (rdata->pages.pagevec != rdata->pages.page_array)
>  		kfree(rdata->pages.pagevec);
> -	nfs_readhdr_free(rdata->header);
> +	if (container_of(hdr, struct nfs_read_header, header) !=
> +	    container_of(rdata, struct nfs_read_header, rpc_data))
> +		kfree(rdata);
> +	if (atomic_dec_and_test(&hdr->refcnt))
> +		nfs_read_completion(hdr);
>  }
>  
>  static
> @@ -79,35 +101,6 @@ int nfs_return_empty_page(struct page *page)
>  	return 0;
>  }


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com

��.n��������+%������w��{.n�����{��w���jg��������ݢj����G�������j:+v���w�m������w�������h�����٥