Message-Id: <ae795e6b-bf65-46ec-9629-edcec3dcd0b9@app.fastmail.com>
Date: Wed, 03 Dec 2025 14:00:44 -0500
From: "Chuck Lever" <cel@...nel.org>
To: "Jeff Layton" <jlayton@...nel.org>,
 "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Chuck Lever" <chuck.lever@...cle.com>,
 "Alexander Aring" <alex.aring@...il.com>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 "Jonathan Corbet" <corbet@....net>, NeilBrown <neil@...wn.name>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-nfs@...r.kernel.org
Subject: Re: [PATCH 2/2] filelock: allow lease_managers to dictate what qualifies as a
 conflict



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

Powered by blists - more mailing lists