[pnfs] [PATCH 2/3] nfs41: Allow NFSv4 and NFSv4.1 callbackservices to coexist
William A. (Andy) Adamson
androsadamson at gmail.com
Wed Jun 25 16:32:07 EDT 2008
Hi Guys
OK - I'll look into the recovery code bug - I bet the mount/unmount
DESTROY_SESSION bug is related....
-->Andy
On Wed, Jun 25, 2008 at 4:14 PM, Labiaga, Ricardo <
Ricardo.Labiaga at netapp.com> wrote:
> I can look at the mount/mount/unmount DESTROY_SESSION bug.
>
> Andy, since you've been working on the recovery code, do you want to
> look at the third problem?
>
> - ricardo
>
>
> > -----Original Message-----
> > From: Halevy, Benny [mailto:bhalevy at panasas.com]
> > Sent: Wednesday, June 25, 2008 1:08 PM
> > To: Labiaga, Ricardo; Adamson, Andy
> > Cc: pnfs at linux-nfs.org
> > Subject: RE: [pnfs] [PATCH 2/3] nfs41: Allow NFSv4 and
> > NFSv4.1 callbackservices to coexist
> >
> > No need for an updated patch...
> > I can just squash the one I included in my reply...
> >
> > I've reworked my patchset based on Trond's feedback and
> > rebased the tree over 2.6.26-rc8.
> > It now passes connectathon tests with your patches on top
> > against both a 4.1 server and a 4.0 server with automatic fallback.
> >
> > I'm still seeing a few other bugs but I'll push the patches to the
> > public tree anyway.
> >
> > One of the bugs happens when running the cthon tests from a
> > directory mounted over nfsv3 against a v4.0 server.
> > After looking up the test directory on the test mount, the test can't
> > cd to the "basic" directory from the cwd. I'll continue working
> > on that tomorrow.
> >
> > The other bug happens when mounting 4.1 twice on an existing
> > mount point and then umounting:
> >
> > [root at buml ~]# mount -t nfs4 localhost:/ /mnt/localhost
> > [root at buml ~]# mount -t nfs4 localhost:/ /mnt/localhost
> > [root at buml ~]# umount /mnt/localhost
> > Got error -10052 from the server on DESTROY_SESSION. Session
> > has been destroyed regardless...
> >
> > (10052 is NFSERR_BADSESSION)
> >
> > The third might be related and is more serious.
> > Reproducable when running the cthon test twice over 4.1
> > (without umounting in between). session recovery hits this:
> >
> > Jun 25 16:02:39 buml kernel: Got error -10052 from the server
> > on DESTROY_SESSION
> > . Session has been destroyed regardless...
> > Jun 25 16:02:39 buml kernel: Error: session recovery failed
> > on NFSv4.1 server wi
> > th error -10022
> > Jun 25 16:02:39 buml kernel: ------------[ cut here ]------------
> > Jun 25 16:02:39 buml kernel: WARNING: at
> > /usr0/export/dev/bhalevy/git/linux-pnfs
> > -bh-nfs41/fs/nfs/nfs4proc.c:4566
> > nfs4_proc_create_session+0x3bc/0x575 [nfs]()
> > Jun 25 16:02:39 buml kernel: Modules linked in: nfsd
> > auth_rpcgss exportfs nfs lo
> > ckd nfs_acl sunrpc
> > Jun 25 16:02:39 buml kernel: Call Trace:
> > Jun 25 16:02:39 buml kernel: 6e899c98: [<6002f471>]
> > warn_on_slowpath+0x54/0x8e
> > Jun 25 16:02:39 buml kernel: 6e899cc8: [<60005d73>]
> > free_bootmem_core+0x63/0xd4
> > Jun 25 16:02:39 buml kernel: 6e899cd8: [<6005800f>]
> > mempool_free+0x75/0x7d
> > Jun 25 16:02:39 buml kernel: 6e899d28: [<7103cd7c>]
> > nfs4_proc_create_session+0x2fa/0x575 [nfs]
> > Jun 25 16:02:39 buml kernel: 6e899d38: [<60073e8c>]
> > cache_alloc_debugcheck_after+0x110/0x1b2
> > Jun 25 16:02:39 buml kernel: 6e899d68: [<7103cd7c>]
> > nfs4_proc_create_session+0x2fa/0x575 [nfs]
> > Jun 25 16:02:39 buml kernel: 6e899db8: [<7103ce3e>]
> > nfs4_proc_create_session+0x3bc/0x575 [nfs]
> > Jun 25 16:02:39 buml kernel: 6e899e68: [<7104d1d4>]
> > session_reclaimer+0x0/0x154 [nfs]
> > Jun 25 16:02:39 buml kernel: 6e899ed8: [<7104d287>]
> > session_reclaimer+0xb3/0x154 [nfs]
> > Jun 25 16:02:39 buml kernel: 6e899ef8: [<7104d1d4>]
> > session_reclaimer+0x0/0x154 [nfs]
> > Jun 25 16:02:39 buml kernel: 6e899f08: [<60042f54>] kthread+0x56/0x8f
> > Jun 25 16:02:39 buml kernel: 6e899f48: [<60021cc9>]
> > run_kernel_thread+0x41/0x4a
> > Jun 25 16:02:39 buml kernel: 6e899f58: [<60042efe>] kthread+0x0/0x8f
> > Jun 25 16:02:39 buml kernel: 6e899f98: [<60021cb0>]
> > run_kernel_thread+0x28/0x4a
> > Jun 25 16:02:39 buml kernel: 6e899fc8: [<60013829>]
> > new_thread_handler+0x72/0x9c
> > Jun 25 16:02:39 buml kernel:
> > Jun 25 16:02:39 buml kernel: ---[ end trace 59c37dcd76de288e ]---
> >
> > and although this is a WARN_ON the machince never really recovers.
> > Benny
> >
> > ________________________________
> >
> > From: Ricardo Labiaga [mailto:ricardo.labiaga at netapp.com]
> > Sent: Wed 2008-06-25 22:46
> > To: Halevy, Benny
> > Cc: pnfs at linux-nfs.org
> > Subject: Re: [pnfs] [PATCH 2/3] nfs41: Allow NFSv4 and
> > NFSv4.1 callbackservices to coexist
> >
> >
> >
> > On Wed, 2008-06-25 at 11:49 +0300, Benny Halevy wrote:
> > > On Jun. 25, 2008, 2:17 +0300, "Labiaga, Ricardo"
> > <Ricardo.Labiaga at netapp.com> wrote:
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Benny Halevy [mailto:bhalevy at panasas.com]
> > > >> Sent: Tuesday, June 24, 2008 1:57 PM
> > > >> To: Labiaga, Ricardo
> > > >> Cc: pnfs at linux-nfs.org
> > > >> Subject: Re: [pnfs] [PATCH 2/3] nfs41: Allow NFSv4 and
> > > >> NFSv4.1 callback services to coexist
> > > >>
> > > >> Ricardo Labiaga wrote:
> > > >>> Tracks the nfs_callback_info for both versions, enabling
> > > >> the callback
> > > >>> service for v4 and v4.1 to run concurrently and be stopped
> > > >> independently
> > > >>> of each other.
> > > >>>
> > > >>> Signed-off-by: Ricardo Labiaga <ricardo.labiaga at netapp.com>
> > > >>> ---
> > > >>> fs/nfs/callback.c | 37 ++++++++++++++++++++-----------------
> > > >>> fs/nfs/callback.h | 2 +-
> > > >>> fs/nfs/client.c | 2 +-
> > > >>> 3 files changed, 22 insertions(+), 19 deletions(-)
> > > >>>
> > > >>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > > >>> index f41d64e..17efb85 100644
> > > >>> --- a/fs/nfs/callback.c
> > > >>> +++ b/fs/nfs/callback.c
> > > >>> @@ -34,7 +34,7 @@ struct nfs_callback_data {
> > > >>> struct task_struct *task;
> > > >>> };
> > > >>>
> > > >>> -static struct nfs_callback_data nfs_callback_info;
> > > >>> +static struct nfs_callback_data
> > > >> nfs_callback_info[NFS4_MAX_MINOR_VERSION + 1];
> > > >>> static DEFINE_MUTEX(nfs_callback_mutex);
> > > >>> static struct svc_program nfs4_callback_program;
> > > >>>
> > > >>> @@ -57,7 +57,7 @@ module_param_call(callback_tcpport,
> > > >> param_set_port, param_get_int,
> > > >>> &nfs_callback_set_tcpport, 0644);
> > > >>>
> > > >>> /*
> > > >>> - * This is the callback kernel thread.
> > > >>> + * This is the NFSv4 callback kernel thread.
> > > >>> */
> > > >>> static int
> > > >>> nfs4_callback_svc(void *vrqstp)
> > > >>> @@ -94,7 +94,7 @@ nfs4_callback_svc(void *vrqstp)
> > > >>> svc_process(rqstp);
> > > >>> }
> > > >>> unlock_kernel();
> > > >>> - nfs_callback_info.task = NULL;
> > > >>> + nfs_callback_info[0].task = NULL;
> > > >>> svc_exit_thread(rqstp);
> > > >>> return 0;
> > > >>> }
> > > >>> @@ -159,7 +159,7 @@ nfs41_callback_svc(void *vrqstp)
> > > >>> finish_wait(&serv->sv_cb_waitq, &wq);
> > > >>> }
> > > >>> unlock_kernel();
> > > >>> - nfs_callback_info.task = NULL;
> > > >>> + nfs_callback_info[1].task = NULL;
> > > >>> svc_exit_thread(rqstp);
> > > >>> return 0;
> > > >>> }
> > > >>> @@ -214,10 +214,11 @@ int nfs_callback_up(u32 minorversion,
> > > >> void *args)
> > > >>>
> > > >>> lock_kernel();
> > > >>> mutex_lock(&nfs_callback_mutex);
> > > >>> - if (nfs_callback_info.users++ || nfs_callback_info.task
> > > >> != NULL) {
> > > >>> + if (nfs_callback_info[minorversion].users++ ||
> > > >>> + nfs_callback_info[minorversion].task
> > != NULL) {
> > > >>> #if defined(CONFIG_NFS_V4_1)
> > > >>> if (minorversion)
> > > >>> - xprt->bc_serv = nfs_callback_info.serv;
> > > >>> + xprt->bc_serv =
> > > >> nfs_callback_info[minorversion].serv;
> > > >>> #endif /* CONFIG_NFS_V4_1 */
> > > >>> goto out;
> > > >>> }
> > > >>> @@ -248,13 +249,14 @@ int nfs_callback_up(u32 minorversion,
> > > >> void *args)
> > > >>> }
> > > >>>
> > > >>> svc_sock_update_bufs(serv);
> > > >>> - nfs_callback_info.serv = serv;
> > > >>> + nfs_callback_info[minorversion].serv = serv;
> > > >>> sprintf(svc_name, "nfsv4.%u-svc", minorversion);
> > > >>> - nfs_callback_info.task = kthread_run(callback_svc,
> > > >> rqstp, svc_name);
> > > >>> - if (IS_ERR(nfs_callback_info.task)) {
> > > >>> - ret = PTR_ERR(nfs_callback_info.task);
> > > >>> - nfs_callback_info.serv = NULL;
> > > >>> - nfs_callback_info.task = NULL;
> > > >>> + nfs_callback_info[minorversion].task =
> > > >>> + kthread_run(callback_svc, rqstp, svc_name);
> > > >>> + if (IS_ERR(nfs_callback_info[minorversion].task)) {
> > > >>> + ret = PTR_ERR(nfs_callback_info[minorversion].task);
> > > >>> + nfs_callback_info[minorversion].serv = NULL;
> > > >>> + nfs_callback_info[minorversion].task = NULL;
> > > >>> svc_exit_thread(rqstp);
> > > >>> goto out_err;
> > > >>> }
> > > >>> @@ -273,20 +275,21 @@ out:
> > > >>> out_err:
> > > >>> dprintk("Couldn't create callback socket or server
> > > >> thread; err = %d\n",
> > > >>> ret);
> > > >>> - nfs_callback_info.users--;
> > > >>> + nfs_callback_info[minorversion].users--;
> > > >> Since nfs_callback_info[minorversion] is used extensively,
> > > >> I'd consider defining a struct nfs_callback_data *
> > > >> and initializing it once to &nfs_callback_info[minorversion];
> > > >
> > > > Do you think this is more readable, or you're concerned about
> > > > performance? I'm told that modern compilers optimize the
> > dereferencing
> > > > of the indexed element.
> > >
> > > In general I'm actually worried about both (plus machine code size),
> > > though in this particular case performance is not really a
> > problem since
> > > this code is very rarely called. gcc does not do a good job with
> > > optimizing this (at least with the optimization options
> > used by the kernel
> > > Makefiles)
> > >
> > > The following patch reduces the text segment size by 40 bytes while
> > > improving readability...
> > >
> >
> > We could argue about readability for ever on this one :-)
> > I'll send an
> > updated patch shortly.
> >
> > - ricardo
> >
> >
> > > Benny
> > >
> > > >From 09a9c0f31bbcf981a63b1fd4af0f5911bd23a7a7 Mon Sep 17
> > 00:00:00 2001
> > > From: Benny Halevy <bhalevy at panasas.com>
> > > Date: Wed, 25 Jun 2008 11:44:04 +0300
> > > Subject: [PATCH] [SQUASHME] nfs41: optimize use of
> > nfs_callback_info[minorversion]
> > >
> > > Signed-off-by: Benny Halevy <bhalevy at panasas.com>
> > > ---
> > > fs/nfs/callback.c | 32 +++++++++++++++++---------------
> > > 1 files changed, 17 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> > > index 3599abc..fa52ca9 100644
> > > --- a/fs/nfs/callback.c
> > > +++ b/fs/nfs/callback.c
> > > @@ -206,6 +206,7 @@ int nfs_callback_up(u32 minorversion,
> > void *args)
> > > struct svc_serv *serv = NULL;
> > > struct svc_rqst *rqstp;
> > > int (* callback_svc)(void *vrqstp);
> > > + struct nfs_callback_data *cb_info =
> > &nfs_callback_info[minorversion];
> > > char svc_name[12];
> > > int ret = 0;
> > > #if defined(CONFIG_NFS_V4_1)
> > > @@ -214,11 +215,11 @@ int nfs_callback_up(u32 minorversion,
> > void *args)
> > >
> > > lock_kernel();
> > > mutex_lock(&nfs_callback_mutex);
> > > - if (nfs_callback_info[minorversion].users++ ||
> > > - nfs_callback_info[minorversion].task
> > != NULL) {
> > > + if (cb_info->users++ ||
> > > + cb_info->task != NULL) {
> > > #if defined(CONFIG_NFS_V4_1)
> > > if (minorversion)
> > > - xprt->bc_serv =
> > nfs_callback_info[minorversion].serv;
> > > + xprt->bc_serv = cb_info->serv;
> > > #endif /* CONFIG_NFS_V4_1 */
> > > goto out;
> > > }
> > > @@ -250,14 +251,13 @@ int nfs_callback_up(u32 minorversion,
> > void *args)
> > > }
> > >
> > > svc_sock_update_bufs(serv);
> > > - nfs_callback_info[minorversion].serv = serv;
> > > + cb_info->serv = serv;
> > > sprintf(svc_name, "nfsv4.%u-svc", minorversion);
> > > - nfs_callback_info[minorversion].task =
> > > - kthread_run(callback_svc, rqstp, svc_name);
> > > - if (IS_ERR(nfs_callback_info[minorversion].task)) {
> > > - ret = PTR_ERR(nfs_callback_info[minorversion].task);
> > > - nfs_callback_info[minorversion].serv = NULL;
> > > - nfs_callback_info[minorversion].task = NULL;
> > > + cb_info->task = kthread_run(callback_svc, rqstp, svc_name);
> > > + if (IS_ERR(cb_info->task)) {
> > > + ret = PTR_ERR(cb_info->task);
> > > + cb_info->serv = NULL;
> > > + cb_info->task = NULL;
> > > svc_exit_thread(rqstp);
> > > goto out_err;
> > > }
> > > @@ -276,7 +276,7 @@ out:
> > > out_err:
> > > dprintk("Couldn't create callback socket or server
> > thread; err = %d\n",
> > > ret);
> > > - nfs_callback_info[minorversion].users--;
> > > + cb_info->users--;
> > > goto out;
> > > }
> > >
> > > @@ -285,12 +285,14 @@ out_err:
> > > */
> > > void nfs_callback_down(int minorversion)
> > > {
> > > + struct nfs_callback_data *cb_info =
> > &nfs_callback_info[minorversion];
> > > +
> > > lock_kernel();
> > > mutex_lock(&nfs_callback_mutex);
> > > - nfs_callback_info[minorversion].users--;
> > > - if (nfs_callback_info[minorversion].users == 0 &&
> > > - nfs_callback_info[minorversion].task != NULL)
> > > - kthread_stop(nfs_callback_info[minorversion].task);
> > > + cb_info->users--;
> > > + if (cb_info->users == 0 &&
> > > + cb_info->task != NULL)
> > > + kthread_stop(cb_info->task);
> > > mutex_unlock(&nfs_callback_mutex);
> > > unlock_kernel();
> > > }
> >
> >
> >
> _______________________________________________
> 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/20080625/d79976c8/attachment-0001.htm
More information about the pNFS
mailing list