[PATCH 0/2] filelock: fix conflict detection with userland file delegations

[PATCH 0/2] filelock: fix conflict detection with userland file delegations

[Jeff Layton]

This patchset fixes the way that conflicts are detected when userland
requests file delegations. The problem is due to a hack that was added
long ago which worked up until userland could request a file delegation.

This fixes the bug and makes things a bit less hacky. Please consider
for v6.19.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Jeff Layton (2):
      filelock: add lease_dispose_list() helper
      filelock: allow lease_managers to dictate what qualifies as a conflict

 Documentation/filesystems/locking.rst |   1 +
 fs/locks.c                            | 119 +++++++++++++++++-----------------
 fs/nfsd/nfs4layouts.c                 |  11 +++-
 fs/nfsd/nfs4state.c                   |   7 ++
 include/linux/filelock.h              |   1 +
 5 files changed, 79 insertions(+), 60 deletions(-)
---
base-commit: 76c63ff12e067e1ff77b19a83c24774899ed01fc
change-id: 20251201-dir-deleg-ro-41a16bc22838

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/2] filelock: add lease_dispose_list() helper

[Jeff Layton]

...and call that from the lease handling code instead of
locks_dispose_list(). Remove the lease handling parts from
locks_dispose_list().

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/locks.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 7f4ccc7974bc8d3e82500ee692c6520b53f2280f..e974f8e180fe48682a271af4f143e6bc8e9c4d3b 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -369,10 +369,19 @@ locks_dispose_list(struct list_head *dispose)
 	while (!list_empty(dispose)) {
 		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
 		list_del_init(&flc->flc_list);
-		if (flc->flc_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT))
-			locks_free_lease(file_lease(flc));
-		else
-			locks_free_lock(file_lock(flc));
+		locks_free_lock(file_lock(flc));
+	}
+}
+
+static void
+lease_dispose_list(struct list_head *dispose)
+{
+	struct file_lock_core *flc;
+
+	while (!list_empty(dispose)) {
+		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
+		list_del_init(&flc->flc_list);
+		locks_free_lease(file_lease(flc));
 	}
 }
 
@@ -1620,7 +1629,7 @@ int __break_lease(struct inode *inode, unsigned int flags)
 	spin_unlock(&ctx->flc_lock);
 	percpu_up_read(&file_rwsem);
 
-	locks_dispose_list(&dispose);
+	lease_dispose_list(&dispose);
 	error = wait_event_interruptible_timeout(new_fl->c.flc_wait,
 						 list_empty(&new_fl->c.flc_blocked_member),
 						 break_time);
@@ -1643,7 +1652,7 @@ int __break_lease(struct inode *inode, unsigned int flags)
 out:
 	spin_unlock(&ctx->flc_lock);
 	percpu_up_read(&file_rwsem);
-	locks_dispose_list(&dispose);
+	lease_dispose_list(&dispose);
 free_lock:
 	locks_free_lease(new_fl);
 	return error;
@@ -1726,7 +1735,7 @@ static int __fcntl_getlease(struct file *filp, unsigned int flavor)
 		spin_unlock(&ctx->flc_lock);
 		percpu_up_read(&file_rwsem);
 
-		locks_dispose_list(&dispose);
+		lease_dispose_list(&dispose);
 	}
 	return type;
 }
@@ -1895,7 +1904,7 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr
 out:
 	spin_unlock(&ctx->flc_lock);
 	percpu_up_read(&file_rwsem);
-	locks_dispose_list(&dispose);
+	lease_dispose_list(&dispose);
 	if (is_deleg)
 		inode_unlock(inode);
 	if (!error && !my_fl)
@@ -1931,7 +1940,7 @@ static int generic_delete_lease(struct file *filp, void *owner)
 		error = fl->fl_lmops->lm_change(victim, F_UNLCK, &dispose);
 	spin_unlock(&ctx->flc_lock);
 	percpu_up_read(&file_rwsem);
-	locks_dispose_list(&dispose);
+	lease_dispose_list(&dispose);
 	return error;
 }
 
@@ -2726,7 +2735,7 @@ locks_remove_lease(struct file *filp, struct file_lock_context *ctx)
 	spin_unlock(&ctx->flc_lock);
 	percpu_up_read(&file_rwsem);
 
-	locks_dispose_list(&dispose);
+	lease_dispose_list(&dispose);
 }
 
 /*

-- 
2.52.0

[PATCH 2/2] filelock: allow lease_managers to dictate what qualifies as a conflict

[Jeff Layton]

Requesting a delegation on a file from the userland fcntl() interface
currently succeeds when there are conflicting opens present.

This is because the lease handling code ignores conflicting opens for
FL_LAYOUT and FL_DELEG leases. This was a hack put in place long ago,
because nfsd already checks for conflicts in its own way. The kernel
needs to perform this check for userland delegations the same way it is
done for leases, however.

Make this dependent on the lease_manager by adding a new
->lm_open_conflict() lease_manager operation and have
generic_add_lease() call that instead of check_conflicting_open().
Morph check_conflicting_open() into a ->lm_open_conflict() op that is
only called for userland leases/delegations. Set the
->lm_open_conflict() operations for nfsd to trivial functions that
always return 0.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 Documentation/filesystems/locking.rst |  1 +
 fs/locks.c                            | 90 ++++++++++++++++-------------------
 fs/nfsd/nfs4layouts.c                 | 11 ++++-
 fs/nfsd/nfs4state.c                   |  7 +++
 include/linux/filelock.h              |  1 +
 5 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
index 77704fde98457423beae7ff00525a7383e37132b..29d453a2201bcafa03b26b706e4c68eaf5683829 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -416,6 +416,7 @@ lm_change		yes		no			no
 lm_breaker_owns_lease:	yes     	no			no
 lm_lock_expirable	yes		no			no
 lm_expire_lock		no		no			yes
+lm_open_conflict        yes             no                      no
 ======================	=============	=================	=========
 
 buffer_head
diff --git a/fs/locks.c b/fs/locks.c
index e974f8e180fe48682a271af4f143e6bc8e9c4d3b..a58c51c2cdd0cc4496538ed54d063cd523264128 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -585,10 +585,50 @@ lease_setup(struct file_lease *fl, void **priv)
 	__f_setown(filp, task_pid(current), PIDTYPE_TGID, 0);
 }
 
+/**
+ * lease_open_conflict - see if the given file points to an inode that has
+ *			 an existing open that would conflict with the
+ *			 desired lease.
+ * @filp:	file to check
+ * @arg:	type of lease that we're trying to acquire
+ *
+ * Check to see if there's an existing open fd on this file that would
+ * conflict with the lease we're trying to set.
+ */
+static int
+lease_open_conflict(struct file *filp, const int arg)
+{
+	struct inode *inode = file_inode(filp);
+	int self_wcount = 0, self_rcount = 0;
+
+	if (arg == F_RDLCK)
+		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
+	else if (arg != F_WRLCK)
+		return 0;
+
+	/*
+	 * Make sure that only read/write count is from lease requestor.
+	 * Note that this will result in denying write leases when i_writecount
+	 * is negative, which is what we want.  (We shouldn't grant write leases
+	 * on files open for execution.)
+	 */
+	if (filp->f_mode & FMODE_WRITE)
+		self_wcount = 1;
+	else if (filp->f_mode & FMODE_READ)
+		self_rcount = 1;
+
+	if (atomic_read(&inode->i_writecount) != self_wcount ||
+	    atomic_read(&inode->i_readcount) != self_rcount)
+		return -EAGAIN;
+
+	return 0;
+}
+
 static const struct lease_manager_operations lease_manager_ops = {
 	.lm_break = lease_break_callback,
 	.lm_change = lease_modify,
 	.lm_setup = lease_setup,
+	.lm_open_conflict = lease_open_conflict,
 };
 
 /*
@@ -1753,52 +1793,6 @@ int fcntl_getdeleg(struct file *filp, struct delegation *deleg)
 	return 0;
 }
 
-/**
- * check_conflicting_open - see if the given file points to an inode that has
- *			    an existing open that would conflict with the
- *			    desired lease.
- * @filp:	file to check
- * @arg:	type of lease that we're trying to acquire
- * @flags:	current lock flags
- *
- * Check to see if there's an existing open fd on this file that would
- * conflict with the lease we're trying to set.
- */
-static int
-check_conflicting_open(struct file *filp, const int arg, int flags)
-{
-	struct inode *inode = file_inode(filp);
-	int self_wcount = 0, self_rcount = 0;
-
-	if (flags & FL_LAYOUT)
-		return 0;
-	if (flags & FL_DELEG)
-		/* We leave these checks to the caller */
-		return 0;
-
-	if (arg == F_RDLCK)
-		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
-	else if (arg != F_WRLCK)
-		return 0;
-
-	/*
-	 * Make sure that only read/write count is from lease requestor.
-	 * Note that this will result in denying write leases when i_writecount
-	 * is negative, which is what we want.  (We shouldn't grant write leases
-	 * on files open for execution.)
-	 */
-	if (filp->f_mode & FMODE_WRITE)
-		self_wcount = 1;
-	else if (filp->f_mode & FMODE_READ)
-		self_rcount = 1;
-
-	if (atomic_read(&inode->i_writecount) != self_wcount ||
-	    atomic_read(&inode->i_readcount) != self_rcount)
-		return -EAGAIN;
-
-	return 0;
-}
-
 static int
 generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **priv)
 {
@@ -1835,7 +1829,7 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr
 	percpu_down_read(&file_rwsem);
 	spin_lock(&ctx->flc_lock);
 	time_out_leases(inode, &dispose);
-	error = check_conflicting_open(filp, arg, lease->c.flc_flags);
+	error = lease->fl_lmops->lm_open_conflict(filp, arg);
 	if (error)
 		goto out;
 
@@ -1892,7 +1886,7 @@ generic_add_lease(struct file *filp, int arg, struct file_lease **flp, void **pr
 	 * precedes these checks.
 	 */
 	smp_mb();
-	error = check_conflicting_open(filp, arg, lease->c.flc_flags);
+	error = lease->fl_lmops->lm_open_conflict(filp, arg);
 	if (error) {
 		locks_unlink_lock_ctx(&lease->c);
 		goto out;
diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 683bd1130afe298f9df774684192c89f68102b72..ca7ec7a022bd5c12fad60ff9e51145d9cca55527 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -764,9 +764,16 @@ nfsd4_layout_lm_change(struct file_lease *onlist, int arg,
 	return lease_modify(onlist, arg, dispose);
 }
 
+static int
+nfsd4_layout_lm_open_conflict(struct file *filp, int arg)
+{
+	return 0;
+}
+
 static const struct lease_manager_operations nfsd4_layouts_lm_ops = {
-	.lm_break	= nfsd4_layout_lm_break,
-	.lm_change	= nfsd4_layout_lm_change,
+	.lm_break		= nfsd4_layout_lm_break,
+	.lm_change		= nfsd4_layout_lm_change,
+	.lm_open_conflict	= nfsd4_layout_lm_open_conflict,
 };
 
 int
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8f8c9385101e15b64883eabec71775f26b14f890..669fabb095407e61525e5b71268cf1f06fc09877 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5543,10 +5543,17 @@ nfsd_change_deleg_cb(struct file_lease *onlist, int arg,
 		return -EAGAIN;
 }
 
+static int
+nfsd4_deleg_lm_open_conflict(struct file *filp, int arg)
+{
+	return 0;
+}
+
 static const struct lease_manager_operations nfsd_lease_mng_ops = {
 	.lm_breaker_owns_lease = nfsd_breaker_owns_lease,
 	.lm_break = nfsd_break_deleg_cb,
 	.lm_change = nfsd_change_deleg_cb,
+	.lm_open_conflict = nfsd4_deleg_lm_open_conflict,
 };
 
 static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, struct nfs4_stateowner *so, u32 seqid)
diff --git a/include/linux/filelock.h b/include/linux/filelock.h
index 54b824c05299261e6bd6acc4175cb277ea35b35d..2f5e5588ee0733c200103801d0d2ba19bebbf9af 100644
--- a/include/linux/filelock.h
+++ b/include/linux/filelock.h
@@ -49,6 +49,7 @@ struct lease_manager_operations {
 	int (*lm_change)(struct file_lease *, int, struct list_head *);
 	void (*lm_setup)(struct file_lease *, void **);
 	bool (*lm_breaker_owns_lease)(struct file_lease *);
+	int (*lm_open_conflict)(struct file *, int);
 };
 
 struct lock_manager {

-- 
2.52.0

Re: [PATCH 0/2] filelock: fix conflict detection with userland file delegations

[Chuck Lever]

On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> This patchset fixes the way that conflicts are detected when userland
> requests file delegations. The problem is due to a hack that was added
> long ago which worked up until userland could request a file delegation.
>
> This fixes the bug and makes things a bit less hacky. Please consider
> for v6.19.

I would like a little more time to review this carefully, especially
in light of similar work Dai has already posted in this area. If by
"v6.19" you mean "not before v6.19-rcN where N > 3", then that WFM.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> Jeff Layton (2):
>       filelock: add lease_dispose_list() helper
>       filelock: allow lease_managers to dictate what qualifies as a conflict
>
>  Documentation/filesystems/locking.rst |   1 +
>  fs/locks.c                            | 119 +++++++++++++++++-----------------
>  fs/nfsd/nfs4layouts.c                 |  11 +++-
>  fs/nfsd/nfs4state.c                   |   7 ++
>  include/linux/filelock.h              |   1 +
>  5 files changed, 79 insertions(+), 60 deletions(-)
> ---
> base-commit: 76c63ff12e067e1ff77b19a83c24774899ed01fc
> change-id: 20251201-dir-deleg-ro-41a16bc22838
>
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>

-- 
Chuck Lever

Re: [PATCH 0/2] filelock: fix conflict detection with userland file delegations

[Jeff Layton]

On Mon, 2025-12-01 at 10:19 -0500, Chuck Lever wrote:
> 
> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> > This patchset fixes the way that conflicts are detected when userland
> > requests file delegations. The problem is due to a hack that was added
> > long ago which worked up until userland could request a file delegation.
> > 
> > This fixes the bug and makes things a bit less hacky. Please consider
> > for v6.19.
> 
> I would like a little more time to review this carefully, especially
> in light of similar work Dai has already posted in this area. If by
> "v6.19" you mean "not before v6.19-rcN where N > 3", then that WFM.
> 

Ok. Do you have a specific concern? FWIW, I did mention to Dai that the
first patch in this series would make it more palatable to handle his
new lm_breaker_timedout operation in lease_dispose_list().

By v6.19, I mean before v6.19 ships. This bug needs to be fixed before
we release a kernel that provides the new F_SETDELEG interface.

> 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > Jeff Layton (2):
> >       filelock: add lease_dispose_list() helper
> >       filelock: allow lease_managers to dictate what qualifies as a conflict
> > 
> >  Documentation/filesystems/locking.rst |   1 +
> >  fs/locks.c                            | 119 +++++++++++++++++-----------------
> >  fs/nfsd/nfs4layouts.c                 |  11 +++-
> >  fs/nfsd/nfs4state.c                   |   7 ++
> >  include/linux/filelock.h              |   1 +
> >  5 files changed, 79 insertions(+), 60 deletions(-)
> > ---
> > base-commit: 76c63ff12e067e1ff77b19a83c24774899ed01fc
> > change-id: 20251201-dir-deleg-ro-41a16bc22838
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/2] filelock: fix conflict detection with userland file delegations

[Chuck Lever]

On Mon, Dec 1, 2025, at 10:52 AM, Jeff Layton wrote:
> On Mon, 2025-12-01 at 10:19 -0500, Chuck Lever wrote:
>> 
>> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
>> > This patchset fixes the way that conflicts are detected when userland
>> > requests file delegations. The problem is due to a hack that was added
>> > long ago which worked up until userland could request a file delegation.
>> > 
>> > This fixes the bug and makes things a bit less hacky. Please consider
>> > for v6.19.
>> 
>> I would like a little more time to review this carefully, especially
>> in light of similar work Dai has already posted in this area. If by
>> "v6.19" you mean "not before v6.19-rcN where N > 3", then that WFM.
>> 
>
> Ok. Do you have a specific concern?

It looks so similar to what Dai was doing to deal with nfsd deadlocking
and my recent RFC in the same area that we should ensure that these
efforts are all going in a compatible direction.

Clearly, adding callbacks to NFSD that just return 0 is not a
functional risk ;-) But during a merge window I can't guarantee I'll
have time to look at this closely.


> FWIW, I did mention to Dai that the
> first patch in this series would make it more palatable to handle his
> new lm_breaker_timedout operation in lease_dispose_list().
>
> By v6.19, I mean before v6.19 ships. This bug needs to be fixed before
> we release a kernel that provides the new F_SETDELEG interface.

No problem. v6.19-rc rather than in the merge window is all I need.


>> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> > ---
>> > Jeff Layton (2):
>> >       filelock: add lease_dispose_list() helper
>> >       filelock: allow lease_managers to dictate what qualifies as a conflict
>> > 
>> >  Documentation/filesystems/locking.rst |   1 +
>> >  fs/locks.c                            | 119 +++++++++++++++++-----------------
>> >  fs/nfsd/nfs4layouts.c                 |  11 +++-
>> >  fs/nfsd/nfs4state.c                   |   7 ++
>> >  include/linux/filelock.h              |   1 +
>> >  5 files changed, 79 insertions(+), 60 deletions(-)
>> > ---
>> > base-commit: 76c63ff12e067e1ff77b19a83c24774899ed01fc
>> > change-id: 20251201-dir-deleg-ro-41a16bc22838
>> > 
>> > Best regards,
>> > -- 
>> > Jeff Layton <jlayton@kernel.org>
>
> -- 
> Jeff Layton <jlayton@kernel.org>

-- 
Chuck Lever

Re: [PATCH 1/2] filelock: add lease_dispose_list() helper

[Chuck Lever]

On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> ...and call that from the lease handling code instead of
> locks_dispose_list(). Remove the lease handling parts from
> locks_dispose_list().

The actual change here isn't bothering me, but I'm having trouble
understanding why it's needed. It doesn't appear to be a strict
functional prerequisite for 2/2.

A little more context in the commit message would be helpful.
Sample commit description:

  The lease-handling code paths always know they're disposing of leases,
  yet locks_dispose_list() checks flags at runtime to determine whether
  to call locks_free_lease() or locks_free_lock().

  Split out a dedicated lease_dispose_list() helper for lease code paths.
  This makes the type handling explicit and prepares for the upcoming
  lease_manager enhancements where lease-specific operations are being
  consolidated.

But that reflects only my naive understanding of the patch. You
might have something else in mind.


> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/locks.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/fs/locks.c b/fs/locks.c
> index 
> 7f4ccc7974bc8d3e82500ee692c6520b53f2280f..e974f8e180fe48682a271af4f143e6bc8e9c4d3b 
> 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -369,10 +369,19 @@ locks_dispose_list(struct list_head *dispose)
>  	while (!list_empty(dispose)) {
>  		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
>  		list_del_init(&flc->flc_list);
> -		if (flc->flc_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT))
> -			locks_free_lease(file_lease(flc));
> -		else
> -			locks_free_lock(file_lock(flc));
> +		locks_free_lock(file_lock(flc));
> +	}
> +}
> +
> +static void
> +lease_dispose_list(struct list_head *dispose)
> +{
> +	struct file_lock_core *flc;
> +
> +	while (!list_empty(dispose)) {
> +		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
> +		list_del_init(&flc->flc_list);
> +		locks_free_lease(file_lease(flc));
>  	}
>  }
> 
> @@ -1620,7 +1629,7 @@ int __break_lease(struct inode *inode, unsigned int flags)
>  	spin_unlock(&ctx->flc_lock);
>  	percpu_up_read(&file_rwsem);
> 
> -	locks_dispose_list(&dispose);
> +	lease_dispose_list(&dispose);
>  	error = wait_event_interruptible_timeout(new_fl->c.flc_wait,
>  						 list_empty(&new_fl->c.flc_blocked_member),
>  						 break_time);
> @@ -1643,7 +1652,7 @@ int __break_lease(struct inode *inode, unsigned 
> int flags)
>  out:
>  	spin_unlock(&ctx->flc_lock);
>  	percpu_up_read(&file_rwsem);
> -	locks_dispose_list(&dispose);
> +	lease_dispose_list(&dispose);
>  free_lock:
>  	locks_free_lease(new_fl);
>  	return error;
> @@ -1726,7 +1735,7 @@ static int __fcntl_getlease(struct file *filp, 
> unsigned int flavor)
>  		spin_unlock(&ctx->flc_lock);
>  		percpu_up_read(&file_rwsem);
> 
> -		locks_dispose_list(&dispose);
> +		lease_dispose_list(&dispose);
>  	}
>  	return type;
>  }
> @@ -1895,7 +1904,7 @@ generic_add_lease(struct file *filp, int arg, 
> struct file_lease **flp, void **pr
>  out:
>  	spin_unlock(&ctx->flc_lock);
>  	percpu_up_read(&file_rwsem);
> -	locks_dispose_list(&dispose);
> +	lease_dispose_list(&dispose);
>  	if (is_deleg)
>  		inode_unlock(inode);
>  	if (!error && !my_fl)
> @@ -1931,7 +1940,7 @@ static int generic_delete_lease(struct file 
> *filp, void *owner)
>  		error = fl->fl_lmops->lm_change(victim, F_UNLCK, &dispose);
>  	spin_unlock(&ctx->flc_lock);
>  	percpu_up_read(&file_rwsem);
> -	locks_dispose_list(&dispose);
> +	lease_dispose_list(&dispose);
>  	return error;
>  }
> 
> @@ -2726,7 +2735,7 @@ locks_remove_lease(struct file *filp, struct 
> file_lock_context *ctx)
>  	spin_unlock(&ctx->flc_lock);
>  	percpu_up_read(&file_rwsem);
> 
> -	locks_dispose_list(&dispose);
> +	lease_dispose_list(&dispose);
>  }
> 
>  /*
>
> -- 
> 2.52.0

-- 
Chuck Lever

Re: [PATCH 2/2] filelock: allow lease_managers to dictate what qualifies as a conflict

[Chuck Lever]

On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> Requesting a delegation on a file from the userland fcntl() interface
> currently succeeds when there are conflicting opens present.
>
> This is because the lease handling code ignores conflicting opens for
> FL_LAYOUT and FL_DELEG leases. This was a hack put in place long ago,
> because nfsd already checks for conflicts in its own way. The kernel
> needs to perform this check for userland delegations the same way it is
> done for leases, however.
>
> Make this dependent on the lease_manager by adding a new
> ->lm_open_conflict() lease_manager operation and have
> generic_add_lease() call that instead of check_conflicting_open().
> Morph check_conflicting_open() into a ->lm_open_conflict() op that is
> only called for userland leases/delegations. Set the
> ->lm_open_conflict() operations for nfsd to trivial functions that
> always return 0.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  Documentation/filesystems/locking.rst |  1 +
>  fs/locks.c                            | 90 ++++++++++++++++-------------------
>  fs/nfsd/nfs4layouts.c                 | 11 ++++-
>  fs/nfsd/nfs4state.c                   |  7 +++
>  include/linux/filelock.h              |  1 +
>  5 files changed, 60 insertions(+), 50 deletions(-)
>
> diff --git a/Documentation/filesystems/locking.rst 
> b/Documentation/filesystems/locking.rst
> index 
> 77704fde98457423beae7ff00525a7383e37132b..29d453a2201bcafa03b26b706e4c68eaf5683829 
> 100644
> --- a/Documentation/filesystems/locking.rst
> +++ b/Documentation/filesystems/locking.rst
> @@ -416,6 +416,7 @@ lm_change		yes		no			no
>  lm_breaker_owns_lease:	yes     	no			no
>  lm_lock_expirable	yes		no			no
>  lm_expire_lock		no		no			yes
> +lm_open_conflict        yes             no                      no
>  ======================	=============	=================	=========
> 
>  buffer_head
> diff --git a/fs/locks.c b/fs/locks.c
> index 
> e974f8e180fe48682a271af4f143e6bc8e9c4d3b..a58c51c2cdd0cc4496538ed54d063cd523264128 
> 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -585,10 +585,50 @@ lease_setup(struct file_lease *fl, void **priv)
>  	__f_setown(filp, task_pid(current), PIDTYPE_TGID, 0);
>  }
> 
> +/**
> + * lease_open_conflict - see if the given file points to an inode that has
> + *			 an existing open that would conflict with the
> + *			 desired lease.
> + * @filp:	file to check
> + * @arg:	type of lease that we're trying to acquire
> + *
> + * Check to see if there's an existing open fd on this file that would
> + * conflict with the lease we're trying to set.
> + */
> +static int
> +lease_open_conflict(struct file *filp, const int arg)
> +{
> +	struct inode *inode = file_inode(filp);
> +	int self_wcount = 0, self_rcount = 0;
> +
> +	if (arg == F_RDLCK)
> +		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> +	else if (arg != F_WRLCK)
> +		return 0;
> +
> +	/*
> +	 * Make sure that only read/write count is from lease requestor.
> +	 * Note that this will result in denying write leases when i_writecount
> +	 * is negative, which is what we want.  (We shouldn't grant write leases
> +	 * on files open for execution.)
> +	 */
> +	if (filp->f_mode & FMODE_WRITE)
> +		self_wcount = 1;
> +	else if (filp->f_mode & FMODE_READ)
> +		self_rcount = 1;
> +
> +	if (atomic_read(&inode->i_writecount) != self_wcount ||
> +	    atomic_read(&inode->i_readcount) != self_rcount)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
>  static const struct lease_manager_operations lease_manager_ops = {
>  	.lm_break = lease_break_callback,
>  	.lm_change = lease_modify,
>  	.lm_setup = lease_setup,
> +	.lm_open_conflict = lease_open_conflict,
>  };
> 
>  /*
> @@ -1753,52 +1793,6 @@ int fcntl_getdeleg(struct file *filp, struct 
> delegation *deleg)
>  	return 0;
>  }
> 
> -/**
> - * check_conflicting_open - see if the given file points to an inode 
> that has
> - *			    an existing open that would conflict with the
> - *			    desired lease.
> - * @filp:	file to check
> - * @arg:	type of lease that we're trying to acquire
> - * @flags:	current lock flags
> - *
> - * Check to see if there's an existing open fd on this file that would
> - * conflict with the lease we're trying to set.
> - */
> -static int
> -check_conflicting_open(struct file *filp, const int arg, int flags)
> -{
> -	struct inode *inode = file_inode(filp);
> -	int self_wcount = 0, self_rcount = 0;
> -
> -	if (flags & FL_LAYOUT)
> -		return 0;
> -	if (flags & FL_DELEG)
> -		/* We leave these checks to the caller */
> -		return 0;
> -
> -	if (arg == F_RDLCK)
> -		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> -	else if (arg != F_WRLCK)
> -		return 0;
> -
> -	/*
> -	 * Make sure that only read/write count is from lease requestor.
> -	 * Note that this will result in denying write leases when 
> i_writecount
> -	 * is negative, which is what we want.  (We shouldn't grant write 
> leases
> -	 * on files open for execution.)
> -	 */
> -	if (filp->f_mode & FMODE_WRITE)
> -		self_wcount = 1;
> -	else if (filp->f_mode & FMODE_READ)
> -		self_rcount = 1;
> -
> -	if (atomic_read(&inode->i_writecount) != self_wcount ||
> -	    atomic_read(&inode->i_readcount) != self_rcount)
> -		return -EAGAIN;
> -
> -	return 0;
> -}
> -
>  static int
>  generic_add_lease(struct file *filp, int arg, struct file_lease **flp, 
> void **priv)
>  {
> @@ -1835,7 +1829,7 @@ generic_add_lease(struct file *filp, int arg, 
> struct file_lease **flp, void **pr
>  	percpu_down_read(&file_rwsem);
>  	spin_lock(&ctx->flc_lock);
>  	time_out_leases(inode, &dispose);
> -	error = check_conflicting_open(filp, arg, lease->c.flc_flags);
> +	error = lease->fl_lmops->lm_open_conflict(filp, arg);
>  	if (error)
>  		goto out;
> 
> @@ -1892,7 +1886,7 @@ generic_add_lease(struct file *filp, int arg, 
> struct file_lease **flp, void **pr
>  	 * precedes these checks.
>  	 */
>  	smp_mb();
> -	error = check_conflicting_open(filp, arg, lease->c.flc_flags);
> +	error = lease->fl_lmops->lm_open_conflict(filp, arg);
>  	if (error) {
>  		locks_unlink_lock_ctx(&lease->c);
>  		goto out;
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 
> 683bd1130afe298f9df774684192c89f68102b72..ca7ec7a022bd5c12fad60ff9e51145d9cca55527 
> 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -764,9 +764,16 @@ nfsd4_layout_lm_change(struct file_lease *onlist, 
> int arg,
>  	return lease_modify(onlist, arg, dispose);
>  }
> 
> +static int
> +nfsd4_layout_lm_open_conflict(struct file *filp, int arg)
> +{
> +	return 0;
> +}
> +

The usual idiom for no-op callbacks is to make them optional.
Then generic_add_lease would check if the ->lm_open_conflict
callback is defined first and skip the call if it's not.

If that doesn't make sense to do, and these NFSD-specific
functions need to remain, then our usual practice is to add
a kdoc comment for both of the new functions that looks like
the one you added above for lease_open_conflict().


Otherwise, I'm comfortable that this change fits in with the
deadlock prevention patches we are considering for NFSD.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

For both 1/2 and 2/2.


>  static const struct lease_manager_operations nfsd4_layouts_lm_ops = {
> -	.lm_break	= nfsd4_layout_lm_break,
> -	.lm_change	= nfsd4_layout_lm_change,
> +	.lm_break		= nfsd4_layout_lm_break,
> +	.lm_change		= nfsd4_layout_lm_change,
> +	.lm_open_conflict	= nfsd4_layout_lm_open_conflict,
>  };
> 
>  int
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 
> 8f8c9385101e15b64883eabec71775f26b14f890..669fabb095407e61525e5b71268cf1f06fc09877 
> 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5543,10 +5543,17 @@ nfsd_change_deleg_cb(struct file_lease *onlist, 
> int arg,
>  		return -EAGAIN;
>  }
> 
> +static int
> +nfsd4_deleg_lm_open_conflict(struct file *filp, int arg)
> +{
> +	return 0;
> +}
> +
>  static const struct lease_manager_operations nfsd_lease_mng_ops = {
>  	.lm_breaker_owns_lease = nfsd_breaker_owns_lease,
>  	.lm_break = nfsd_break_deleg_cb,
>  	.lm_change = nfsd_change_deleg_cb,
> +	.lm_open_conflict = nfsd4_deleg_lm_open_conflict,
>  };
> 
>  static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, 
> struct nfs4_stateowner *so, u32 seqid)
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index 
> 54b824c05299261e6bd6acc4175cb277ea35b35d..2f5e5588ee0733c200103801d0d2ba19bebbf9af 
> 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -49,6 +49,7 @@ struct lease_manager_operations {
>  	int (*lm_change)(struct file_lease *, int, struct list_head *);
>  	void (*lm_setup)(struct file_lease *, void **);
>  	bool (*lm_breaker_owns_lease)(struct file_lease *);
> +	int (*lm_open_conflict)(struct file *, int);
>  };
> 
>  struct lock_manager {
>
> -- 
> 2.52.0

-- 
Chuck Lever

Re: [PATCH 1/2] filelock: add lease_dispose_list() helper

[Jeff Layton]

On Wed, 2025-12-03 at 13:55 -0500, Chuck Lever wrote:
> 
> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> > ...and call that from the lease handling code instead of
> > locks_dispose_list(). Remove the lease handling parts from
> > locks_dispose_list().
> 
> The actual change here isn't bothering me, but I'm having trouble
> understanding why it's needed. It doesn't appear to be a strict
> functional prerequisite for 2/2.
> 

It's not. We can table this patch for now if that's preferable, but I
do think it's a worthwhile cleanup.
 
> A little more context in the commit message would be helpful.
> Sample commit description:
> 
>   The lease-handling code paths always know they're disposing of leases,
>   yet locks_dispose_list() checks flags at runtime to determine whether
>   to call locks_free_lease() or locks_free_lock().
> 
>   Split out a dedicated lease_dispose_list() helper for lease code paths.
>   This makes the type handling explicit and prepares for the upcoming
>   lease_manager enhancements where lease-specific operations are being
>   consolidated.
> 

I may crib this if I end up resending it.

> But that reflects only my naive understanding of the patch. You
> might have something else in mind.
> 
> 

Nope, no ulterior motive here. It's just a nice to have cleanup that
helps to further separate the lock and lease handling code.

> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/locks.c | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> > 
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 
> > 7f4ccc7974bc8d3e82500ee692c6520b53f2280f..e974f8e180fe48682a271af4f143e6bc8e9c4d3b 
> > 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -369,10 +369,19 @@ locks_dispose_list(struct list_head *dispose)
> >  	while (!list_empty(dispose)) {
> >  		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
> >  		list_del_init(&flc->flc_list);
> > -		if (flc->flc_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT))
> > -			locks_free_lease(file_lease(flc));
> > -		else
> > -			locks_free_lock(file_lock(flc));
> > +		locks_free_lock(file_lock(flc));
> > +	}
> > +}
> > +
> > +static void
> > +lease_dispose_list(struct list_head *dispose)
> > +{
> > +	struct file_lock_core *flc;
> > +
> > +	while (!list_empty(dispose)) {
> > +		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
> > +		list_del_init(&flc->flc_list);
> > +		locks_free_lease(file_lease(flc));
> >  	}
> >  }
> > 
> > @@ -1620,7 +1629,7 @@ int __break_lease(struct inode *inode, unsigned int flags)
> >  	spin_unlock(&ctx->flc_lock);
> >  	percpu_up_read(&file_rwsem);
> > 
> > -	locks_dispose_list(&dispose);
> > +	lease_dispose_list(&dispose);
> >  	error = wait_event_interruptible_timeout(new_fl->c.flc_wait,
> >  						 list_empty(&new_fl->c.flc_blocked_member),
> >  						 break_time);
> > @@ -1643,7 +1652,7 @@ int __break_lease(struct inode *inode, unsigned 
> > int flags)
> >  out:
> >  	spin_unlock(&ctx->flc_lock);
> >  	percpu_up_read(&file_rwsem);
> > -	locks_dispose_list(&dispose);
> > +	lease_dispose_list(&dispose);
> >  free_lock:
> >  	locks_free_lease(new_fl);
> >  	return error;
> > @@ -1726,7 +1735,7 @@ static int __fcntl_getlease(struct file *filp, 
> > unsigned int flavor)
> >  		spin_unlock(&ctx->flc_lock);
> >  		percpu_up_read(&file_rwsem);
> > 
> > -		locks_dispose_list(&dispose);
> > +		lease_dispose_list(&dispose);
> >  	}
> >  	return type;
> >  }
> > @@ -1895,7 +1904,7 @@ generic_add_lease(struct file *filp, int arg, 
> > struct file_lease **flp, void **pr
> >  out:
> >  	spin_unlock(&ctx->flc_lock);
> >  	percpu_up_read(&file_rwsem);
> > -	locks_dispose_list(&dispose);
> > +	lease_dispose_list(&dispose);
> >  	if (is_deleg)
> >  		inode_unlock(inode);
> >  	if (!error && !my_fl)
> > @@ -1931,7 +1940,7 @@ static int generic_delete_lease(struct file 
> > *filp, void *owner)
> >  		error = fl->fl_lmops->lm_change(victim, F_UNLCK, &dispose);
> >  	spin_unlock(&ctx->flc_lock);
> >  	percpu_up_read(&file_rwsem);
> > -	locks_dispose_list(&dispose);
> > +	lease_dispose_list(&dispose);
> >  	return error;
> >  }
> > 
> > @@ -2726,7 +2735,7 @@ locks_remove_lease(struct file *filp, struct 
> > file_lock_context *ctx)
> >  	spin_unlock(&ctx->flc_lock);
> >  	percpu_up_read(&file_rwsem);
> > 
> > -	locks_dispose_list(&dispose);
> > +	lease_dispose_list(&dispose);
> >  }
> > 
> >  /*
> > 
> > -- 
> > 2.52.0

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] filelock: add lease_dispose_list() helper

[Chuck Lever]

On 12/3/25 2:33 PM, Jeff Layton wrote:
> On Wed, 2025-12-03 at 13:55 -0500, Chuck Lever wrote:
>>
>> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
>>> ...and call that from the lease handling code instead of
>>> locks_dispose_list(). Remove the lease handling parts from
>>> locks_dispose_list().
>>
>> The actual change here isn't bothering me, but I'm having trouble
>> understanding why it's needed. It doesn't appear to be a strict
>> functional prerequisite for 2/2.
>>
> 
> It's not. We can table this patch for now if that's preferable, but I
> do think it's a worthwhile cleanup.
>  
>> A little more context in the commit message would be helpful.
>> Sample commit description:
>>
>>   The lease-handling code paths always know they're disposing of leases,
>>   yet locks_dispose_list() checks flags at runtime to determine whether
>>   to call locks_free_lease() or locks_free_lock().
>>
>>   Split out a dedicated lease_dispose_list() helper for lease code paths.
>>   This makes the type handling explicit and prepares for the upcoming
>>   lease_manager enhancements where lease-specific operations are being
>>   consolidated.
>>
> 
> I may crib this if I end up resending it.
> 
>> But that reflects only my naive understanding of the patch. You
>> might have something else in mind.
>>
>>
> 
> Nope, no ulterior motive here. It's just a nice to have cleanup that
> helps to further separate the lock and lease handling code.

Yep, it's a good clean-up.


>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>>  fs/locks.c | 29 +++++++++++++++++++----------
>>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/fs/locks.c b/fs/locks.c
>>> index 
>>> 7f4ccc7974bc8d3e82500ee692c6520b53f2280f..e974f8e180fe48682a271af4f143e6bc8e9c4d3b 
>>> 100644
>>> --- a/fs/locks.c
>>> +++ b/fs/locks.c
>>> @@ -369,10 +369,19 @@ locks_dispose_list(struct list_head *dispose)
>>>  	while (!list_empty(dispose)) {
>>>  		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
>>>  		list_del_init(&flc->flc_list);
>>> -		if (flc->flc_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT))
>>> -			locks_free_lease(file_lease(flc));
>>> -		else
>>> -			locks_free_lock(file_lock(flc));
>>> +		locks_free_lock(file_lock(flc));
>>> +	}
>>> +}
>>> +
>>> +static void
>>> +lease_dispose_list(struct list_head *dispose)
>>> +{
>>> +	struct file_lock_core *flc;
>>> +
>>> +	while (!list_empty(dispose)) {
>>> +		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
>>> +		list_del_init(&flc->flc_list);
>>> +		locks_free_lease(file_lease(flc));
>>>  	}
>>>  }
>>>
>>> @@ -1620,7 +1629,7 @@ int __break_lease(struct inode *inode, unsigned int flags)
>>>  	spin_unlock(&ctx->flc_lock);
>>>  	percpu_up_read(&file_rwsem);
>>>
>>> -	locks_dispose_list(&dispose);
>>> +	lease_dispose_list(&dispose);
>>>  	error = wait_event_interruptible_timeout(new_fl->c.flc_wait,
>>>  						 list_empty(&new_fl->c.flc_blocked_member),
>>>  						 break_time);
>>> @@ -1643,7 +1652,7 @@ int __break_lease(struct inode *inode, unsigned 
>>> int flags)
>>>  out:
>>>  	spin_unlock(&ctx->flc_lock);
>>>  	percpu_up_read(&file_rwsem);
>>> -	locks_dispose_list(&dispose);
>>> +	lease_dispose_list(&dispose);
>>>  free_lock:
>>>  	locks_free_lease(new_fl);
>>>  	return error;
>>> @@ -1726,7 +1735,7 @@ static int __fcntl_getlease(struct file *filp, 
>>> unsigned int flavor)
>>>  		spin_unlock(&ctx->flc_lock);
>>>  		percpu_up_read(&file_rwsem);
>>>
>>> -		locks_dispose_list(&dispose);
>>> +		lease_dispose_list(&dispose);
>>>  	}
>>>  	return type;
>>>  }
>>> @@ -1895,7 +1904,7 @@ generic_add_lease(struct file *filp, int arg, 
>>> struct file_lease **flp, void **pr
>>>  out:
>>>  	spin_unlock(&ctx->flc_lock);
>>>  	percpu_up_read(&file_rwsem);
>>> -	locks_dispose_list(&dispose);
>>> +	lease_dispose_list(&dispose);
>>>  	if (is_deleg)
>>>  		inode_unlock(inode);
>>>  	if (!error && !my_fl)
>>> @@ -1931,7 +1940,7 @@ static int generic_delete_lease(struct file 
>>> *filp, void *owner)
>>>  		error = fl->fl_lmops->lm_change(victim, F_UNLCK, &dispose);
>>>  	spin_unlock(&ctx->flc_lock);
>>>  	percpu_up_read(&file_rwsem);
>>> -	locks_dispose_list(&dispose);
>>> +	lease_dispose_list(&dispose);
>>>  	return error;
>>>  }
>>>
>>> @@ -2726,7 +2735,7 @@ locks_remove_lease(struct file *filp, struct 
>>> file_lock_context *ctx)
>>>  	spin_unlock(&ctx->flc_lock);
>>>  	percpu_up_read(&file_rwsem);
>>>
>>> -	locks_dispose_list(&dispose);
>>> +	lease_dispose_list(&dispose);
>>>  }
>>>
>>>  /*
>>>
>>> -- 
>>> 2.52.0
> 


-- 
Chuck Lever

Re: [PATCH 2/2] filelock: allow lease_managers to dictate what qualifies as a conflict

[Jeff Layton]

On Wed, 2025-12-03 at 14:00 -0500, Chuck Lever wrote:
> 
> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> > Requesting a delegation on a file from the userland fcntl() interface
> > currently succeeds when there are conflicting opens present.
> > 
> > This is because the lease handling code ignores conflicting opens for
> > FL_LAYOUT and FL_DELEG leases. This was a hack put in place long ago,
> > because nfsd already checks for conflicts in its own way. The kernel
> > needs to perform this check for userland delegations the same way it is
> > done for leases, however.
> > 
> > Make this dependent on the lease_manager by adding a new
> > ->lm_open_conflict() lease_manager operation and have
> > generic_add_lease() call that instead of check_conflicting_open().
> > Morph check_conflicting_open() into a ->lm_open_conflict() op that is
> > only called for userland leases/delegations. Set the
> > ->lm_open_conflict() operations for nfsd to trivial functions that
> > always return 0.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  Documentation/filesystems/locking.rst |  1 +
> >  fs/locks.c                            | 90 ++++++++++++++++-------------------
> >  fs/nfsd/nfs4layouts.c                 | 11 ++++-
> >  fs/nfsd/nfs4state.c                   |  7 +++
> >  include/linux/filelock.h              |  1 +
> >  5 files changed, 60 insertions(+), 50 deletions(-)
> > 
> > diff --git a/Documentation/filesystems/locking.rst 
> > b/Documentation/filesystems/locking.rst
> > index 
> > 77704fde98457423beae7ff00525a7383e37132b..29d453a2201bcafa03b26b706e4c68eaf5683829 
> > 100644
> > --- a/Documentation/filesystems/locking.rst
> > +++ b/Documentation/filesystems/locking.rst
> > @@ -416,6 +416,7 @@ lm_change		yes		no			no
> >  lm_breaker_owns_lease:	yes     	no			no
> >  lm_lock_expirable	yes		no			no
> >  lm_expire_lock		no		no			yes
> > +lm_open_conflict        yes             no                      no
> >  ======================	=============	=================	=========
> > 
> >  buffer_head
> > diff --git a/fs/locks.c b/fs/locks.c
> > index 
> > e974f8e180fe48682a271af4f143e6bc8e9c4d3b..a58c51c2cdd0cc4496538ed54d063cd523264128 
> > 100644
> > --- a/fs/locks.c
> > +++ b/fs/locks.c
> > @@ -585,10 +585,50 @@ lease_setup(struct file_lease *fl, void **priv)
> >  	__f_setown(filp, task_pid(current), PIDTYPE_TGID, 0);
> >  }
> > 
> > +/**
> > + * lease_open_conflict - see if the given file points to an inode that has
> > + *			 an existing open that would conflict with the
> > + *			 desired lease.
> > + * @filp:	file to check
> > + * @arg:	type of lease that we're trying to acquire
> > + *
> > + * Check to see if there's an existing open fd on this file that would
> > + * conflict with the lease we're trying to set.
> > + */
> > +static int
> > +lease_open_conflict(struct file *filp, const int arg)
> > +{
> > +	struct inode *inode = file_inode(filp);
> > +	int self_wcount = 0, self_rcount = 0;
> > +
> > +	if (arg == F_RDLCK)
> > +		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> > +	else if (arg != F_WRLCK)
> > +		return 0;
> > +
> > +	/*
> > +	 * Make sure that only read/write count is from lease requestor.
> > +	 * Note that this will result in denying write leases when i_writecount
> > +	 * is negative, which is what we want.  (We shouldn't grant write leases
> > +	 * on files open for execution.)
> > +	 */
> > +	if (filp->f_mode & FMODE_WRITE)
> > +		self_wcount = 1;
> > +	else if (filp->f_mode & FMODE_READ)
> > +		self_rcount = 1;
> > +
> > +	if (atomic_read(&inode->i_writecount) != self_wcount ||
> > +	    atomic_read(&inode->i_readcount) != self_rcount)
> > +		return -EAGAIN;
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct lease_manager_operations lease_manager_ops = {
> >  	.lm_break = lease_break_callback,
> >  	.lm_change = lease_modify,
> >  	.lm_setup = lease_setup,
> > +	.lm_open_conflict = lease_open_conflict,
> >  };
> > 
> >  /*
> > @@ -1753,52 +1793,6 @@ int fcntl_getdeleg(struct file *filp, struct 
> > delegation *deleg)
> >  	return 0;
> >  }
> > 
> > -/**
> > - * check_conflicting_open - see if the given file points to an inode 
> > that has
> > - *			    an existing open that would conflict with the
> > - *			    desired lease.
> > - * @filp:	file to check
> > - * @arg:	type of lease that we're trying to acquire
> > - * @flags:	current lock flags
> > - *
> > - * Check to see if there's an existing open fd on this file that would
> > - * conflict with the lease we're trying to set.
> > - */
> > -static int
> > -check_conflicting_open(struct file *filp, const int arg, int flags)
> > -{
> > -	struct inode *inode = file_inode(filp);
> > -	int self_wcount = 0, self_rcount = 0;
> > -
> > -	if (flags & FL_LAYOUT)
> > -		return 0;
> > -	if (flags & FL_DELEG)
> > -		/* We leave these checks to the caller */
> > -		return 0;
> > -
> > -	if (arg == F_RDLCK)
> > -		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> > -	else if (arg != F_WRLCK)
> > -		return 0;
> > -
> > -	/*
> > -	 * Make sure that only read/write count is from lease requestor.
> > -	 * Note that this will result in denying write leases when 
> > i_writecount
> > -	 * is negative, which is what we want.  (We shouldn't grant write 
> > leases
> > -	 * on files open for execution.)
> > -	 */
> > -	if (filp->f_mode & FMODE_WRITE)
> > -		self_wcount = 1;
> > -	else if (filp->f_mode & FMODE_READ)
> > -		self_rcount = 1;
> > -
> > -	if (atomic_read(&inode->i_writecount) != self_wcount ||
> > -	    atomic_read(&inode->i_readcount) != self_rcount)
> > -		return -EAGAIN;
> > -
> > -	return 0;
> > -}
> > -
> >  static int
> >  generic_add_lease(struct file *filp, int arg, struct file_lease **flp, 
> > void **priv)
> >  {
> > @@ -1835,7 +1829,7 @@ generic_add_lease(struct file *filp, int arg, 
> > struct file_lease **flp, void **pr
> >  	percpu_down_read(&file_rwsem);
> >  	spin_lock(&ctx->flc_lock);
> >  	time_out_leases(inode, &dispose);
> > -	error = check_conflicting_open(filp, arg, lease->c.flc_flags);
> > +	error = lease->fl_lmops->lm_open_conflict(filp, arg);
> >  	if (error)
> >  		goto out;
> > 
> > @@ -1892,7 +1886,7 @@ generic_add_lease(struct file *filp, int arg, 
> > struct file_lease **flp, void **pr
> >  	 * precedes these checks.
> >  	 */
> >  	smp_mb();
> > -	error = check_conflicting_open(filp, arg, lease->c.flc_flags);
> > +	error = lease->fl_lmops->lm_open_conflict(filp, arg);
> >  	if (error) {
> >  		locks_unlink_lock_ctx(&lease->c);
> >  		goto out;
> > diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> > index 
> > 683bd1130afe298f9df774684192c89f68102b72..ca7ec7a022bd5c12fad60ff9e51145d9cca55527 
> > 100644
> > --- a/fs/nfsd/nfs4layouts.c
> > +++ b/fs/nfsd/nfs4layouts.c
> > @@ -764,9 +764,16 @@ nfsd4_layout_lm_change(struct file_lease *onlist, 
> > int arg,
> >  	return lease_modify(onlist, arg, dispose);
> >  }
> > 
> > +static int
> > +nfsd4_layout_lm_open_conflict(struct file *filp, int arg)
> > +{
> > +	return 0;
> > +}
> > +
> 
> The usual idiom for no-op callbacks is to make them optional.
> Then generic_add_lease would check if the ->lm_open_conflict
> callback is defined first and skip the call if it's not.
> 

That is what we usually do, but there are only a few lease managers and
they all need to define this op, so it saves us a trivial pointer check
to not do that. I can switch to doing it that way if you have a
preference.

> If that doesn't make sense to do, and these NFSD-specific
> functions need to remain, then our usual practice is to add
> a kdoc comment for both of the new functions that looks like
> the one you added above for lease_open_conflict().
> 

Even for one that just returns 0? Ok

> 
> Otherwise, I'm comfortable that this change fits in with the
> deadlock prevention patches we are considering for NFSD.
> 
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> 
> For both 1/2 and 2/2.
> 

Thanks!

> 
> >  static const struct lease_manager_operations nfsd4_layouts_lm_ops = {
> > -	.lm_break	= nfsd4_layout_lm_break,
> > -	.lm_change	= nfsd4_layout_lm_change,
> > +	.lm_break		= nfsd4_layout_lm_break,
> > +	.lm_change		= nfsd4_layout_lm_change,
> > +	.lm_open_conflict	= nfsd4_layout_lm_open_conflict,
> >  };
> > 
> >  int
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 
> > 8f8c9385101e15b64883eabec71775f26b14f890..669fabb095407e61525e5b71268cf1f06fc09877 
> > 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -5543,10 +5543,17 @@ nfsd_change_deleg_cb(struct file_lease *onlist, 
> > int arg,
> >  		return -EAGAIN;
> >  }
> > 
> > +static int
> > +nfsd4_deleg_lm_open_conflict(struct file *filp, int arg)
> > +{
> > +	return 0;
> > +}
> > +
> >  static const struct lease_manager_operations nfsd_lease_mng_ops = {
> >  	.lm_breaker_owns_lease = nfsd_breaker_owns_lease,
> >  	.lm_break = nfsd_break_deleg_cb,
> >  	.lm_change = nfsd_change_deleg_cb,
> > +	.lm_open_conflict = nfsd4_deleg_lm_open_conflict,
> >  };
> > 
> >  static __be32 nfsd4_check_seqid(struct nfsd4_compound_state *cstate, 
> > struct nfs4_stateowner *so, u32 seqid)
> > diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> > index 
> > 54b824c05299261e6bd6acc4175cb277ea35b35d..2f5e5588ee0733c200103801d0d2ba19bebbf9af 
> > 100644
> > --- a/include/linux/filelock.h
> > +++ b/include/linux/filelock.h
> > @@ -49,6 +49,7 @@ struct lease_manager_operations {
> >  	int (*lm_change)(struct file_lease *, int, struct list_head *);
> >  	void (*lm_setup)(struct file_lease *, void **);
> >  	bool (*lm_breaker_owns_lease)(struct file_lease *);
> > +	int (*lm_open_conflict)(struct file *, int);
> >  };
> > 
> >  struct lock_manager {
> > 
> > -- 
> > 2.52.0

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 1/2] filelock: add lease_dispose_list() helper

[NeilBrown]

On Thu, 04 Dec 2025, Chuck Lever wrote:
> 
> On Mon, Dec 1, 2025, at 10:08 AM, Jeff Layton wrote:
> > ...and call that from the lease handling code instead of
> > locks_dispose_list(). Remove the lease handling parts from
> > locks_dispose_list().
> 
> The actual change here isn't bothering me, but I'm having trouble
> understanding why it's needed. It doesn't appear to be a strict
> functional prerequisite for 2/2.

This was almost exactly my thought too.  The commit message should say
*why* the change is being made and this one just left us guessing.
But I *do* like the change and would rather it were kept in the series,
but with a simple addition to the commit message saying that is a
simplification that isn't strictly necessary.

Thanks,
NeilBrown