[06/40] nfsd: Cleanup the freeing of stateids

Commit Message

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

Add a ->free() callback to the struct nfs4_stid, so that we can
release a reference to the stid without caring about the contents.

Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfsd/nfs4state.c | 62 +++++++++++++++++++++++++++++++++++------------------
 fs/nfsd/state.h     |  2 ++
 2 files changed, 43 insertions(+), 21 deletions(-)

Comments

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

On Mon, Jul 21, 2014 at 11:02:18AM -0400, Jeff Layton wrote:
> Add a ->free() callback to the struct nfs4_stid, so that we can
> release a reference to the stid without caring about the contents.

Looks good, but I'd do it much earlier so that the nfs4_put_stid doesn't
have to change around again - IMHO adding the free callback should go
together with introducing the refcounting, as it's kinda required
to do the refcounting sanely.

Also when Trond first posted this I suggested to introduce an ops vector
instead of a free callback as there wil be a lot more things in the
stateid handling that could make use of it.  I'm not going to insist on
the ops vector at this point, but I think it would make things quite
a bit cleaner.


> +static void nfs4_free_generic_stateid(struct nfs4_stid *stid);

s/generic/ol/

Also maybe just move it into the right place instead of the forward
declaration given how trivial it is?

> +static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
> +{
> +	if (s->sc_file)
> +		put_nfs4_file(s->sc_file);
> +	kmem_cache_free(slab, s);
> +}

I think the layering here is wrong - the generic code should put the
file before calling into the ->free method as the file is in the
generic code.  Then the free callback can simply opencode the
kmem_cache_free instead of adding this helper.

> +static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
> +					 struct kmem_cache *slab)
>  {
>  	struct nfs4_stid *stid;
>  	int new_id;
> @@ -492,7 +500,22 @@ out_free:
>  
>  static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
>  {

Having and nfs4_alloc_stid and an nfs4_alloc_stateid is a very confusing
nameing scheme.  I think nfs4_alloc_stateid should be rename to
nfs4_alloc_ol_stateid.  And nfs4_alloc_stid might actually just be
redone to nfs4_init_stid, leaving the allocation to the caller similar
how the specific type handles the free in the ->free callback.

>  void
>  nfs4_put_delegation(struct nfs4_delegation *dp)
>  {
> -	if (nfs4_put_stid(deleg_slab, &dp->dl_stid))
> -		atomic_long_dec(&num_delegations);
> +	nfs4_put_stid(&dp->dl_stid);
>  }

And reason to keep nfs4_put_delegation around?

>  static void put_generic_stateid(struct nfs4_ol_stateid *stp)
>  {
> -	nfs4_put_stid(stateid_slab, &stp->st_stid);
> +	nfs4_put_stid(&stp->st_stid);
>  }

Ditto for put_generic_stateid.

--
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 0243a19dbef4..7b329b28734c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -71,6 +71,7 @@  static u64 current_sessionid = 1;
 
 /* forward declarations */
 static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner);
+static void nfs4_free_generic_stateid(struct nfs4_stid *stid);
 
 /* Locking: */
 
@@ -452,8 +453,15 @@  static void nfs4_file_put_access(struct nfs4_file *fp, u32 access)
 		__nfs4_file_put_access(fp, O_RDONLY);
 }
 
-static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct
-kmem_cache *slab)
+static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
+{
+	if (s->sc_file)
+		put_nfs4_file(s->sc_file);
+	kmem_cache_free(slab, s);
+}
+
+static struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl,
+					 struct kmem_cache *slab)
 {
 	struct nfs4_stid *stid;
 	int new_id;
@@ -492,7 +500,22 @@  out_free:
 
 static struct nfs4_ol_stateid * nfs4_alloc_stateid(struct nfs4_client *clp)
 {
-	return openlockstateid(nfs4_alloc_stid(clp, stateid_slab));
+	struct nfs4_stid *stid;
+	struct nfs4_ol_stateid *stp;
+
+	stid = nfs4_alloc_stid(clp, stateid_slab);
+	if (!stid)
+		return NULL;
+
+	stp = openlockstateid(stid);
+	stp->st_stid.sc_free = nfs4_free_generic_stateid;
+	return stp;
+}
+
+static void nfs4_free_deleg(struct nfs4_stid *stid)
+{
+	nfs4_free_stid(deleg_slab, stid);
+	atomic_long_dec(&num_delegations);
 }
 
 /*
@@ -586,6 +609,8 @@  alloc_init_deleg(struct nfs4_client *clp, struct svc_fh *current_fh)
 	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
 	if (dp == NULL)
 		goto out_dec;
+
+	dp->dl_stid.sc_free = nfs4_free_deleg;
 	/*
 	 * delegation seqid's are never incremented.  The 4.1 special
 	 * meaning of seqid 0 isn't meaningful, really, but let's avoid
@@ -611,32 +636,23 @@  static void remove_stid_locked(struct nfs4_client *clp, struct nfs4_stid *s)
 	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
 }
 
-static void nfs4_free_stid(struct kmem_cache *slab, struct nfs4_stid *s)
-{
-	if (s->sc_file)
-		put_nfs4_file(s->sc_file);
-	kmem_cache_free(slab, s);
-}
-
-static bool nfs4_put_stid(struct kmem_cache *slab, struct nfs4_stid *s)
+static void nfs4_put_stid(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;
+		return;
 	remove_stid_locked(clp, s);
 	spin_unlock(&clp->cl_lock);
-	nfs4_free_stid(slab, s);
-	return true;
+	s->sc_free(s);
 }
 
 void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
-	if (nfs4_put_stid(deleg_slab, &dp->dl_stid))
-		atomic_long_dec(&num_delegations);
+	nfs4_put_stid(&dp->dl_stid);
 }
 
 static void nfs4_put_deleg_lease(struct nfs4_file *fp)
@@ -887,14 +903,17 @@  static void unhash_generic_stateid(struct nfs4_ol_stateid *stp)
 	list_del(&stp->st_perstateowner);
 }
 
-static void close_generic_stateid(struct nfs4_ol_stateid *stp)
+static void nfs4_free_generic_stateid(struct nfs4_stid *stid)
 {
+	struct nfs4_ol_stateid *stp = openlockstateid(stid);
+
 	release_all_access(stp);
+	nfs4_free_stid(stateid_slab, stid);
 }
 
 static void put_generic_stateid(struct nfs4_ol_stateid *stp)
 {
-	nfs4_put_stid(stateid_slab, &stp->st_stid);
+	nfs4_put_stid(&stp->st_stid);
 }
 
 static void __release_lock_stateid(struct nfs4_ol_stateid *stp)
@@ -907,7 +926,6 @@  static void __release_lock_stateid(struct nfs4_ol_stateid *stp)
 	file = find_any_file(stp->st_stid.sc_file);
 	if (file)
 		filp_close(file, (fl_owner_t)lockowner(stp->st_stateowner));
-	close_generic_stateid(stp);
 	put_generic_stateid(stp);
 }
 
@@ -965,7 +983,6 @@  static void unhash_open_stateid(struct nfs4_ol_stateid *stp)
 {
 	unhash_generic_stateid(stp);
 	release_open_stateid_locks(stp);
-	close_generic_stateid(stp);
 }
 
 static void release_open_stateid(struct nfs4_ol_stateid *stp)
@@ -4501,8 +4518,11 @@  static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 		oo->oo_last_closed_stid = s;
 		/*
 		 * In the 4.0 case we need to keep the owners around a
-		 * little while to handle CLOSE replay.
+		 * little while to handle CLOSE replay. We still do need
+		 * to release any file access that is held by them
+		 * before returning however.
 		 */
+		release_all_access(s);
 		if (list_empty(&oo->oo_owner.so_stateids))
 			move_to_close_lru(oo, clp->net);
 	}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1be148ec9440..5c30e6064749 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -86,6 +86,8 @@  struct nfs4_stid {
 	stateid_t sc_stateid;
 	struct nfs4_client *sc_client;
 	struct nfs4_file *sc_file;
+
+	void (*sc_free)(struct nfs4_stid *);
 };
 
 struct nfs4_delegation {