| Message ID | 20131030143552.GE3227@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@redhat.com> > 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@redhat.com> Ack > > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> > > 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@redhat.com> > > 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@vger.kernel.org > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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