All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] delegation state fixes and cleanups for 3.13
@ 2013-10-29  9:37 Benny Halevy
  2013-10-29  9:39 ` [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation Benny Halevy
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:37 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: NFS list

commit c7342d3cf5d5677063cbc6341634ed8cea1b176f
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>

commit 87174785bcec37371b718757af67dde0f6d083e3
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>

commit 775a7353f9d1e60ebd9096b1a82bfb78fe7f3cc9
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>

commit fdf586869a6b026893f818cb538574a7b77f5604
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>

commit 635a93446fde8d8fb1f4bb825926750d9b6aea9d
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>

commit 39f26e5974f73b62144abde6793d41b07a32719a
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>

commit cd2b2cac09cbd45419ce7d0e354dd94537a4fbfb
Author: Benny Halevy <bhalevy@primarydata.com>
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 <bhalevy@primarydata.com>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29 14:46   ` J. Bruce Fields
  2013-10-29  9:39 ` [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client Benny Halevy
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

Access to dp->dl_perclnt must be synchronized by the recall_lock

Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
  2013-10-29  9:39 ` [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29 15:04   ` J. Bruce Fields
  2013-10-29  9:39 ` [PATCH 3/7] nfsd4: properly hash delegation in nfs4_setlease Benny Halevy
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

[use list_splice_init]
Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/7] nfsd4: properly hash delegation in nfs4_setlease
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
  2013-10-29  9:39 ` [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation Benny Halevy
  2013-10-29  9:39 ` [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29  9:39 ` [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation Benny Halevy
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

keep all accesses via dl_perclnt under the recall_lock.

Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
                   ` (2 preceding siblings ...)
  2013-10-29  9:39 ` [PATCH 3/7] nfsd4: properly hash delegation in nfs4_setlease Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29 15:54   ` J. Bruce Fields
  2013-10-29  9:39 ` [PATCH 5/7] nfsd4: cleanup hash and unhash delegation Benny Halevy
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

We don't want the stateid to be found in the hash table before the delegation
is granted.

Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/7] nfsd4: cleanup hash and unhash delegation
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
                   ` (3 preceding siblings ...)
  2013-10-29  9:39 ` [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29  9:39 ` [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation Benny Halevy
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

make sure all atomic ops are under the recall_lock

Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
                   ` (4 preceding siblings ...)
  2013-10-29  9:39 ` [PATCH 5/7] nfsd4: cleanup hash and unhash delegation Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29 15:59   ` J. Bruce Fields
  2013-10-29  9:39 ` [PATCH 7/7] nfsd4: hold recall_lock while accessing nfs4_delegation.dl_recall_lru Benny Halevy
  2013-10-29 16:06 ` [PATCH 0/7] delegation state fixes and cleanups for 3.13 J. Bruce Fields
  7 siblings, 1 reply; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

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 <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 7/7] nfsd4: hold recall_lock while accessing nfs4_delegation.dl_recall_lru
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
                   ` (5 preceding siblings ...)
  2013-10-29  9:39 ` [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation Benny Halevy
@ 2013-10-29  9:39 ` Benny Halevy
  2013-10-29 16:06 ` [PATCH 0/7] delegation state fixes and cleanups for 3.13 J. Bruce Fields
  7 siblings, 0 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-29  9:39 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Benny Halevy

Signed-off-by: Benny Halevy <bhalevy@primarydata.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation
  2013-10-29  9:39 ` [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation Benny Halevy
@ 2013-10-29 14:46   ` J. Bruce Fields
  2013-10-30 14:02     ` Benny Halevy
  0 siblings, 1 reply; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-29 14:46 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client
  2013-10-29  9:39 ` [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client Benny Halevy
@ 2013-10-29 15:04   ` J. Bruce Fields
  2013-10-29 16:02     ` J. Bruce Fields
  0 siblings, 1 reply; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-29 15:04 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation
  2013-10-29  9:39 ` [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation Benny Halevy
@ 2013-10-29 15:54   ` J. Bruce Fields
  2013-10-30 14:10     ` Benny Halevy
  0 siblings, 1 reply; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-29 15:54 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation
  2013-10-29  9:39 ` [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation Benny Halevy
@ 2013-10-29 15:59   ` J. Bruce Fields
  2013-10-30 14:16     ` Benny Halevy
  0 siblings, 1 reply; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-29 15:59 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client
  2013-10-29 15:04   ` J. Bruce Fields
@ 2013-10-29 16:02     ` J. Bruce Fields
  2013-10-30 14:07       ` Benny Halevy
  0 siblings, 1 reply; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-29 16:02 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
> > ---
> >  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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/7] delegation state fixes and cleanups for 3.13
  2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
                   ` (6 preceding siblings ...)
  2013-10-29  9:39 ` [PATCH 7/7] nfsd4: hold recall_lock while accessing nfs4_delegation.dl_recall_lru Benny Halevy
@ 2013-10-29 16:06 ` J. Bruce Fields
  7 siblings, 0 replies; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-29 16:06 UTC (permalink / raw)
  To: Benny Halevy; +Cc: J. Bruce Fields, NFS list

"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 <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> commit 87174785bcec37371b718757af67dde0f6d083e3
> Author: Benny Halevy <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> commit 775a7353f9d1e60ebd9096b1a82bfb78fe7f3cc9
> Author: Benny Halevy <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> commit fdf586869a6b026893f818cb538574a7b77f5604
> Author: Benny Halevy <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> commit 635a93446fde8d8fb1f4bb825926750d9b6aea9d
> Author: Benny Halevy <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> commit 39f26e5974f73b62144abde6793d41b07a32719a
> Author: Benny Halevy <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> commit cd2b2cac09cbd45419ce7d0e354dd94537a4fbfb
> Author: Benny Halevy <bhalevy@primarydata.com>
> 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 <bhalevy@primarydata.com>
> 
> --
> 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation
  2013-10-29 14:46   ` J. Bruce Fields
@ 2013-10-30 14:02     ` Benny Halevy
  2013-10-30 14:35       ` J. Bruce Fields
  0 siblings, 1 reply; 21+ messages in thread
From: Benny Halevy @ 2013-10-30 14:02 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
>> ---
>>  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 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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client
  2013-10-29 16:02     ` J. Bruce Fields
@ 2013-10-30 14:07       ` Benny Halevy
  0 siblings, 0 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-30 14:07 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
>>> ---
>>>  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 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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation
  2013-10-29 15:54   ` J. Bruce Fields
@ 2013-10-30 14:10     ` Benny Halevy
  0 siblings, 0 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-30 14:10 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
>> ---
>>  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 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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation
  2013-10-29 15:59   ` J. Bruce Fields
@ 2013-10-30 14:16     ` Benny Halevy
  0 siblings, 0 replies; 21+ messages in thread
From: Benny Halevy @ 2013-10-30 14:16 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: bfields, linux-nfs

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 <bhalevy@primarydata.com>
>> ---
>>  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 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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation
  2013-10-30 14:02     ` Benny Halevy
@ 2013-10-30 14:35       ` J. Bruce Fields
  2013-10-30 14:43         ` Benny Halevy
  0 siblings, 1 reply; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-30 14:35 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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 <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>

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation
  2013-10-30 14:35       ` J. Bruce Fields
@ 2013-10-30 14:43         ` Benny Halevy
  2013-10-30 22:19           ` J. Bruce Fields
  0 siblings, 1 reply; 21+ messages in thread
From: Benny Halevy @ 2013-10-30 14:43 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: bfields, linux-nfs

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
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation
  2013-10-30 14:43         ` Benny Halevy
@ 2013-10-30 22:19           ` J. Bruce Fields
  0 siblings, 0 replies; 21+ messages in thread
From: J. Bruce Fields @ 2013-10-30 22:19 UTC (permalink / raw)
  To: Benny Halevy; +Cc: bfields, linux-nfs

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
> > 

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2013-10-30 22:20 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-29  9:37 [PATCH 0/7] delegation state fixes and cleanups for 3.13 Benny Halevy
2013-10-29  9:39 ` [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation Benny Halevy
2013-10-29 14:46   ` J. Bruce Fields
2013-10-30 14:02     ` Benny Halevy
2013-10-30 14:35       ` J. Bruce Fields
2013-10-30 14:43         ` Benny Halevy
2013-10-30 22:19           ` J. Bruce Fields
2013-10-29  9:39 ` [PATCH 2/7] nfsd4: need to destroy revoked delegations in destroy_client Benny Halevy
2013-10-29 15:04   ` J. Bruce Fields
2013-10-29 16:02     ` J. Bruce Fields
2013-10-30 14:07       ` Benny Halevy
2013-10-29  9:39 ` [PATCH 3/7] nfsd4: properly hash delegation in nfs4_setlease Benny Halevy
2013-10-29  9:39 ` [PATCH 4/7] nfsd4: hash deleg stateid only on successful nfs4_set_delegation Benny Halevy
2013-10-29 15:54   ` J. Bruce Fields
2013-10-30 14:10     ` Benny Halevy
2013-10-29  9:39 ` [PATCH 5/7] nfsd4: cleanup hash and unhash delegation Benny Halevy
2013-10-29  9:39 ` [PATCH 6/7] nfsd4: move nfs4_put_file from unhash to put delegation Benny Halevy
2013-10-29 15:59   ` J. Bruce Fields
2013-10-30 14:16     ` Benny Halevy
2013-10-29  9:39 ` [PATCH 7/7] nfsd4: hold recall_lock while accessing nfs4_delegation.dl_recall_lru Benny Halevy
2013-10-29 16:06 ` [PATCH 0/7] delegation state fixes and cleanups for 3.13 J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.