[05/40] nfsd: Ensure atomicity of stateid destruction and idr tree removal

Commit Message

Jeff Layton July 21, 2014, 3:02 p.m. UTC

Preparation for removal of the client_mutex. Ensure that they are done
while holding the clp->cl_lock.

Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfsd/nfs4state.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

Comments

Christoph Hellwig July 27, 2014, 1:28 p.m. UTC | #1

On Mon, Jul 21, 2014 at 11:02:17AM -0400, Jeff Layton wrote:
> Preparation for removal of the client_mutex. Ensure that they are done
> while holding the clp->cl_lock.
> 
> Signed-off-by: Trond Myklebust <[e-mail]>

Looks good, although the orderig vs the earlier patches looks odd
to me.  I'd introduce the nfs4_put_stid before the locking for the
stateid idr, which would then naturally fall into place with the
right atomicity.

Independent of that: is there any point in keeping the separate
remove_stid_locked helper instead of inlining it into the caller?

Reviewed-by: Christoph Hellwig <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 076de12c3fa6..0243a19dbef4 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -604,13 +604,11 @@  out_dec:
 	return NULL;
 }
 
-static void remove_stid(struct nfs4_stid *s)
+static void remove_stid_locked(struct nfs4_client *clp, struct nfs4_stid *s)
 {
-	struct nfs4_client *clp = s->sc_client;
+	lockdep_assert_held(&clp->cl_lock);
 
-	spin_lock(&clp->cl_lock);
 	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
-	spin_unlock(&clp->cl_lock);
 }
 
 static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
@@ -620,14 +618,25 @@  static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
 	kmem_cache_free(slab, s);
 }
 
+static bool nfs4_put_stid(struct kmem_cache *slab, struct nfs4_stid *s)
+{
+	struct nfs4_client *clp = s->sc_client;
+
+	might_lock(&clp->cl_lock);
+
+	if (!atomic_dec_and_lock(&s->sc_count, &clp->cl_lock))
+		return false;
+	remove_stid_locked(clp, s);
+	spin_unlock(&clp->cl_lock);
+	nfs4_free_stid(slab, s);
+	return true;
+}
+
 void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
-	if (atomic_dec_and_test(&dp->dl_stid.sc_count)) {
-		remove_stid(&dp->dl_stid);
-		nfs4_free_stid(deleg_slab, &dp->dl_stid);
+	if (nfs4_put_stid(deleg_slab, &dp->dl_stid))
 		atomic_long_dec(&num_delegations);
-	}
 }
 
 static void nfs4_put_deleg_lease(struct nfs4_file *fp)
@@ -885,10 +894,7 @@  static void close_generic_stateid(struct nfs4_ol_stateid *stp)
 
 static void put_generic_stateid(struct nfs4_ol_stateid *stp)
 {
-	if (!atomic_dec_and_test(&stp->st_stid.sc_count))
-		return;
-	remove_stid(&stp->st_stid);
-	nfs4_free_stid(stateid_slab, &stp->st_stid);
+	nfs4_put_stid(stateid_slab, &stp->st_stid);
 }
 
 static void __release_lock_stateid(struct nfs4_ol_stateid *stp)