[pnfs] [PATCH 2/3] nfs41: Allow NFSv4 and NFSv4.1callbackservices to coexist
Adamson, Andy
William.Adamson at netapp.com
Fri Jun 27 08:31:20 EDT 2008
cool! do multiple mounts succeed, or does the code now return "busy or already mounted" on the second mount?
-->Andy
-----Original Message-----
From: Labiaga, Ricardo
Sent: Fri 6/27/2008 12:37 AM
To: Halevy, Benny
Cc: Adamson, Andy; pnfs at linux-nfs.org
Subject: RE: [pnfs] [PATCH 2/3] nfs41: Allow NFSv4 and NFSv4.1callbackservices to coexist
Found the problem with the 10052 error doing two mounts on the same
mount point. We didn't have the rpc_ops initialized in the nfs_client.
Therefore we did not find the previous nfs_client from the previous
mount forcing us to create and keep a new nfs_server structure. Patch
coming next.
- ricardo
On Wed, 2008-06-25 at 16:07 -0400, Halevy, Benny wrote:
> 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();
> > }
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://linux-nfs.org/pipermail/pnfs/attachments/20080627/ec47d2a2/attachment-0001.htm
More information about the pNFS
mailing list