linux-nfs.vger.kernel.org archive mirror
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).