Re: [PATCH v4 2/3] locks: Threads with layout conflict must wait until client was fenced.

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2025-11-15 at 11:16 -0800, Dai Ngo wrote:
> If multiple threads are waiting for a layout conflict on the same
> file in __break_lease, these threads must wait until one of the
> waiting threads completes the fencing operation before proceeding.
> This ensures that I/O operations from these threads can only occurs
> after the client was fenced.
> 
> Fixes: f99d4fbdae67 ("nfsd: add SCSI layout support")
> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/locks.c               | 24 ++++++++++++++++++++++++
>  include/linux/filelock.h |  5 +++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 1f254e0cd398..b6fd6aa2498c 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -191,6 +191,7 @@ locks_get_lock_context(struct inode *inode, int type)
>  	INIT_LIST_HEAD(&ctx->flc_flock);
>  	INIT_LIST_HEAD(&ctx->flc_posix);
>  	INIT_LIST_HEAD(&ctx->flc_lease);
> +	init_waitqueue_head(&ctx->flc_dispose_wait);
>  
>  	/*
>  	 * Assign the pointer if it's not already assigned. If it is, then
> @@ -1609,6 +1610,10 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
>  		error = -EWOULDBLOCK;
>  		goto out;
>  	}
> +	if (type == FL_LAYOUT && !ctx->flc_conflict) {
> +		ctx->flc_conflict = true;
> +		ctx->flc_wait_for_dispose = false;
> +	}

I don't like special casing this for FL_LAYOUT leases. It seems like we
ought to be able to set up a lm_breaker_timedout operation on any sort
of lease.

>  
>  restart:
>  	fl = list_first_entry(&ctx->flc_lease, struct file_lease, c.flc_list);
> @@ -1640,12 +1645,31 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
>  			time_out_leases(inode, &dispose);
>  		if (any_leases_conflict(inode, new_fl))
>  			goto restart;
> +		if (type == FL_LAYOUT && ctx->flc_wait_for_dispose) {
> +			/*
> +			 * wait for flc_wait_for_dispose to ensure
> +			 * the offending client has been fenced.
> +			 */
> +			spin_unlock(&ctx->flc_lock);
> +			wait_event_interruptible(ctx->flc_dispose_wait,
> +				ctx->flc_wait_for_dispose == false);
> +			spin_lock(&ctx->flc_lock);
> +		}
>  		error = 0;
> +		if (type == FL_LAYOUT)
> +			ctx->flc_wait_for_dispose = true;
>  	}
>  out:
>  	spin_unlock(&ctx->flc_lock);
>  	percpu_up_read(&file_rwsem);
>  	locks_dispose_list(&dispose);
> +	if (type == FL_LAYOUT) {
> +		spin_lock(&ctx->flc_lock);
> +		ctx->flc_wait_for_dispose = false;
> +		ctx->flc_conflict = false;
> +		wake_up(&ctx->flc_dispose_wait);
> +		spin_unlock(&ctx->flc_lock);
> +	}
>  free_lock:
>  	locks_free_lease(new_fl);
>  	return error;
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index 06ccd6b66012..5c5353aabbc8 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -146,6 +146,11 @@ struct file_lock_context {
>  	struct list_head	flc_flock;
>  	struct list_head	flc_posix;
>  	struct list_head	flc_lease;
> +
> +	/* for FL_LAYOUT */
> +	bool			flc_conflict;
> +	bool			flc_wait_for_dispose;

I'm also not a fan of this particular bool. Waiting for any
lm_breaker_timeout operations to complete seems like something we ought
to just always do. In the trivial case where we have no special fencing
to do, that should just return quickly anyway.

> +	wait_queue_head_t	flc_dispose_wait;
>  };
>  
>  #ifdef CONFIG_FILE_LOCKING

-- 
Jeff Layton <jlayton@kernel.org>