commit c7342d3cf5d5677063cbc6341634ed8cea1b176f
Author: Benny Halevy <[email protected]>
Date: Tue Oct 15 17:07:31 2013 +0300
nfsd4: fix recall_lock use in unhash_delegation
Access to dp->dl_perclnt must be synchronized by the recall_lock
Signed-off-by: Benny Halevy <[email protected]>
commit 87174785bcec37371b718757af67dde0f6d083e3
Author: Benny Halevy <[email protected]>
Date: Tue Oct 15 17:29:13 2013 +0300
nfsd4: need to destroy revoked delegations in destroy_client
[use list_splice_init]
Signed-off-by: Benny Halevy <[email protected]>
commit 775a7353f9d1e60ebd9096b1a82bfb78fe7f3cc9
Author: Benny Halevy <[email protected]>
Date: Tue Oct 15 17:41:53 2013 +0300
nfsd4: properly hash delegation in nfs4_setlease
keep all accesses via dl_perclnt under the recall_lock.
Signed-off-by: Benny Halevy <[email protected]>
commit fdf586869a6b026893f818cb538574a7b77f5604
Author: Benny Halevy <[email protected]>
Date: Tue Oct 15 21:08:48 2013 +0300
nfsd4: hash deleg stateid only on successful nfs4_set_delegation
We don't want the stateid to be found in the hash table before the delegation
is granted.
Signed-off-by: Benny Halevy <[email protected]>
commit 635a93446fde8d8fb1f4bb825926750d9b6aea9d
Author: Benny Halevy <[email protected]>
Date: Tue Oct 15 23:46:55 2013 +0300
nfsd4: cleanup hash and unhash delegation
make sure all atomic ops are under the recall_lock
Signed-off-by: Benny Halevy <[email protected]>
commit 39f26e5974f73b62144abde6793d41b07a32719a
Author: Benny Halevy <[email protected]>
Date: Tue Oct 15 23:50:30 2013 +0300
nfsd4: move nfs4_put_file from unhash to put delegation
revoked delegations are unhashed but are kept around in nfsv4.1 and we better
hang on to dp_file in this case.
Signed-off-by: Benny Halevy <[email protected]>
commit cd2b2cac09cbd45419ce7d0e354dd94537a4fbfb
Author: Benny Halevy <[email protected]>
Date: Wed Oct 16 00:17:14 2013 +0300
nfsd4: hold recall_lock while accessing nfs4_delegation.dl_recall_lru
Signed-off-by: Benny Halevy <[email protected]>
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 <[email protected]>
> > 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 <[email protected]>
>
> 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
make sure all atomic ops are under the recall_lock
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 64fcdd6..8840206 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -436,6 +436,7 @@ static void unhash_stid(struct nfs4_stid *s)
{
list_add(&dp->dl_perfile, &fp->fi_delegations);
list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
+ dp->dl_stid.sc_type = NFS4_DELEG_STID;
}
/* Called under the state lock. */
@@ -446,8 +447,9 @@ static void unhash_stid(struct nfs4_stid *s)
list_del_init(&dp->dl_perclnt);
list_del_init(&dp->dl_perfile);
list_del_init(&dp->dl_recall_lru);
- spin_unlock(&recall_lock);
+ dp->dl_stid.sc_type = 0;
nfs4_put_deleg_lease(dp->dl_file);
+ spin_unlock(&recall_lock);
put_nfs4_file(dp->dl_file);
dp->dl_file = NULL;
}
@@ -3078,7 +3080,6 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
atomic_inc(&fp->fi_delegees);
hash_delegation_locked(dp, fp);
spin_unlock(&recall_lock);
- dp->dl_stid.sc_type = NFS4_DELEG_STID;
return 0;
out_free:
put_nfs4_file(fp);
--
1.8.3.1
Access to dp->dl_perclnt must be synchronized by the recall_lock
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a90949a..a403502 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -436,8 +436,8 @@ static void unhash_stid(struct nfs4_stid *s)
static void
unhash_delegation(struct nfs4_delegation *dp)
{
- list_del_init(&dp->dl_perclnt);
spin_lock(&recall_lock);
+ list_del_init(&dp->dl_perclnt);
list_del_init(&dp->dl_perfile);
list_del_init(&dp->dl_recall_lru);
spin_unlock(&recall_lock);
--
1.8.3.1
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7f78ff5..d9161ea 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -441,24 +441,29 @@ static void unhash_stid(struct nfs4_stid *s)
dp->dl_stid.sc_type = NFS4_DELEG_STID;
}
-/* Called under the state lock. */
static void
-unhash_delegation(struct nfs4_delegation *dp)
+unhash_delegation_locked(struct nfs4_delegation *dp)
{
- spin_lock(&recall_lock);
list_del_init(&dp->dl_perclnt);
list_del_init(&dp->dl_perfile);
list_del_init(&dp->dl_recall_lru);
dp->dl_stid.sc_type = 0;
nfs4_put_deleg_lease(dp->dl_file);
- spin_unlock(&recall_lock);
}
-
+static void
+unhash_delegation(struct nfs4_delegation *dp)
+{
+ spin_lock(&recall_lock);
+ unhash_delegation_locked(dp);
+ spin_unlock(&recall_lock);
+}
static void destroy_revoked_delegation(struct nfs4_delegation *dp)
{
+ spin_lock(&recall_lock);
list_del_init(&dp->dl_recall_lru);
+ spin_unlock(&recall_lock);
nfs4_put_delegation(dp);
}
@@ -475,9 +480,11 @@ static void revoke_delegation(struct nfs4_delegation *dp)
if (clp->cl_minorversion == 0)
destroy_delegation(dp);
else {
- unhash_delegation(dp);
+ spin_lock(&recall_lock);
+ unhash_delegation_locked(dp);
dp->dl_stid.sc_type = NFS4_REVOKED_DELEG_STID;
list_add(&dp->dl_recall_lru, &clp->cl_revoked);
+ spin_unlock(&recall_lock);
}
}
@@ -2729,6 +2736,7 @@ static void nfsd_break_one_deleg(struct nfs4_delegation *dp)
struct nfs4_client *clp = dp->dl_stid.sc_client;
struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
+ lockdep_assert_held(&recall_lock);
/* We're assuming the state code never drops its reference
* without first removing the lease. Since we're in this lease
* callback (and since the lease code is serialized by the kernel
--
1.8.3.1
Whoops, yes, looks like a good fix.
I'm not convinced of the need for the recall_lock here for reasons given
before. Could you just drop the recall_lock here and resend?
--b.
On Tue, Oct 29, 2013 at 11:39:12AM +0200, Benny Halevy wrote:
> [use list_splice_init]
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a403502..a66b0ad 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1129,6 +1129,13 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
> dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
> destroy_delegation(dp);
> }
> + spin_lock(&recall_lock);
> + list_splice_init(&clp->cl_revoked, &reaplist);
> + spin_unlock(&recall_lock);
> + while (!list_empty(&reaplist)) {
> + dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
> + destroy_revoked_delegation(dp);
> + }
> while (!list_empty(&clp->cl_openowners)) {
> oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
> release_openowner(oo);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[use list_splice_init]
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a403502..a66b0ad 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1129,6 +1129,13 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
destroy_delegation(dp);
}
+ spin_lock(&recall_lock);
+ list_splice_init(&clp->cl_revoked, &reaplist);
+ spin_unlock(&recall_lock);
+ while (!list_empty(&reaplist)) {
+ dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
+ destroy_revoked_delegation(dp);
+ }
while (!list_empty(&clp->cl_openowners)) {
oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
release_openowner(oo);
--
1.8.3.1
On 2013-10-29 18:02, J. Bruce Fields wrote:
> On Tue, Oct 29, 2013 at 11:04:14AM -0400, bfields wrote:
>> Whoops, yes, looks like a good fix.
>>
>> I'm not convinced of the need for the recall_lock here for reasons given
>> before. Could you just drop the recall_lock here and resend?
>
> Actually never mind I've done that on my local tree. I'll wait to push
> it out till you've had a chance to object.
no objection. thanks.
>
> --b.
>
>>
>> --b.
>>
>> On Tue, Oct 29, 2013 at 11:39:12AM +0200, Benny Halevy wrote:
>>> [use list_splice_init]
>>> Signed-off-by: Benny Halevy <[email protected]>
>>> ---
>>> fs/nfsd/nfs4state.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index a403502..a66b0ad 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1129,6 +1129,13 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
>>> dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
>>> destroy_delegation(dp);
>>> }
>>> + spin_lock(&recall_lock);
>>> + list_splice_init(&clp->cl_revoked, &reaplist);
>>> + spin_unlock(&recall_lock);
>>> + while (!list_empty(&reaplist)) {
>>> + dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
>>> + destroy_revoked_delegation(dp);
>>> + }
>>> while (!list_empty(&clp->cl_openowners)) {
>>> oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
>>> release_openowner(oo);
>>> --
>>> 1.8.3.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
We don't want the stateid to be found in the hash table before the delegation
is granted.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 405649f..64fcdd6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -375,7 +375,6 @@ static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
if (dp == NULL)
return dp;
- dp->dl_stid.sc_type = NFS4_DELEG_STID;
/*
* delegation seqid's are never incremented. The 4.1 special
* meaning of seqid 0 isn't meaningful, really, but let's avoid
@@ -3079,6 +3078,7 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
atomic_inc(&fp->fi_delegees);
hash_delegation_locked(dp, fp);
spin_unlock(&recall_lock);
+ dp->dl_stid.sc_type = NFS4_DELEG_STID;
return 0;
out_free:
put_nfs4_file(fp);
--
1.8.3.1
keep all accesses via dl_perclnt under the recall_lock.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a66b0ad..405649f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -432,6 +432,13 @@ static void unhash_stid(struct nfs4_stid *s)
s->sc_type = 0;
}
+static void
+hash_delegation_locked(struct nfs4_delegation *dp, struct nfs4_file *fp)
+{
+ list_add(&dp->dl_perfile, &fp->fi_delegations);
+ list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
+}
+
/* Called under the state lock. */
static void
unhash_delegation(struct nfs4_delegation *dp)
@@ -3035,17 +3042,17 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
if (!fl)
return -ENOMEM;
fl->fl_file = find_readable_file(fp);
- list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
if (status) {
- list_del_init(&dp->dl_perclnt);
locks_free_lock(fl);
return status;
}
fp->fi_lease = fl;
fp->fi_deleg_file = get_file(fl->fl_file);
atomic_set(&fp->fi_delegees, 1);
- list_add(&dp->dl_perfile, &fp->fi_delegations);
+ spin_lock(&recall_lock);
+ hash_delegation_locked(dp, fp);
+ spin_unlock(&recall_lock);
return 0;
}
@@ -3070,9 +3077,8 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
goto out_free;
}
atomic_inc(&fp->fi_delegees);
- list_add(&dp->dl_perfile, &fp->fi_delegations);
+ hash_delegation_locked(dp, fp);
spin_unlock(&recall_lock);
- list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
return 0;
out_free:
put_nfs4_file(fp);
@@ -4871,6 +4877,7 @@ static u64 nfsd_find_all_delegations(struct nfs4_client *clp, u64 max,
struct nfs4_delegation *dp, *next;
u64 count = 0;
+ lockdep_assert_held(&recall_lock);
list_for_each_entry_safe(dp, next, &clp->cl_delegations, dl_perclnt) {
if (victims)
list_move(&dp->dl_recall_lru, victims);
--
1.8.3.1
"nfsd4: need to destroy revoked delegations in destroy_client" is the
only one that looks to me like it fixes a current bug.
I understand the others may be laying the groundwork for a state lock
change, but I don't have the context to review those yet....
--b.
On Tue, Oct 29, 2013 at 11:37:34AM +0200, Benny Halevy wrote:
> commit c7342d3cf5d5677063cbc6341634ed8cea1b176f
> Author: Benny Halevy <[email protected]>
> Date: Tue Oct 15 17:07:31 2013 +0300
>
> nfsd4: fix recall_lock use in unhash_delegation
>
> Access to dp->dl_perclnt must be synchronized by the recall_lock
>
> Signed-off-by: Benny Halevy <[email protected]>
>
> commit 87174785bcec37371b718757af67dde0f6d083e3
> Author: Benny Halevy <[email protected]>
> Date: Tue Oct 15 17:29:13 2013 +0300
>
> nfsd4: need to destroy revoked delegations in destroy_client
>
> [use list_splice_init]
> Signed-off-by: Benny Halevy <[email protected]>
>
> commit 775a7353f9d1e60ebd9096b1a82bfb78fe7f3cc9
> Author: Benny Halevy <[email protected]>
> Date: Tue Oct 15 17:41:53 2013 +0300
>
> nfsd4: properly hash delegation in nfs4_setlease
>
> keep all accesses via dl_perclnt under the recall_lock.
>
> Signed-off-by: Benny Halevy <[email protected]>
>
> commit fdf586869a6b026893f818cb538574a7b77f5604
> Author: Benny Halevy <[email protected]>
> Date: Tue Oct 15 21:08:48 2013 +0300
>
> nfsd4: hash deleg stateid only on successful nfs4_set_delegation
>
> We don't want the stateid to be found in the hash table before the delegation
> is granted.
>
> Signed-off-by: Benny Halevy <[email protected]>
>
> commit 635a93446fde8d8fb1f4bb825926750d9b6aea9d
> Author: Benny Halevy <[email protected]>
> Date: Tue Oct 15 23:46:55 2013 +0300
>
> nfsd4: cleanup hash and unhash delegation
>
> make sure all atomic ops are under the recall_lock
>
> Signed-off-by: Benny Halevy <[email protected]>
>
> commit 39f26e5974f73b62144abde6793d41b07a32719a
> Author: Benny Halevy <[email protected]>
> Date: Tue Oct 15 23:50:30 2013 +0300
>
> nfsd4: move nfs4_put_file from unhash to put delegation
>
> revoked delegations are unhashed but are kept around in nfsv4.1 and we better
> hang on to dp_file in this case.
>
> Signed-off-by: Benny Halevy <[email protected]>
>
> commit cd2b2cac09cbd45419ce7d0e354dd94537a4fbfb
> Author: Benny Halevy <[email protected]>
> Date: Wed Oct 16 00:17:14 2013 +0300
>
> nfsd4: hold recall_lock while accessing nfs4_delegation.dl_recall_lru
>
> Signed-off-by: Benny Halevy <[email protected]>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 29, 2013 at 11:39:32AM +0200, Benny Halevy wrote:
> revoked delegations are unhashed but are kept around in nfsv4.1 and we better
> hang on to dp_file in this case.
I don't agree: we shouldn't need dl_file any more once the delegation is
revoked. It exists only to process delegreturns and set
SEQ4_STATUS_RECALLABLE_STATE_REVOKED.
If some other code (like delegreturn) *is* attempting to use dl_file on
revoked delegation, then that sounds like a delegreturn bug.
Among other things I think we'd like to be able to free up the reference
and deallocate the file on disk if necessary once the delegation has
been revoked.
--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 8840206..7f78ff5 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -411,6 +411,8 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
> {
> remove_stid(&dp->dl_stid);
> if (atomic_dec_and_test(&dp->dl_count)) {
> + if (dp->dl_file)
> + put_nfs4_file(dp->dl_file);
> nfs4_free_stid(deleg_slab, &dp->dl_stid);
> num_delegations--;
> }
> @@ -450,8 +452,6 @@ static void unhash_stid(struct nfs4_stid *s)
> dp->dl_stid.sc_type = 0;
> nfs4_put_deleg_lease(dp->dl_file);
> spin_unlock(&recall_lock);
> - put_nfs4_file(dp->dl_file);
> - dp->dl_file = NULL;
> }
>
>
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a90949a..a403502 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -436,8 +436,8 @@ static void unhash_stid(struct nfs4_stid *s)
> static void
> unhash_delegation(struct nfs4_delegation *dp)
> {
> - list_del_init(&dp->dl_perclnt);
> spin_lock(&recall_lock);
> + list_del_init(&dp->dl_perclnt);
> list_del_init(&dp->dl_perfile);
> list_del_init(&dp->dl_recall_lru);
> spin_unlock(&recall_lock);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
--b.
commit 2c9407ae694dad8a1ad6394d9bf6077024b9fae7
Author: J. Bruce Fields <[email protected]>
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 <[email protected]>
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
On Tue, Oct 29, 2013 at 11:04:14AM -0400, bfields wrote:
> Whoops, yes, looks like a good fix.
>
> I'm not convinced of the need for the recall_lock here for reasons given
> before. Could you just drop the recall_lock here and resend?
Actually never mind I've done that on my local tree. I'll wait to push
it out till you've had a chance to object.
--b.
>
> --b.
>
> On Tue, Oct 29, 2013 at 11:39:12AM +0200, Benny Halevy wrote:
> > [use list_splice_init]
> > Signed-off-by: Benny Halevy <[email protected]>
> > ---
> > fs/nfsd/nfs4state.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index a403502..a66b0ad 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1129,6 +1129,13 @@ static struct nfs4_client *alloc_client(struct xdr_netobj name)
> > dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
> > destroy_delegation(dp);
> > }
> > + spin_lock(&recall_lock);
> > + list_splice_init(&clp->cl_revoked, &reaplist);
> > + spin_unlock(&recall_lock);
> > + while (!list_empty(&reaplist)) {
> > + dp = list_entry(reaplist.next, struct nfs4_delegation, dl_recall_lru);
> > + destroy_revoked_delegation(dp);
> > + }
> > while (!list_empty(&clp->cl_openowners)) {
> > oo = list_entry(clp->cl_openowners.next, struct nfs4_openowner, oo_perclient);
> > release_openowner(oo);
> > --
> > 1.8.3.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
revoked delegations are unhashed but are kept around in nfsv4.1 and we better
hang on to dp_file in this case.
Signed-off-by: Benny Halevy <[email protected]>
---
fs/nfsd/nfs4state.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8840206..7f78ff5 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -411,6 +411,8 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
{
remove_stid(&dp->dl_stid);
if (atomic_dec_and_test(&dp->dl_count)) {
+ if (dp->dl_file)
+ put_nfs4_file(dp->dl_file);
nfs4_free_stid(deleg_slab, &dp->dl_stid);
num_delegations--;
}
@@ -450,8 +452,6 @@ static void unhash_stid(struct nfs4_stid *s)
dp->dl_stid.sc_type = 0;
nfs4_put_deleg_lease(dp->dl_file);
spin_unlock(&recall_lock);
- put_nfs4_file(dp->dl_file);
- dp->dl_file = NULL;
}
--
1.8.3.1
On 2013-10-29 17:54, J. Bruce Fields wrote:
> On Tue, Oct 29, 2013 at 11:39:21AM +0200, Benny Halevy wrote:
>> We don't want the stateid to be found in the hash table before the delegation
>> is granted.
>
> With all this code and any find_stateid calls under the state lock, this
> isn't currently an issue, right?
Right.
>
> The changelog needs to make clear whether this is fixing a current bug
> or preparing to allow dropping of the state lock somewhere. If the
> latter, it needs to outline what the expected change is that we're
> preparing for.
Agreed. We can delay this patch to the state lock elimination series.
Benny
>
> --b.
>
>>
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 405649f..64fcdd6 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -375,7 +375,6 @@ static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
>> dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
>> if (dp == NULL)
>> return dp;
>> - dp->dl_stid.sc_type = NFS4_DELEG_STID;
>> /*
>> * delegation seqid's are never incremented. The 4.1 special
>> * meaning of seqid 0 isn't meaningful, really, but let's avoid
>> @@ -3079,6 +3078,7 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
>> atomic_inc(&fp->fi_delegees);
>> hash_delegation_locked(dp, fp);
>> spin_unlock(&recall_lock);
>> + dp->dl_stid.sc_type = NFS4_DELEG_STID;
>> return 0;
>> out_free:
>> put_nfs4_file(fp);
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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.
Therefore, we either need to grab the state lock in nfs4_state_shutdown_net
which adds yet another problem to fix, or access dl_perclnt always under the
spin lock.
Benny
>
> --b.
>
>>
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index a90949a..a403502 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -436,8 +436,8 @@ static void unhash_stid(struct nfs4_stid *s)
>> static void
>> unhash_delegation(struct nfs4_delegation *dp)
>> {
>> - list_del_init(&dp->dl_perclnt);
>> spin_lock(&recall_lock);
>> + list_del_init(&dp->dl_perclnt);
>> list_del_init(&dp->dl_perfile);
>> list_del_init(&dp->dl_recall_lru);
>> spin_unlock(&recall_lock);
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]>
> 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 <[email protected]>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, Oct 29, 2013 at 11:39:21AM +0200, Benny Halevy wrote:
> We don't want the stateid to be found in the hash table before the delegation
> is granted.
With all this code and any find_stateid calls under the state lock, this
isn't currently an issue, right?
The changelog needs to make clear whether this is fixing a current bug
or preparing to allow dropping of the state lock somewhere. If the
latter, it needs to outline what the expected change is that we're
preparing for.
--b.
>
> Signed-off-by: Benny Halevy <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 405649f..64fcdd6 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -375,7 +375,6 @@ static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
> dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
> if (dp == NULL)
> return dp;
> - dp->dl_stid.sc_type = NFS4_DELEG_STID;
> /*
> * delegation seqid's are never incremented. The 4.1 special
> * meaning of seqid 0 isn't meaningful, really, but let's avoid
> @@ -3079,6 +3078,7 @@ static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
> atomic_inc(&fp->fi_delegees);
> hash_delegation_locked(dp, fp);
> spin_unlock(&recall_lock);
> + dp->dl_stid.sc_type = NFS4_DELEG_STID;
> return 0;
> out_free:
> put_nfs4_file(fp);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-10-29 17:59, J. Bruce Fields wrote:
> On Tue, Oct 29, 2013 at 11:39:32AM +0200, Benny Halevy wrote:
>> revoked delegations are unhashed but are kept around in nfsv4.1 and we better
>> hang on to dp_file in this case.
>
> I don't agree: we shouldn't need dl_file any more once the delegation is
> revoked. It exists only to process delegreturns and set
> SEQ4_STATUS_RECALLABLE_STATE_REVOKED.
OK, makes sense.
>
> If some other code (like delegreturn) *is* attempting to use dl_file on
> revoked delegation, then that sounds like a delegreturn bug.
It doesn't seem so.
>
> Among other things I think we'd like to be able to free up the reference
> and deallocate the file on disk if necessary once the delegation has
> been revoked.
OK, I'll deal with blocking case in my patchset, but putting the file will
need to beseparated from unhashing as the latter will be done under a spin lock
and put_nfs4_file -> iput may block.
Benny
>
> --b.
>
>>
>> Signed-off-by: Benny Halevy <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 8840206..7f78ff5 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -411,6 +411,8 @@ static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
>> {
>> remove_stid(&dp->dl_stid);
>> if (atomic_dec_and_test(&dp->dl_count)) {
>> + if (dp->dl_file)
>> + put_nfs4_file(dp->dl_file);
>> nfs4_free_stid(deleg_slab, &dp->dl_stid);
>> num_delegations--;
>> }
>> @@ -450,8 +452,6 @@ static void unhash_stid(struct nfs4_stid *s)
>> dp->dl_stid.sc_type = 0;
>> nfs4_put_deleg_lease(dp->dl_file);
>> spin_unlock(&recall_lock);
>> - put_nfs4_file(dp->dl_file);
>> - dp->dl_file = NULL;
>> }
>>
>>
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>