On Mon, Sep 15, 2014 at 07:19:19PM -0400, Jeff Layton wrote:
> On Mon, 15 Sep 2014 18:09:39 -0400
> "J. Bruce Fields" <bfields@xxxxxxxxxxxx> wrote:
>
> > On Mon, Sep 15, 2014 at 06:08:13PM -0400, J. Bruce Fields wrote:
> > > On Thu, Aug 28, 2014 at 04:24:43PM -0400, J. Bruce Fields 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.
> > >
> > > I sat down to fix this today then decided there's not a real bug:
> > >
> > > All the grace_done upcall really does is throw away any previous clients
> > > that haven't reclaimed yet.
> > >
> > > It's legal to do that as soon as the correct amount of time has passed.
> > > It's actually OK to continue to allow the grace period to run past that
> > > point, the only requirement is that all reclaims precede all new opens,
> > > which the code still does enforce.
> > >
> > > So I think it might just be worth a little extra explanation.
> >
> > I considered doing something like the following anyway (total
> > off-the-cuff draft, not tested), but I think it's overkill.
> >
> > --b.
> >
>
> Yeah, I tend to agree. I don't see a real need for it.
>
> In principle, I guess you could end up in a situation where the lockd
> and nfsd grace periods different by a large amount. In that case, you
> could end up in a situation where you started denying reclaims for v4.x
> clients that haven't reclaimed anything yet, but then still didn't
> allow them to establish new locks either.
Oh, right, I forget that this also keeps old clients from doing further
reclaims during this boot.
I think that could actually be fixed purely in userspace--instead of
purging those old clients, just record the new end-of-grace timestamp
and keep them around a while longer. You still know which ones to deny
on the next boot, but they're still around if you want to keep allowing
them to reclaim.
Anyway:
> So it might be reasonable to add something like this, but I don't see
> it as anything most setups would ever hit -- particularly not in sane
> configurations.
Yeah, no big deal.
--b.
>
>
> > commit 675121da48fb4d7d85b58467c7f79dd3a6964879
> > Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> > Date: Mon Sep 15 17:42:14 2014 -0400
> >
> > nfsd4: end grace only when last grace period ends
> >
> > In the case both lockd and nfsd4 are running, or when we have multiple
> > containers exporting the same filesystem, there may be multiple grace
> > periods. In that case the grace period doesn't really end till the last
> > one does and though it's harmless to do the grace_done upcall before
> > then, it would probably be better not to.
> >
> > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
> >
> > diff --git a/fs/nfs_common/grace.c b/fs/nfs_common/grace.c
> > index ae6e58e..dc67e5d 100644
> > --- a/fs/nfs_common/grace.c
> > +++ b/fs/nfs_common/grace.c
> > @@ -12,6 +12,11 @@
> > static int grace_net_id;
> > static DEFINE_SPINLOCK(grace_lock);
> >
> > +struct grace_net {
> > + struct list_head *grace_list;
> > + bool in_grace;
> > +};
> > +
> > /**
> > * locks_start_grace
> > * @net: net namespace that this lock manager belongs to
> > @@ -27,10 +32,10 @@ static DEFINE_SPINLOCK(grace_lock);
> > void
> > locks_start_grace(struct net *net, struct lock_manager *lm)
> > {
> > - struct list_head *grace_list = net_generic(net, grace_net_id);
> > + struct list_head *gn = net_generic(net, grace_net_id);
> >
> > spin_lock(&grace_lock);
> > - list_add(&lm->list, grace_list);
> > + list_add(&lm->list, &gn->grace_list);
> > spin_unlock(&grace_lock);
> > }
> > EXPORT_SYMBOL_GPL(locks_start_grace);
> > @@ -49,9 +54,31 @@ EXPORT_SYMBOL_GPL(locks_start_grace);
> > void
> > locks_end_grace(struct lock_manager *lm)
> > {
> > + struct list_head *gn = net_generic(net, grace_net_id);
> > +
> > + if (list_empty(&lm->list))
> > + return;
> > spin_lock(&grace_lock);
> > list_del_init(&lm->list);
> > spin_unlock(&grace_lock);
> > + if (list_empty(&gn->grace_list))
> > + return;
> > + /*
> > + * If the server goes down again right now, an NFSv4
> > + * client will still be reclaim after it comes back up, even if
> > + * it hasn't yet had a chance to reclaim state this time.
> > + */
> > + lm->finish_grace(lm);
> > + /*
> > + * At this point, NFSv4 clients can still reclaim. But if the
> > + * server crashes, any that have not yet reclaimed will be out
> > + * of luck on the next boot.
> > + */
> > + ln->in_grace = false;
>
> gn->in_grace = false;
>
> > + /*
> > + * At this point, no reclaims are permitted. Regular locking
> > + * can resume.
> > + */
> > }
> > EXPORT_SYMBOL_GPL(locks_end_grace);
> >
> > @@ -65,27 +92,28 @@ EXPORT_SYMBOL_GPL(locks_end_grace);
> > int
> > locks_in_grace(struct net *net)
> > {
> > - struct list_head *grace_list = net_generic(net, grace_net_id);
> > + struct list_head *gn = net_generic(net, grace_net_id);
> >
> > - return !list_empty(grace_list);
> > + return gn->in_grace;
> > }
> > 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);
> > + struct list_head *gn = net_generic(net, grace_net_id);
> >
> > - INIT_LIST_HEAD(grace_list);
> > + INIT_LIST_HEAD(&gn->grace_list);
> > + gn->in_grace = true;
> > return 0;
> > }
> >
> > static void __net_exit
> > grace_exit_net(struct net *net)
> > {
> > - struct list_head *grace_list = net_generic(net, grace_net_id);
> > + struct list_head *gn = net_generic(net, grace_net_id);
> >
> > - BUG_ON(!list_empty(grace_list));
> > + BUG_ON(!list_empty(&gn->grace_list));
> > }
> >
> > static struct pernet_operations grace_net_ops = {
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fc88b57..968acd0 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4122,7 +4122,6 @@ nfsd4_end_grace(struct nfsd_net *nn)
> >
> > dprintk("NFSD: end of grace period\n");
> > nn->grace_ended = true;
> > - nfsd4_record_grace_done(nn);
> > locks_end_grace(&nn->nfsd4_manager);
> > /*
> > * Now that every NFSv4 client has had the chance to recover and
> > @@ -6341,6 +6340,13 @@ nfs4_state_destroy_net(struct net *net)
> > put_net(net);
> > }
> >
> > +void nfsd4_finish_grace(struct lock_manager *lm)
> > +{
> > + struct nfsd_net *nn = lm->private;
> > +
> > + nfsd4_record_grace_done(nn);
> > +}
> > +
> > int
> > nfs4_state_start_net(struct net *net)
> > {
> > @@ -6352,6 +6358,8 @@ nfs4_state_start_net(struct net *net)
> > return ret;
> > nn->boot_time = get_seconds();
> > nn->grace_ended = false;
> > + nn->nfsd4_manager.finish_grace = nfsd4_finish_grace;
> > + nn->nfsd4_manager.private = nn;
> > locks_start_grace(net, &nn->nfsd4_manager);
> > nfsd4_client_tracking_init(net);
> > printk(KERN_INFO "NFSD: starting %ld-second grace period (net %p)\n",
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 9418772..0e33e7c 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -876,6 +876,8 @@ struct lock_manager_operations {
> >
> > struct lock_manager {
> > struct list_head list;
> > + void (*finish_grace)(struct lock_manager *lm);
> > + void *private;
> > };
> >
> > struct net;
> > --
> > 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
>
>
> --
> 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
