[v1,012/104] NFSd: Clean up helper __release_lock_stateid

Commit Message

Jeff Layton June 19, 2014, 2:49 p.m. UTC

From: Trond Myklebust <[e-mail]>

Use filp_close() instead of open coding

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

Comments

Christoph Hellwig June 24, 2014, 11:53 a.m. UTC | #1

On Thu, Jun 19, 2014 at 10:49:18AM -0400, Jeff Layton wrote:
> From: Trond Myklebust <[e-mail]>
> 
> Use filp_close() instead of open coding

filp_close does more than what's currently opencoded here:

 - file_count debug check which seems pointless but harmless
 - calling ->flush. This doesn't do much on nfs exportable filesystem
   except for exofs, which does dumb shit in it that should be removed,
   but the maintainer refuse.
 - call into dnotify, which we probably should be doing here.

> -static void __release_lock_stateid(struct nfs4_ol_stateid *stp)
> +static void __release_lock_stateid(struct nfs4_lockowner *lo,
> +		struct nfs4_ol_stateid *stp)

But what's the point of explicitly passing the lock owner instead of
deriving it?

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

Jeff Layton June 24, 2014, noon UTC | #2

On Tue, 24 Jun 2014 04:53:36 -0700
Christoph Hellwig <[e-mail]> wrote:

> On Thu, Jun 19, 2014 at 10:49:18AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <[e-mail]>
> > 
> > Use filp_close() instead of open coding
> 
> filp_close does more than what's currently opencoded here:
> 
>  - file_count debug check which seems pointless but harmless
>  - calling ->flush. This doesn't do much on nfs exportable filesystem
>    except for exofs, which does dumb shit in it that should be removed,
>    but the maintainer refuse.
>  - call into dnotify, which we probably should be doing here.
> 

Yeah. It is a small change in behavior but seems like the right thing
to do. I'll flesh out the commit message to spell that out though.

> > -static void __release_lock_stateid(struct nfs4_ol_stateid *stp)
> > +static void __release_lock_stateid(struct nfs4_lockowner *lo,
> > +		struct nfs4_ol_stateid *stp)
> 
> But what's the point of explicitly passing the lock owner instead of
> deriving it?
> 

I thought that was a little silly too, but didn't care enough to change
it. I can only assume that Trond figured that the callers all needed to
know what the lockowner was so you wouldn't need to do two lockowner()
calls. Still, those are just container_of(), so I don't see the point
in avoiding them either. I'll change it not to pass the owner.

Thanks,

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8b0aa9ea343d..f4c71bc96970 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -739,7 +739,8 @@  static void free_generic_stateid(struct nfs4_ol_stateid *stp)
 	nfs4_free_stid(stateid_slab, &stp->st_stid);
 }
 
-static void __release_lock_stateid(struct nfs4_ol_stateid *stp)
+static void __release_lock_stateid(struct nfs4_lockowner *lo,
+		struct nfs4_ol_stateid *stp)
 {
 	struct file *file;
 
@@ -747,10 +748,8 @@  static void __release_lock_stateid(struct nfs4_ol_stateid *stp)
 	unhash_generic_stateid(stp);
 	unhash_stid(&stp->st_stid);
 	file = find_any_file(stp->st_file);
-	if (file) {
-		locks_remove_posix(file, (fl_owner_t)lockowner(stp->st_stateowner));
-		fput(file);
-	}
+	if (file)
+		filp_close(file, (fl_owner_t)lo);
 	close_generic_stateid(stp);
 	free_generic_stateid(stp);
 }
@@ -764,7 +763,7 @@  static void unhash_lockowner(struct nfs4_lockowner *lo)
 	while (!list_empty(&lo->lo_owner.so_stateids)) {
 		stp = list_first_entry(&lo->lo_owner.so_stateids,
 				struct nfs4_ol_stateid, st_perstateowner);
-		__release_lock_stateid(stp);
+		__release_lock_stateid(lo, stp);
 	}
 }
 
@@ -791,7 +790,7 @@  static void release_lock_stateid(struct nfs4_ol_stateid *stp)
 	struct nfs4_lockowner *lo;
 
 	lo = lockowner(stp->st_stateowner);
-	__release_lock_stateid(stp);
+	__release_lock_stateid(lo, stp);
 	release_lockowner_if_empty(lo);
 }