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

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-11-13 at 15:23 -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               | 15 ++++++++++++++-
>  include/linux/filelock.h |  2 ++
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/locks.c b/fs/locks.c
> index 1f254e0cd398..7840108aad71 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1609,6 +1609,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;
> +	}
>  
>  restart:
>  	fl = list_first_entry(&ctx->flc_lease, struct file_lease, c.flc_list);
> @@ -1638,14 +1642,23 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
>  		 */
>  		if (error == 0)
>  			time_out_leases(inode, &dispose);
> -		if (any_leases_conflict(inode, new_fl))
> +		if (any_leases_conflict(inode, new_fl) ||
> +				(type == FL_LAYOUT && ctx->flc_wait_for_dispose))
>  			goto restart;
>  		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;
> +		spin_unlock(&ctx->flc_lock);
> +	}

I think the problem with doing it this way is that the main wait in
this function is here:

        error = wait_event_interruptible_timeout(new_fl->c.flc_wait,
                                                 list_empty(&new_fl->c.flc_blocked_member),
                                                 break_time);

Eventually, the lease will time out, and flc_blocked_member will be
empty, but the offending client won't have been fenced yet. That may
take a while, depending on the mechanism. So, those other threads are
just going to end up spinning here and never sleeping during that
duration.

If you're going to do it this way, then you need to ensure that if
flc_wait_for_dispose is true that other tasks properly wait on it to go
false without spinning.


>  free_lock:
>  	locks_free_lease(new_fl);
>  	return error;
> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
> index 06ccd6b66012..95f489806c61 100644
> --- a/include/linux/filelock.h
> +++ b/include/linux/filelock.h
> @@ -146,6 +146,8 @@ struct file_lock_context {
>  	struct list_head	flc_flock;
>  	struct list_head	flc_posix;
>  	struct list_head	flc_lease;
> +	bool			flc_conflict;
> +	bool			flc_wait_for_dispose;
>  };
>  
>  #ifdef CONFIG_FILE_LOCKING

-- 
Jeff Layton <jlayton@kernel.org>