[pnfs] [PATCH 02/10] pnfs: file layout module free_request_data
Benny Halevy
bhalevy at panasas.com
Thu Jan 17 09:36:28 EST 2008
dprintk's changes applied to the pnfs branch.
I'm waiting for your response with regards to my other comments.
Benny
On Jan. 17, 2008, 12:13 +0200, Benny Halevy <bhalevy at panasas.com> wrote:
> 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,
>
> _______________________________________________
> pNFS mailing list
> pNFS at linux-nfs.org
> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>
More information about the pNFS
mailing list