On Thu, 28 Aug 2014 16:24:43 -0400
"J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
> On Thu, Aug 28, 2014 at 04:01:00PM -0400, J. Bruce Fields wrote:
> > On Tue, Aug 19, 2014 at 02:38:25PM -0400, Jeff Layton wrote:
> > > Currently, all of the grace period handling is part of lockd. Eventually
> > > though we'd like to be able to build v4-only servers, at which point
> > > we'll need to put all of this elsewhere.
> > >
> > > Move the code itself into fs/nfs_common and have it build a grace.ko
> > > module. Then, rejigger the Kconfig options so that both nfsd and lockd
> > > enable it automatically.
> >
> > Thanks, applying this one for 3.18 indepedently of the others.
>
> This code should also be fixed, though.
>
> Currently nfsd is recording the grace period as done when its own timer
> runs out, but then it continuing to accept reclaims until lockd is also
> done.
>
> This is an unusual bug to actually hit in part because lockd's grace
> period is by default less than nfsd's.
>
> Your code introduces new cases when nfsd's grace period could end before
> lockd's, but in all of them future reclaims would be denied (because the
> only clients active on last shutdown would be 4.1 clients that have all
> issued RECLAIM_COMPLETE, so reclaims are all going to be denied).
>
> So maybe it's not urgent.
>
> --b.
>
Yeah, I don't really see a bug here...
We'll only lift the nfsd grace period once all the v4.1 clients have
sent a RECLAIM_COMPLETE and if there aren't any v4.0 ones. At that
point, no one should be sending any reclaims but we'll still allow them
if the lockd grace period hasn't been lifted yet.
I suppose that we could deny them in principle, but there doesn't seem
to be any real harm in allowing it to happen. The simple "fix" for it
would be to have nfs4_check_open_reclaim do a test for
NFSD4_CLIENT_RECLAIM_COMPLETE, and reject the reclaim request if it has
been set.
It also occurs to me that we probably ought to have the nfsdcltrack
upcall set and test NFSD4_CLIENT_STABLE to reduce upcalls. The fact
that we don't do that there seems inefficient (though it shouldn't harm
correctness).
> >
> > --b.
> >
> > >
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> > > ---
> > > fs/Kconfig | 6 +++-
> > > fs/lockd/Makefile | 2 +-
> > > fs/lockd/netns.h | 1 -
> > > fs/lockd/svc.c | 1 -
> > > fs/nfs_common/Makefile | 3 +-
> > > fs/{lockd => nfs_common}/grace.c | 68 ++++++++++++++++++++++++++++++++++------
> > > fs/nfsd/Kconfig | 1 +
> > > include/linux/proc_fs.h | 2 ++
> > > 8 files changed, 69 insertions(+), 15 deletions(-)
> > > rename fs/{lockd => nfs_common}/grace.c (50%)
> > >
> > > diff --git a/fs/Kconfig b/fs/Kconfig
> > > index 312393f32948..db5dc1598716 100644
> > > --- a/fs/Kconfig
> > > +++ b/fs/Kconfig
> > > @@ -233,9 +233,13 @@ if NETWORK_FILESYSTEMS
> > > source "fs/nfs/Kconfig"
> > > source "fs/nfsd/Kconfig"
> > >
> > > +config GRACE_PERIOD
> > > + tristate
> > > +
> > > config LOCKD
> > > tristate
> > > depends on FILE_LOCKING
> > > + select GRACE_PERIOD
> > >
> > > config LOCKD_V4
> > > bool
> > > @@ -249,7 +253,7 @@ config NFS_ACL_SUPPORT
> > >
> > > config NFS_COMMON
> > > bool
> > > - depends on NFSD || NFS_FS
> > > + depends on NFSD || NFS_FS || LOCKD
> > > default y
> > >
> > > source "net/sunrpc/Kconfig"
> > > diff --git a/fs/lockd/Makefile b/fs/lockd/Makefile
> > > index ca58d64374ca..6a0b351ce30e 100644
> > > --- a/fs/lockd/Makefile
> > > +++ b/fs/lockd/Makefile
> > > @@ -5,6 +5,6 @@
> > > obj-$(CONFIG_LOCKD) += lockd.o
> > >
> > > lockd-objs-y := clntlock.o clntproc.o clntxdr.o host.o svc.o svclock.o \
> > > - svcshare.o svcproc.o svcsubs.o mon.o xdr.o grace.o
> > > + svcshare.o svcproc.o svcsubs.o mon.o xdr.o
> > > lockd-objs-$(CONFIG_LOCKD_V4) += clnt4xdr.o xdr4.o svc4proc.o
> > > lockd-objs := $(lockd-objs-y)
> > > diff --git a/fs/lockd/netns.h b/fs/lockd/netns.h
> > > index 5010b55628b4..097bfa3adb1c 100644
> > > --- a/fs/lockd/netns.h
> > > +++ b/fs/lockd/netns.h
> > > @@ -11,7 +11,6 @@ struct lockd_net {
> > >
> > > struct delayed_work grace_period_end;
> > > struct lock_manager lockd_manager;
> > > - struct list_head grace_list;
> > >
> > > spinlock_t nsm_clnt_lock;
> > > unsigned int nsm_users;
> > > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > > index 8f27c93f8d2e..3599c9ca28ae 100644
> > > --- a/fs/lockd/svc.c
> > > +++ b/fs/lockd/svc.c
> > > @@ -583,7 +583,6 @@ static int lockd_init_net(struct net *net)
> > > struct lockd_net *ln = net_generic(net, lockd_net_id);
> > >
> > > INIT_DELAYED_WORK(&ln->grace_period_end, grace_ender);
> > > - INIT_LIST_HEAD(&ln->grace_list);
> > > spin_lock_init(&ln->nsm_clnt_lock);
> > > return 0;
> > > }
> > > diff --git a/fs/nfs_common/Makefile b/fs/nfs_common/Makefile
> > > index f689ed82af3a..d153ca3ea577 100644
> > > --- a/fs/nfs_common/Makefile
> > > +++ b/fs/nfs_common/Makefile
> > > @@ -3,5 +3,6 @@
> > > #
> > >
> > > obj-$(CONFIG_NFS_ACL_SUPPORT) += nfs_acl.o
> > > -
> > > nfs_acl-objs := nfsacl.o
> > > +
> > > +obj-$(CONFIG_GRACE_PERIOD) += grace.o
> > > diff --git a/fs/lockd/grace.c b/fs/nfs_common/grace.c
> > > similarity index 50%
> > > rename from fs/lockd/grace.c
> > > rename to fs/nfs_common/grace.c
> > > index 6d1ee7204c88..ae6e58ea4de5 100644
> > > --- a/fs/lockd/grace.c
> > > +++ b/fs/nfs_common/grace.c
> > > @@ -1,17 +1,20 @@
> > > /*
> > > * Common code for control of lockd and nfsv4 grace periods.
> > > + *
> > > + * Transplanted from lockd code
> > > */
> > >
> > > #include <linux/module.h>
> > > -#include <linux/lockd/bind.h>
> > > #include <net/net_namespace.h>
> > > +#include <net/netns/generic.h>
> > > +#include <linux/fs.h>
> > >
> > > -#include "netns.h"
> > > -
> > > +static int grace_net_id;
> > > static DEFINE_SPINLOCK(grace_lock);
> > >
> > > /**
> > > * locks_start_grace
> > > + * @net: net namespace that this lock manager belongs to
> > > * @lm: who this grace period is for
> > > *
> > > * A grace period is a period during which locks should not be given
> > > @@ -21,18 +24,20 @@ static DEFINE_SPINLOCK(grace_lock);
> > > *
> > > * This function is called to start a grace period.
> > > */
> > > -void locks_start_grace(struct net *net, struct lock_manager *lm)
> > > +void
> > > +locks_start_grace(struct net *net, struct lock_manager *lm)
> > > {
> > > - struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > + struct list_head *grace_list = net_generic(net, grace_net_id);
> > >
> > > spin_lock(&grace_lock);
> > > - list_add(&lm->list, &ln->grace_list);
> > > + list_add(&lm->list, grace_list);
> > > spin_unlock(&grace_lock);
> > > }
> > > EXPORT_SYMBOL_GPL(locks_start_grace);
> > >
> > > /**
> > > * locks_end_grace
> > > + * @net: net namespace that this lock manager belongs to
> > > * @lm: who this grace period is for
> > > *
> > > * Call this function to state that the given lock manager is ready to
> > > @@ -41,7 +46,8 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
> > > * Note that callers count on it being safe to call this more than once,
> > > * and the second call should be a no-op.
> > > */
> > > -void locks_end_grace(struct lock_manager *lm)
> > > +void
> > > +locks_end_grace(struct lock_manager *lm)
> > > {
> > > spin_lock(&grace_lock);
> > > list_del_init(&lm->list);
> > > @@ -56,10 +62,52 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> > > * to answer ordinary lock requests, and when they should accept only
> > > * lock reclaims.
> > > */
> > > -int locks_in_grace(struct net *net)
> > > +int
> > > +locks_in_grace(struct net *net)
> > > {
> > > - struct lockd_net *ln = net_generic(net, lockd_net_id);
> > > + struct list_head *grace_list = net_generic(net, grace_net_id);
> > >
> > > - return !list_empty(&ln->grace_list);
> > > + return !list_empty(grace_list);
> > > }
> > > EXPORT_SYMBOL_GPL(locks_in_grace);
> > > +
> > > +static int __net_init
> > > +grace_init_net(struct net *net)
> > > +{
> > > + struct list_head *grace_list = net_generic(net, grace_net_id);
> > > +
> > > + INIT_LIST_HEAD(grace_list);
> > > + return 0;
> > > +}
> > > +
> > > +static void __net_exit
> > > +grace_exit_net(struct net *net)
> > > +{
> > > + struct list_head *grace_list = net_generic(net, grace_net_id);
> > > +
> > > + BUG_ON(!list_empty(grace_list));
> > > +}
> > > +
> > > +static struct pernet_operations grace_net_ops = {
> > > + .init = grace_init_net,
> > > + .exit = grace_exit_net,
> > > + .id = &grace_net_id,
> > > + .size = sizeof(struct list_head),
> > > +};
> > > +
> > > +static int __init
> > > +init_grace(void)
> > > +{
> > > + return register_pernet_subsys(&grace_net_ops);
> > > +}
> > > +
> > > +static void __exit
> > > +exit_grace(void)
> > > +{
> > > + unregister_pernet_subsys(&grace_net_ops);
> > > +}
> > > +
> > > +MODULE_AUTHOR("Jeff Layton <jlayton@xxxxxxxxxxxxxxx>");
> > > +MODULE_LICENSE("GPL");
> > > +module_init(init_grace)
> > > +module_exit(exit_grace)
> > > diff --git a/fs/nfsd/Kconfig b/fs/nfsd/Kconfig
> > > index f994e750e0d1..4fa98764de21 100644
> > > --- a/fs/nfsd/Kconfig
> > > +++ b/fs/nfsd/Kconfig
> > > @@ -71,6 +71,7 @@ config NFSD_V4
> > > select FS_POSIX_ACL
> > > select SUNRPC_GSS
> > > select CRYPTO
> > > + select GRACE_PERIOD
> > > help
> > > This option enables support in your system's NFS server for
> > > version 4 of the NFS protocol (RFC 3530).
> > > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > > index 9d117f61d976..b97bf2ef996e 100644
> > > --- a/include/linux/proc_fs.h
> > > +++ b/include/linux/proc_fs.h
> > > @@ -74,6 +74,8 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> > >
> > > #endif /* CONFIG_PROC_FS */
> > >
> > > +struct net;
> > > +
> > > static inline struct proc_dir_entry *proc_net_mkdir(
> > > struct net *net, const char *name, struct proc_dir_entry *parent)
> > > {
> > > --
> > > 1.9.3
> > >
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
