nfsd4_locku / nfs4_free_lock_stateid question

nfsd4_locku / nfs4_free_lock_stateid question

[Christoph Hellwig]

Hi Jeff,

while reviewing some of your patches I've started wondering about
some v4 locking code.

In nfsd4_locku we're doing a call to find_any_file to grab a file
structure for the lock stateid, which nfs4_free_lock_stateid tries to
close. But what guarantees that we're actually getting the same file
descriptor back?

The nfs4_file is shared by and stateid that access a given inode,
so the first call to find_any_file might return the read/only file
structure because that's the only one available so far, while
by the time we unlock we might have a read/write and/or write-only file
available as well, which find_any_file will return.

It seems like the lock stateid needs a pointer to the actual file
locked, and keep a reference to it independent of the nfs4_file, or am I
missing something?

Re: nfsd4_locku / nfs4_free_lock_stateid question

[Jeff Layton]

On Sun, 13 Jul 2014 04:00:47 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> Hi Jeff,
> 
> while reviewing some of your patches I've started wondering about
> some v4 locking code.
> 
> In nfsd4_locku we're doing a call to find_any_file to grab a file
> structure for the lock stateid, which nfs4_free_lock_stateid tries to
> close. But what guarantees that we're actually getting the same file
> descriptor back?
> 
> The nfs4_file is shared by and stateid that access a given inode,
> so the first call to find_any_file might return the read/only file
> structure because that's the only one available so far, while
> by the time we unlock we might have a read/write and/or write-only file
> available as well, which find_any_file will return.
> 
> It seems like the lock stateid needs a pointer to the actual file
> locked, and keep a reference to it independent of the nfs4_file, or am I
> missing something?

It is weird, but I don't think it really matters as the filp is only
really used as a way to get to the inode -- it really doesn't matter
which struct file we use there. find_any_file will both take a
reference to and return the file, which is then eventually fput in
filp_close, so there should be no refcount leak or anything.

The weirdness all comes from the vfs-layer file locking interfaces,
many of which take a struct file argument when they really would be
fine with a struct inode. Maybe one of these days we can get around to
cleaning that up.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: nfsd4_locku / nfs4_free_lock_stateid question

[Christoph Hellwig]

On Sun, Jul 13, 2014 at 08:05:41AM -0400, Jeff Layton wrote:
> It is weird, but I don't think it really matters as the filp is only
> really used as a way to get to the inode -- it really doesn't matter
> which struct file we use there. find_any_file will both take a
> reference to and return the file, which is then eventually fput in
> filp_close, so there should be no refcount leak or anything.
> 
> The weirdness all comes from the vfs-layer file locking interfaces,
> many of which take a struct file argument when they really would be
> fine with a struct inode. Maybe one of these days we can get around to
> cleaning that up.

If filesystems get the file passed we should assume that they actually
use it.  In fact AFS does, but it's not NFS exportable at the moment,
and ceph does in a debug printk.  I'd be much happier to waste a pointer
in the lock stateid to avoid this inconsistant interface.  And it would
allow to kill find_any_fileas well..

Re: nfsd4_locku / nfs4_free_lock_stateid question

[Jeff Layton]

On Sun, 13 Jul 2014 05:19:19 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Jul 13, 2014 at 08:05:41AM -0400, Jeff Layton wrote:
> > It is weird, but I don't think it really matters as the filp is only
> > really used as a way to get to the inode -- it really doesn't matter
> > which struct file we use there. find_any_file will both take a
> > reference to and return the file, which is then eventually fput in
> > filp_close, so there should be no refcount leak or anything.
> > 
> > The weirdness all comes from the vfs-layer file locking interfaces,
> > many of which take a struct file argument when they really would be
> > fine with a struct inode. Maybe one of these days we can get around to
> > cleaning that up.
> 
> If filesystems get the file passed we should assume that they actually
> use it.  In fact AFS does, but it's not NFS exportable at the moment,
> and ceph does in a debug printk.  I'd be much happier to waste a pointer
> in the lock stateid to avoid this inconsistant interface.  And it would
> allow to kill find_any_fileas well..
> 

Fair enough -- that would be cleaner. I'll look into changing that as
well while I'm in here.

-- 
Jeff Layton <jlayton@primarydata.com>

Re: nfsd4_locku / nfs4_free_lock_stateid question

[Jeff Layton]

On Sun, 13 Jul 2014 05:19:19 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Sun, Jul 13, 2014 at 08:05:41AM -0400, Jeff Layton wrote:
> > It is weird, but I don't think it really matters as the filp is only
> > really used as a way to get to the inode -- it really doesn't matter
> > which struct file we use there. find_any_file will both take a
> > reference to and return the file, which is then eventually fput in
> > filp_close, so there should be no refcount leak or anything.
> > 
> > The weirdness all comes from the vfs-layer file locking interfaces,
> > many of which take a struct file argument when they really would be
> > fine with a struct inode. Maybe one of these days we can get around to
> > cleaning that up.
> 
> If filesystems get the file passed we should assume that they actually
> use it.  In fact AFS does, but it's not NFS exportable at the moment,
> and ceph does in a debug printk.  I'd be much happier to waste a pointer
> in the lock stateid to avoid this inconsistant interface.  And it would
> allow to kill find_any_fileas well..
> 

I started looking at this this morning...

FWIW, I don't see where AFS uses the struct file for anything
non-trivial in the filp_close codepaths. afs_do_unlk doesn't seem to do
much with it, and I don't see a ->flush operation for AFS. Am I missing
something there?

Here's what I think this change would look like. It builds but is
otherwise untested, and the commit log is sort of half-assed right now.
It should get slotted on top of this patch in the series:

    nfsd: do filp_close in sc_free callback for lock stateids

I'm not sure this really improves anything.

For one thing, we can only store a single struct file, but there's no
guarantee that the locks represented by the lock stateid all used that
file. Also, find_any_file is rather cheap, so I don't see this helping
performance much.

Given that, is this really worth the extra memory? Thoughts?

-------------------------[snip]---------------------------

[PATCH] nfsd: avoid having to search for struct file in lock stateid release codepath

Add a struct file pointer to struct nfs4_ol_stateid, and have nfsd4_lock
keep a reference to the file that it finds to set the lock. In the event
that the lock stateid represents more than one lock, then keep the
reference to the last struct file used. With this, we can also eliminate
find_any_file.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 43 +++++++++++++------------------------------
 fs/nfsd/state.h     | 17 +++++++++--------
 2 files changed, 22 insertions(+), 38 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4606b4ec6dc3..795f3ece5167 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -328,22 +328,6 @@ find_readable_file(struct nfs4_file *f)
 	return ret;
 }
 
-static struct file *
-find_any_file(struct nfs4_file *f)
-{
-	struct file *ret;
-
-	spin_lock(&f->fi_lock);
-	ret = __nfs4_get_fd(f, O_RDWR);
-	if (!ret) {
-		ret = __nfs4_get_fd(f, O_WRONLY);
-		if (!ret)
-			ret = __nfs4_get_fd(f, O_RDONLY);
-	}
-	spin_unlock(&f->fi_lock);
-	return ret;
-}
-
 static int num_delegations;
 unsigned long max_delegations;
 
@@ -914,9 +898,8 @@ static void nfs4_free_lock_stateid(struct nfs4_stid *stid)
 	struct nfs4_lockowner *lo = lockowner(stp->st_stateowner);
 	struct file *file;
 
-	file = find_any_file(stp->st_stid.sc_file);
-	if (file)
-		filp_close(file, (fl_owner_t)lo);
+	if (stp->st_file)
+		filp_close(stp->st_file, (fl_owner_t)lo);
 	nfs4_free_generic_stateid(stid);
 }
 
@@ -4866,8 +4849,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		case NFS4_READW_LT:
 			spin_lock(&fp->fi_lock);
 			filp = find_readable_file_locked(fp);
-			if (filp)
+			if (filp) {
+				swap(lock_stp->st_file, filp);
 				get_lock_access(lock_stp, NFS4_SHARE_ACCESS_READ);
+			}
 			spin_unlock(&fp->fi_lock);
 			file_lock->fl_type = F_RDLCK;
 			break;
@@ -4875,8 +4860,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		case NFS4_WRITEW_LT:
 			spin_lock(&fp->fi_lock);
 			filp = find_writeable_file_locked(fp);
-			if (filp)
+			if (filp) {
+				swap(lock_stp->st_file, filp);
 				get_lock_access(lock_stp, NFS4_SHARE_ACCESS_WRITE);
+			}
 			spin_unlock(&fp->fi_lock);
 			file_lock->fl_type = F_WRLCK;
 			break;
@@ -5038,7 +5025,6 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	    struct nfsd4_locku *locku)
 {
 	struct nfs4_ol_stateid *stp;
-	struct file *filp = NULL;
 	struct file_lock *file_lock = NULL;
 	__be32 status;
 	int err;
@@ -5058,8 +5044,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 					&stp, nn);
 	if (status)
 		goto out;
-	filp = find_any_file(stp->st_stid.sc_file);
-	if (!filp) {
+	if (!stp->st_file) {
 		status = nfserr_lock_range;
 		goto out;
 	}
@@ -5067,13 +5052,13 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (!file_lock) {
 		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
 		status = nfserr_jukebox;
-		goto fput;
+		goto out;
 	}
 	locks_init_lock(file_lock);
 	file_lock->fl_type = F_UNLCK;
 	file_lock->fl_owner = (fl_owner_t)lockowner(stp->st_stateowner);
 	file_lock->fl_pid = current->tgid;
-	file_lock->fl_file = filp;
+	file_lock->fl_file = stp->st_file;
 	file_lock->fl_flags = FL_POSIX;
 	file_lock->fl_lmops = &nfsd_posix_mng_ops;
 	file_lock->fl_start = locku->lu_offset;
@@ -5082,15 +5067,13 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 						locku->lu_length);
 	nfs4_transform_lock_offset(file_lock);
 
-	err = vfs_lock_file(filp, F_SETLK, file_lock, NULL);
+	err = vfs_lock_file(stp->st_file, F_SETLK, file_lock, NULL);
 	if (err) {
 		dprintk("NFSD: nfs4_locku: vfs_lock_file failed!\n");
 		goto out_nfserr;
 	}
 	update_stateid(&stp->st_stid.sc_stateid);
 	memcpy(&locku->lu_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
-fput:
-	fput(filp);
 out:
 	nfsd4_bump_seqid(cstate, status);
 	nfs4_unlock_state();
@@ -5100,7 +5083,7 @@ out:
 
 out_nfserr:
 	status = nfserrno(err);
-	goto fput;
+	goto out;
 }
 
 /*
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1b184f92ae51..e9733efc2fe4 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -405,14 +405,15 @@ struct nfs4_file {
 
 /* "ol" stands for "Open or Lock".  Better suggestions welcome. */
 struct nfs4_ol_stateid {
-	struct nfs4_stid    st_stid; /* must be first field */
-	struct list_head              st_perfile;
-	struct list_head              st_perstateowner;
-	struct list_head              st_locks;
-	struct nfs4_stateowner      * st_stateowner;
-	unsigned char                 st_access_bmap;
-	unsigned char                 st_deny_bmap;
-	struct nfs4_ol_stateid         * st_openstp;
+	struct nfs4_stid		st_stid;
+	struct list_head		st_perfile;
+	struct list_head		st_perstateowner;
+	struct list_head		st_locks;
+	struct nfs4_stateowner *	st_stateowner;
+	struct file *			st_file;
+	struct nfs4_ol_stateid *	st_openstp;
+	unsigned char			st_access_bmap;
+	unsigned char			st_deny_bmap;
 };
 
 static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)
-- 
1.9.3

Re: nfsd4_locku / nfs4_free_lock_stateid question

[J. Bruce Fields]

On Tue, Jul 15, 2014 at 08:13:34AM -0400, Jeff Layton wrote:
> On Sun, 13 Jul 2014 05:19:19 -0700
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Sun, Jul 13, 2014 at 08:05:41AM -0400, Jeff Layton wrote:
> > > It is weird, but I don't think it really matters as the filp is only
> > > really used as a way to get to the inode -- it really doesn't matter
> > > which struct file we use there. find_any_file will both take a
> > > reference to and return the file, which is then eventually fput in
> > > filp_close, so there should be no refcount leak or anything.
> > > 
> > > The weirdness all comes from the vfs-layer file locking interfaces,
> > > many of which take a struct file argument when they really would be
> > > fine with a struct inode. Maybe one of these days we can get around to
> > > cleaning that up.
> > 
> > If filesystems get the file passed we should assume that they actually
> > use it.  In fact AFS does, but it's not NFS exportable at the moment,
> > and ceph does in a debug printk.  I'd be much happier to waste a pointer
> > in the lock stateid to avoid this inconsistant interface.  And it would
> > allow to kill find_any_fileas well..
> > 
> 
> I started looking at this this morning...
> 
> FWIW, I don't see where AFS uses the struct file for anything
> non-trivial in the filp_close codepaths. afs_do_unlk doesn't seem to do
> much with it, and I don't see a ->flush operation for AFS. Am I missing
> something there?
> 
> Here's what I think this change would look like. It builds but is
> otherwise untested, and the commit log is sort of half-assed right now.
> It should get slotted on top of this patch in the series:
> 
>     nfsd: do filp_close in sc_free callback for lock stateids
> 
> I'm not sure this really improves anything.
> 
> For one thing, we can only store a single struct file, but there's no
> guarantee that the locks represented by the lock stateid all used that
> file.

Right, so after this patch st_file means "the last file used by a lock
operation using this lock stateid"?

> Also, find_any_file is rather cheap, so I don't see this helping
> performance much.
> 
> Given that, is this really worth the extra memory? Thoughts?

I'm not convinced it is.  But I understand that we're imposing a weird
assumption on the lock interface: "if you're exportable, you should use
the struct file we passed you only to get the inode".

It's out of scope for this work now, but I wonder whether my
f9d7562fdb9d "nfsd4: share file descriptors between stateid's" was a
mistake.  It also seemed potentially surprising to filesystems that we
could write or write-lock using a struct file originally opened for
read, but maybe that wasn't really a problem.

> @@ -4866,8 +4849,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		case NFS4_READW_LT:
>  			spin_lock(&fp->fi_lock);
>  			filp = find_readable_file_locked(fp);
> -			if (filp)
> +			if (filp) {
> +				swap(lock_stp->st_file, filp);
>  				get_lock_access(lock_stp, NFS4_SHARE_ACCESS_READ);

I haven't checked carefully, but in the case of a new lock stateid, is
st_file going to be NULL, in which case this is going to cause the
openmode check following this case statement to fail?

--b.

Re: nfsd4_locku / nfs4_free_lock_stateid question

[Christoph Hellwig]

On Tue, Jul 15, 2014 at 10:50:29AM -0400, J. Bruce Fields wrote:
> I'm not convinced it is.  But I understand that we're imposing a weird
> assumption on the lock interface: "if you're exportable, you should use
> the struct file we passed you only to get the inode".

We might as well stick with the old variant for now.  It's defintively
asking for trouble, but so far it worked..  I'll look into
refactoring the locking code so that it doesn't need the file instead.

Re: nfsd4_locku / nfs4_free_lock_stateid question

[Jeff Layton]

On Tue, 15 Jul 2014 10:50:29 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Jul 15, 2014 at 08:13:34AM -0400, Jeff Layton wrote:
> > On Sun, 13 Jul 2014 05:19:19 -0700
> > Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > > On Sun, Jul 13, 2014 at 08:05:41AM -0400, Jeff Layton wrote:
> > > > It is weird, but I don't think it really matters as the filp is only
> > > > really used as a way to get to the inode -- it really doesn't matter
> > > > which struct file we use there. find_any_file will both take a
> > > > reference to and return the file, which is then eventually fput in
> > > > filp_close, so there should be no refcount leak or anything.
> > > > 
> > > > The weirdness all comes from the vfs-layer file locking interfaces,
> > > > many of which take a struct file argument when they really would be
> > > > fine with a struct inode. Maybe one of these days we can get around to
> > > > cleaning that up.
> > > 
> > > If filesystems get the file passed we should assume that they actually
> > > use it.  In fact AFS does, but it's not NFS exportable at the moment,
> > > and ceph does in a debug printk.  I'd be much happier to waste a pointer
> > > in the lock stateid to avoid this inconsistant interface.  And it would
> > > allow to kill find_any_fileas well..
> > > 
> > 
> > I started looking at this this morning...
> > 
> > FWIW, I don't see where AFS uses the struct file for anything
> > non-trivial in the filp_close codepaths. afs_do_unlk doesn't seem to do
> > much with it, and I don't see a ->flush operation for AFS. Am I missing
> > something there?
> > 
> > Here's what I think this change would look like. It builds but is
> > otherwise untested, and the commit log is sort of half-assed right now.
> > It should get slotted on top of this patch in the series:
> > 
> >     nfsd: do filp_close in sc_free callback for lock stateids
> > 
> > I'm not sure this really improves anything.
> > 
> > For one thing, we can only store a single struct file, but there's no
> > guarantee that the locks represented by the lock stateid all used that
> > file.
> 
> Right, so after this patch st_file means "the last file used by a lock
> operation using this lock stateid"?
> 

Yes. If we did decide to use some variant of this, I might change the
name just to avoid confusion with the old st_file field.

> > Also, find_any_file is rather cheap, so I don't see this helping
> > performance much.
> > 
> > Given that, is this really worth the extra memory? Thoughts?
> 
> I'm not convinced it is.  But I understand that we're imposing a weird
> assumption on the lock interface: "if you're exportable, you should use
> the struct file we passed you only to get the inode".
> 
> It's out of scope for this work now, but I wonder whether my
> f9d7562fdb9d "nfsd4: share file descriptors between stateid's" was a
> mistake.  It also seemed potentially surprising to filesystems that we
> could write or write-lock using a struct file originally opened for
> read, but maybe that wasn't really a problem.
> 

Hmm...maybe.

I think though that you'd have had the same problem if you had tried to
use a different scheme. The VFS just doesn't do open upgrades today,
and that's the part that makes this so difficult.

Doing an upgrade means that you're going to change the struct file, and
that makes it difficult to guarantee that you'll always use the same one
for locking.


> > @@ -4866,8 +4849,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  		case NFS4_READW_LT:
> >  			spin_lock(&fp->fi_lock);
> >  			filp = find_readable_file_locked(fp);
> > -			if (filp)
> > +			if (filp) {
> > +				swap(lock_stp->st_file, filp);
> >  				get_lock_access(lock_stp, NFS4_SHARE_ACCESS_READ);
> 
> I haven't checked carefully, but in the case of a new lock stateid, is
> st_file going to be NULL, in which case this is going to cause the
> openmode check following this case statement to fail?
> 
> --b.


-- 
Jeff Layton <jlayton@primarydata.com>