[pnfs] [PATCH 02/10] pnfs: file layout module free_request_data

[Benny Halevy]

On Jan. 17, 2008, 0:52 +0200, andros at umich.edu wrote:
> From: Andy Adamson <andros at umich.edu>
> 
> Use the free_request_data layoutdriver I/O operation to free the struct
> nfs4_pnfs_dserver kalloc'ed in filelayout_flush_one().
> 
> Signed-off-by: Andy Adamson<andros at umich.edu>
> ---
>  fs/nfs/nfs4filelayout.c |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
> index adda7ea..c8e3be3 100644
> --- a/fs/nfs/nfs4filelayout.c
> +++ b/fs/nfs/nfs4filelayout.c
> @@ -83,6 +83,7 @@ struct pnfs_client_operations *pnfs_callback_ops;
>  
>  /* Forward declaration */
>  ssize_t filelayout_get_stripesize(struct pnfs_layout_type *);
> +struct layoutdriver_io_operations filelayout_io_operations;
>  
>  /* Initialize a mountpoint by retrieving the list of
>   * available devices for it.
> @@ -360,11 +361,11 @@ filelayout_create_dserver(void)
>  {
>  	struct nfs4_pnfs_dserver *local;
>  
> -	dprintk("--> %s\n", __func__);
>  	local = kzalloc(sizeof(*local), GFP_KERNEL);
>  	if (!local)
>  		return NULL;
>  	kref_init(&local->ref);
> +	dprintk("<-- %s dserver %p\n", __func__, local);

Andy, I suggest splitting these dprintk cleanups here into a separate
patch since they're functionally orthogonal to free_request_data...
If I hear no objection I'll go ahead and do that when applying this patchset.

>  	return local;
>  }
>  
> @@ -379,13 +380,11 @@ static void filelayout_free_dserver(struct kref *kref)
>  
>  static void filelayout_release_dserver(struct nfs4_pnfs_dserver *dserver)
>  {
> -	dprintk("--> %s dserver %p\n", __func__, dserver);
>  	kref_put(&dserver->ref, filelayout_free_dserver);
>  }
>  
>  static void filelayout_get_dserver(struct nfs4_pnfs_dserver *dserver)
>  {
> -	dprintk("--> %s\n", __func__);
>  	kref_get(&dserver->ref);
>  }
>  
> @@ -445,6 +444,7 @@ next_ds:
>  		if (status != 0) {
>  			dprintk("%s failed to get dataserver. status %d\n",
>  						__FUNCTION__, status);
> +			filelayout_release_dserver(dserver);

That's fixing an unrelated memory leak, right?

Seems like it requires own patch (this also makes it much easier
for me to reorder the patches and merge them with the respective
patches they are fixing)


>  			status =  -EIO;
>  			goto out;
>  		}
> @@ -452,15 +452,17 @@ next_ds:
>  		ds = dserver->dev->ds_list[0];
>  
>  		use_ds = 1;
> +		goto skip_get;
>  use_ds:
>  		filelayout_get_dserver(dserver);
> -
> +skip_get:

Eh?
The code looks really weird this way...
Seems like you need to refactor this function. I'd take the section
that creates and initializes the dserver out into a different function
and acll it once for the use_ds==0 case and set use_ds = 1.
Otherwise, just call filelayout_get_dserver.

How about something on these lines:

static struct nfs4_pnfs_ds *
filelayout_create_init_ds(inode, nfslay, dsoffset, stripesz, *dsp)
{
	struct nfs4_pnfs_dserver *dserver;
	int status = -ENOMEM;

	*dsp = dserver = filelayout_create_dserver();
	if (!dserver) {
		dprintk("%s failed to get dserver. status %d\n",
					__FUNCTION__, status);
		goto err;
	}

	/* get the data server that serves this stripe */
	status = nfs4_pnfs_dserver_get(inode, nfslay, dsoffset,
			stripesz, dserver);

	if (status != 0) {
		dprintk("%s failed to get dataserver. status %d\n",
					__FUNCTION__, status);
		filelayout_release_dserver(dserver);
		status =  -EIO;
		goto err;
	}

	/* just try the first multipath data server */
	return dserver->dev->ds_list[0];

err:
	return ERR_PTR(status);
}

and in filelayout_flush_one:

	while (!list_empty(head)) {
next_ds:
		req = nfs_list_entry(head->next);

	if (!use_ds) {
		ds = filelayout_create_init_ds(inode, nfslay, dsoffset, stripesz, &dserver);
		if (IS_ERR(ds)) {
			status = PTR_ERR(ds);
			goto out;
		}
		use_ds = 1;
	} else
		filelayout_get_dserver(dserver);

Ah, and separate patch of course.

>  		reqcount = count < PAGE_SIZE? count: PAGE_SIZE;
>  		count -= reqcount;
>  		dstotal += reqcount;
>  
>  		req->wb_devip = ds->ds_ip_addr;
>  		req->wb_devport = ds->ds_port;
> +		req->wb_ops = &filelayout_io_operations;
>  		req->wb_private = dserver;
>  
>  		/* move request to dslist */
> @@ -477,11 +479,6 @@ use_ds:
>  			goto send;
>  		}
>  	}
> -	if (!ds) {
> -		status = -EIO;
> -		goto out;
> -	}
> -

this can be part of the refactoring patch...

>  send:
>  	/* XXX should recover to send through MDS */
>  	dprintk("%s Send: ndspages %d dstotal %Zd list_empty(head) %d \n",
> @@ -507,6 +504,19 @@ out:
>  	return status;
>  }
>  
> +/*
> + * Called by nfs_release_request()
> + */
> +void
> +filelayout_free_request_data(struct nfs_page *req)
> +{
> +	struct nfs4_pnfs_dserver *dserver;
> +
> +	dserver = (struct nfs4_pnfs_dserver *)req->wb_private;
> +	BUG_ON(!dserver);
> +	filelayout_release_dserver(dserver);

should req->wb_private be set to NULL after releasing its contents?

> +}
> +
>  /* Perform async writes.
>   *
>   * TODO: See filelayout_read_pagelist.
> @@ -565,8 +575,6 @@ ssize_t filelayout_write_pagelist(
>  	nfs_initiate_write(data, data->pnfs_client,
>  			&filelayout_write_call_ops, sync);
>  
> -	filelayout_release_dserver(dserver);
> -
>  	return 0;
>  }
>  
> @@ -829,6 +837,7 @@ struct layoutdriver_io_operations filelayout_io_operations = {
>  	.read_pagelist           = filelayout_read_pagelist,
>  	.write_pagelist          = filelayout_write_pagelist,
>  	.flush_one		 = filelayout_flush_one,
> +	.free_request_data	 = filelayout_free_request_data,
>  	.alloc_layout            = filelayout_alloc_layout,
>  	.free_layout             = filelayout_free_layout,
>  	.alloc_lseg              = filelayout_alloc_lseg,