nfsd4_locku / nfs4_free_lock_stateid question

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.

Quoted 10 lines · Jeff Layton

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

Quoted 10 lines · Jeff Layton

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

Quoted 12 lines · On Sun, 13 Jul 2014 05:19:19 -0700 Christoph Hellwig <[email protected]> wrote:

Quoted 10 lines · Jeff Layton

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.

Quoted 6 lines

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?

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

Quoted 35 lines · Jeff Layton

Quoted 12 lines · On Sun, 13 Jul 2014 05:19:19 -0700 Christoph Hellwig <[email protected]> wrote:

Quoted 10 lines · Jeff Layton

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.

Quoted 6 lines

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

Quoted 5 lines

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

Quoted 4 lines

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.

fs/nfsd/nfs4state.c nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, · line 4854

Quoted 8 lines

fs/nfsd/nfs4state.c +2 -1

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