On Wed, 2025-10-15 at 09:10 +1100, NeilBrown wrote:

On Tue, 14 Oct 2025, Jeff Layton wrote:

quoted

On Tue, 2025-10-14 at 16:34 +1100, NeilBrown wrote:

quoted

On Tue, 14 Oct 2025, Jeff Layton wrote:

quoted

The NFSv4.1 protocol adds support for directory delegations, but it
specifies that if you already have a delegation and try to request a new
one on the same filehandle, the server must reply that the delegation is
unavailable.

Add a new lease manager callback to allow the lease manager (nfsd in
this case) to impose this extra check when performing a setlease.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/locks.c               |  5 +++++
 include/linux/filelock.h | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/fs/locks.c b/fs/locks.c
index 0b16921fb52e602ea2e0c3de39d9d772af98ba7d..9e366b13674538dbf482ffdeee92fc717733ee20 100644
--- a/fs/locks.c
+++ b/fs/locks.c

@@ -1826,6 +1826,11 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr
 			continue;
 		}
 
+		/* Allow the lease manager to veto the setlease */
+		if (lease->fl_lmops->lm_may_setlease &&
+		    !lease->fl_lmops->lm_may_setlease(lease, fl))
+			goto out;
+

I don't see any locking around this.  What if the condition which
triggers a veto happens after this check, and before the lm_change
below?
Should lm_change implement the veto?  Return -EAGAIN?

The flc_lock is held over this check and any subsequent lease addition.
Is that not sufficient?

Ah - I didn't see that - sorry.

But I still wonder why ->lm_change cannot do the veto.

I also wonder if the current code can work.  If that loop finds an
existing lease with the same file and the same owner the it invokes
"continue" before the code that you added.
So unless I'm misunderstanding (again) in the case that you are
interested in, the new code doesn't run.

I wrote this a couple of years ago and had to go back and refresh my
memory as to why I did it this way...

The "same owner" check doesn't quite work here. The fl_owner points to
the struct nfs4_delegation, not the nfs4_client, so that test will
never return true. That means that lm_change never happens in this
scenario.

One approach to fix this might be to turn that check into a new
"lm_compare" operation that would return true if the delegations were
both owned by the same client. That could have other effects on the
lease handling code that I haven't considered yet however so I'm
hesitant to go that route.

I'm actually leaning now toward dropping this patch and doing this all
inside of nfsd. I think that's possible and that means less complexity
at the VFS layer. I'll take a stab at respinning this part of the
patchset and see if I can make that work instead.

Thanks for the review!
-- 
Jeff Layton [off-list ref]