Re: [PATCH v5 02/17] filelock: rework the __break_lease API to use flags

On Wed 05-11-25 11:53:48, Jeff Layton wrote:
> Currently __break_lease takes both a type and an openmode. With the
> addition of directory leases, that makes less sense. Declare a set of
> LEASE_BREAK_* flags that can be used to control how lease breaks work
> instead of requiring a type and an openmode.
> 
> Signed-off-by: Jeff Layton <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/locks.c               | 29 +++++++++++++++++----------
>  include/linux/filelock.h | 52 +++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 56 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index b33c327c21dcd49341fbeac47caeb72cdf7455db..3cdd84a0fbedc9bd1b47725a9cf963342aafbce9 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1529,24 +1529,31 @@ any_leases_conflict(struct inode *inode, struct file_lease *breaker)
>  /**
>   *	__break_lease	-	revoke all outstanding leases on file
>   *	@inode: the inode of the file to return
> - *	@mode: O_RDONLY: break only write leases; O_WRONLY or O_RDWR:
> - *	    break all leases
> - *	@type: FL_LEASE: break leases and delegations; FL_DELEG: break
> - *	    only delegations
> + *	@flags: LEASE_BREAK_* flags
>   *
>   *	break_lease (inlined for speed) has checked there already is at least
>   *	some kind of lock (maybe a lease) on this file.  Leases are broken on
> - *	a call to open() or truncate().  This function can sleep unless you
> - *	specified %O_NONBLOCK to your open().
> + *	a call to open() or truncate().  This function can block waiting for the
> + *	lease break unless you specify LEASE_BREAK_NONBLOCK.
>   */
> -int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
> +int __break_lease(struct inode *inode, unsigned int flags)
>  {
> -	int error = 0;
> -	struct file_lock_context *ctx;
>  	struct file_lease *new_fl, *fl, *tmp;
> +	struct file_lock_context *ctx;
>  	unsigned long break_time;
> -	int want_write = (mode & O_ACCMODE) != O_RDONLY;
> +	unsigned int type;
>  	LIST_HEAD(dispose);
> +	bool want_write = !(flags & LEASE_BREAK_OPEN_RDONLY);
> +	int error = 0;
> +
> +	if (flags & LEASE_BREAK_LEASE)
> +		type = FL_LEASE;
> +	else if (flags & LEASE_BREAK_DELEG)
> +		type = FL_DELEG;
> +	else if (flags & LEASE_BREAK_LAYOUT)
> +		type = FL_LAYOUT;
> +	else
> +		return -EINVAL;
>  
>  	new_fl = lease_alloc(NULL, type, want_write ? F_WRLCK : F_RDLCK);
>  	if (IS_ERR(new_fl))
> @@ -1595,7 +1602,7 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
>  	if (list_empty(&ctx->flc_lease))
>  		goto out;
>  
> -	if (mode & O_NONBLOCK) {
> +	if (flags & LEASE_BREAK_NONBLOCK) {
>  		trace_break_lease_noblock(inode, new_fl);
>  		error = -EWOULDBLOCK;
>  		goto out;
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index c2ce8ba05d068b451ecf8f513b7e532819a29944..47da6aa28d8dc9122618d02c6608deda0f3c4d3e 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -212,7 +212,14 @@ int locks_lock_inode_wait(struct inode *inode, struct file_lock *fl);
>  void locks_init_lease(struct file_lease *);
>  void locks_free_lease(struct file_lease *fl);
>  struct file_lease *locks_alloc_lease(void);
> -int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
> +
> +#define LEASE_BREAK_LEASE		BIT(0)	// break leases and delegations
> +#define LEASE_BREAK_DELEG		BIT(1)	// break delegations only
> +#define LEASE_BREAK_LAYOUT		BIT(2)	// break layouts only
> +#define LEASE_BREAK_NONBLOCK		BIT(3)	// non-blocking break
> +#define LEASE_BREAK_OPEN_RDONLY		BIT(4)	// readonly open event
> +
> +int __break_lease(struct inode *inode, unsigned int flags);
>  void lease_get_mtime(struct inode *, struct timespec64 *time);
>  int generic_setlease(struct file *, int, struct file_lease **, void **priv);
>  int kernel_setlease(struct file *, int, struct file_lease **, void **);
> @@ -367,7 +374,7 @@ static inline int locks_lock_inode_wait(struct inode *inode, struct file_lock *f
>  	return -ENOLCK;
>  }
>  
> -static inline int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
> +static inline int __break_lease(struct inode *inode, unsigned int flags)
>  {
>  	return 0;
>  }
> @@ -428,6 +435,17 @@ static inline int locks_lock_file_wait(struct file *filp, struct file_lock *fl)
>  }
>  
>  #ifdef CONFIG_FILE_LOCKING
> +static inline unsigned int openmode_to_lease_flags(unsigned int mode)
> +{
> +	unsigned int flags = 0;
> +
> +	if ((mode & O_ACCMODE) == O_RDONLY)
> +		flags |= LEASE_BREAK_OPEN_RDONLY;
> +	if (mode & O_NONBLOCK)
> +		flags |= LEASE_BREAK_NONBLOCK;
> +	return flags;
> +}
> +
>  static inline int break_lease(struct inode *inode, unsigned int mode)
>  {
>  	struct file_lock_context *flctx;
> @@ -443,11 +461,11 @@ static inline int break_lease(struct inode *inode, unsigned int mode)
>  		return 0;
>  	smp_mb();
>  	if (!list_empty_careful(&flctx->flc_lease))
> -		return __break_lease(inode, mode, FL_LEASE);
> +		return __break_lease(inode, LEASE_BREAK_LEASE | openmode_to_lease_flags(mode));
>  	return 0;
>  }
>  
> -static inline int break_deleg(struct inode *inode, unsigned int mode)
> +static inline int break_deleg(struct inode *inode, unsigned int flags)
>  {
>  	struct file_lock_context *flctx;
>  
> @@ -461,8 +479,10 @@ static inline int break_deleg(struct inode *inode, unsigned int mode)
>  	if (!flctx)
>  		return 0;
>  	smp_mb();
> -	if (!list_empty_careful(&flctx->flc_lease))
> -		return __break_lease(inode, mode, FL_DELEG);
> +	if (!list_empty_careful(&flctx->flc_lease)) {
> +		flags |= LEASE_BREAK_DELEG;
> +		return __break_lease(inode, flags);
> +	}
>  	return 0;
>  }
>  
> @@ -470,7 +490,7 @@ static inline int try_break_deleg(struct inode *inode, struct inode **delegated_
>  {
>  	int ret;
>  
> -	ret = break_deleg(inode, O_WRONLY|O_NONBLOCK);
> +	ret = break_deleg(inode, LEASE_BREAK_NONBLOCK);
>  	if (ret == -EWOULDBLOCK && delegated_inode) {
>  		*delegated_inode = inode;
>  		ihold(inode);
> @@ -482,7 +502,7 @@ static inline int break_deleg_wait(struct inode **delegated_inode)
>  {
>  	int ret;
>  
> -	ret = break_deleg(*delegated_inode, O_WRONLY);
> +	ret = break_deleg(*delegated_inode, 0);
>  	iput(*delegated_inode);
>  	*delegated_inode = NULL;
>  	return ret;
> @@ -491,20 +511,24 @@ static inline int break_deleg_wait(struct inode **delegated_inode)
>  static inline int break_layout(struct inode *inode, bool wait)
>  {
>  	smp_mb();
> -	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease))
> -		return __break_lease(inode,
> -				wait ? O_WRONLY : O_WRONLY | O_NONBLOCK,
> -				FL_LAYOUT);
> +	if (inode->i_flctx && !list_empty_careful(&inode->i_flctx->flc_lease)) {
> +		unsigned int flags = LEASE_BREAK_LAYOUT;
> +
> +		if (!wait)
> +			flags |= LEASE_BREAK_NONBLOCK;
> +
> +		return __break_lease(inode, flags);
> +	}
>  	return 0;
>  }
>  
>  #else /* !CONFIG_FILE_LOCKING */
> -static inline int break_lease(struct inode *inode, unsigned int mode)
> +static inline int break_lease(struct inode *inode, bool wait)
>  {
>  	return 0;
>  }
>  
> -static inline int break_deleg(struct inode *inode, unsigned int mode)
> +static inline int break_deleg(struct inode *inode, unsigned int flags)
>  {
>  	return 0;
>  }
> 
> -- 
> 2.51.1
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR