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 7:47 AM, Chuck Lever wrote:
> On 11/15/25 2:16 PM, 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) {
> The file_lock_context structure is allocated via kmem_cache_alloc() from
> flctx_cache, and it has a NULL constructor. This means newly allocated
> structures contain garbage/stale data.
>
> How are you certain that ctx->flc_conflict has been initialized (ie,
> does not contain a garbage/unknown value) ?

will fix in v5.

>
>
>> +		ctx->flc_conflict = true;
>> +		ctx->flc_wait_for_dispose = false;
>> +	}
>>   
>>   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);
> Nit: scripts/checkpatch.pl might prefer "!ctx->flc_wait_for_dispose"
> instead. Also, it wants the continuation line to align with the open
> parenthesis. Ie:
>
> 	wait_event_interruptible(ctx->flc_dispose_wait,
> 				 !ctx->flc_wait_for_dispose);

will fix in v5.

>
> Notice that if ctx->flc_conflict has a garbage true value above, that
> leaves flc_wait_for_dispose unintialized as well, and this wait could
> become indefinite.

will fix in v5.

-Dai

>
>
>> +			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;
>> +	wait_queue_head_t	flc_dispose_wait;
>>   };
>>   
>>   #ifdef CONFIG_FILE_LOCKING
>