[pnfs] [PATCH 02/10] pnfs: file layout module free_request_data
William A. (Andy) Adamson
andros at citi.umich.edu
Thu Jan 17 10:00:58 EST 2008
On Jan 17, 2008 5:13 AM, 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)
ok
>
>
>
> > 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.
yes, i like your way better.
>
> > 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?
isn't required as this is called only when the request is released.
i'll break the patches up as suggested. thanks for the review
-->Andy
>
>
> > +}
> > +
> > /* 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://linux-nfs.org/pipermail/pnfs/attachments/20080117/0178745c/attachment-0001.htm
More information about the pNFS
mailing list