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

[Dai Ngo <dai.ngo@oracle.com>]

On 11/17/25 10:21 AM, Jeff Layton wrote:
> 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.

I just try to minimize the effect of the change to FL_LAYOUT, but if
you think that is not necessary then I will remove the case in v5.

>
>>   
>>   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.

I have to think more about this. Without the flc_wait_for_dispose flag,
I don't have a way to allow one thread to proceed to do the fencing
while the rest have to wait until the fencing is done. Do you have any
suggestion?

-Dai

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