On Wed, Oct 30, 2013 at 04:43:03PM +0200, Benny Halevy wrote:
> On 2013-10-30 16:35, J. Bruce Fields wrote:
> > On Wed, Oct 30, 2013 at 04:02:47PM +0200, Benny Halevy wrote:
> >> On 2013-10-29 16:46, J. Bruce Fields wrote:
> >>> On Tue, Oct 29, 2013 at 11:39:04AM +0200, Benny Halevy wrote:
> >>>> Access to dp->dl_perclnt must be synchronized by the recall_lock
> >>>
> >>> Are you sure? recall_lock is for stuff that's needed in the delegation
> >>> break callback (nfsd_break_deleg_cb() and any of the subsequent callback
> >>> handling). I don't think that includes dl_perclnt.
> >>
> >> I was mislead by the fact that destroy_client and nfs4_state_shutdown_net
> >> care to acquire the recall_lock for traversing the clp->cl_delegations
> >> and nn->del_recall_lru lists, respectively.
> >>
> >> Now nfs4_state_shutdown_net, although its comment says it should be
> >> called with the state lock held (SHOULD or should, or should it be MUST? :)
> >> It doesn't seem like nfsd_shutdown_net acquires the state lock.
> >
> > Good point.
> >
> >> Therefore, we either need to grab the state lock in nfs4_state_shutdown_net
> >> which adds yet another problem to fix,
> >
> > That sounds simplest as the short-term fix.... What problem do you see?
>
> Nothing specific, just the general use of the global state lock...
>
> >
> > --b.
> >
> > commit 2c9407ae694dad8a1ad6394d9bf6077024b9fae7
> > Author: J. Bruce Fields <bfields@xxxxxxxxxx>
> > Date: Wed Oct 30 10:33:09 2013 -0400
> >
> > nfsd4: nfsd_shutdown_net needs state lock
> >
> > A comment claims the caller should take it, but that's not being done.
> > Note we don't want it around the cancel_delayed_work_sync since that may
> > wait on work which holds the client lock.
> >
> > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> Ack
Thanks, done, and pushed out to
git://linux-nfs.org/~bfields/linux.git for-3.13
--b.
>
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 21eb678..e03e8ef 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5124,7 +5124,6 @@ out_recovery:
> > return ret;
> > }
> >
> > -/* should be called with the state lock held */
> > void
> > nfs4_state_shutdown_net(struct net *net)
> > {
> > @@ -5135,6 +5134,7 @@ nfs4_state_shutdown_net(struct net *net)
> > cancel_delayed_work_sync(&nn->laundromat_work);
> > locks_end_grace(&nn->nfsd4_manager);
> >
> > + nfs4_lock_state();
> > INIT_LIST_HEAD(&reaplist);
> > spin_lock(&recall_lock);
> > list_for_each_safe(pos, next, &nn->del_recall_lru) {
> > @@ -5149,6 +5149,7 @@ nfs4_state_shutdown_net(struct net *net)
> >
> > nfsd4_client_tracking_exit(net);
> > nfs4_state_destroy_net(net);
> > + nfs4_unlock_state();
> > }
> >
> > void
> > --
> > 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
> >
--
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
