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�����٥
