Re: [PATCH v4 1/3] locks: Introduce lm_breaker_timedout operation to lease_manager_operations

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

On 11/19/25 5:52 AM, Jeff Layton wrote:
> On Mon, 2025-11-17 at 11:41 -0800, Dai Ngo wrote:
>> On 11/17/25 10:02 AM, Jeff Layton wrote:
>>> On Sat, 2025-11-15 at 11:16 -0800, Dai Ngo wrote:
>>>> Some consumers of the lease_manager_operations structure need
>>>> to perform additional actions when a lease break, triggered by
>>>> a conflict, times out.
>>>>
>>>> The NFS server is the first consumer of this operation.
>>>>
>>>> When a pNFS layout conflict occurs and the lease break times
>>>> out — resulting in the layout being revoked and its file lease
>>>> removed from the flc_lease list — the NFS server must issue a
>>>> fence operation. This operation ensures that the client is
>>>> prevented from accessing the data server after the layout
>>>> revocation.
>>>>
>>>> Fixes: f99d4fbdae67 ("nfsd: add SCSI layout support")
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>    Documentation/filesystems/locking.rst |  2 ++
>>>>    fs/locks.c                            | 14 +++++++++++---
>>>>    include/linux/filelock.h              |  2 ++
>>>>    3 files changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
>>>> index 77704fde9845..cd600db6c4b9 100644
>>>> --- a/Documentation/filesystems/locking.rst
>>>> +++ b/Documentation/filesystems/locking.rst
>>>> @@ -403,6 +403,7 @@ prototypes::
>>>>    	bool (*lm_breaker_owns_lease)(struct file_lock *);
>>>>            bool (*lm_lock_expirable)(struct file_lock *);
>>>>            void (*lm_expire_lock)(void);
>>>> +        void (*lm_breaker_timedout)(struct file_lease *);
>>>>    
>>>>    locking rules:
>>>>    
>>>> @@ -416,6 +417,7 @@ lm_change		yes		no			no
>>>>    lm_breaker_owns_lease:	yes     	no			no
>>>>    lm_lock_expirable	yes		no			no
>>>>    lm_expire_lock		no		no			yes
>>>> +lm_breaker_timedout     no              no                      yes
>>>>    ======================	=============	=================	=========
>>>>    
>>>>    buffer_head
>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>> index 04a3f0e20724..1f254e0cd398 100644
>>>> --- a/fs/locks.c
>>>> +++ b/fs/locks.c
>>>> @@ -369,9 +369,15 @@ locks_dispose_list(struct list_head *dispose)
>>>>    	while (!list_empty(dispose)) {
>>>>    		flc = list_first_entry(dispose, struct file_lock_core, flc_list);
>>>>    		list_del_init(&flc->flc_list);
>>>> -		if (flc->flc_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT))
>>>> +		if (flc->flc_flags & (FL_LEASE|FL_DELEG|FL_LAYOUT)) {
>>>> +			if (flc->flc_flags & FL_BREAKER_TIMEDOUT) {
>>>> +				struct file_lease *fl = file_lease(flc);
>>>> +
>>>> +				if (fl->fl_lmops->lm_breaker_timedout)
>>>> +					fl->fl_lmops->lm_breaker_timedout(fl);
>>>> +			}
>>> locks_dispose_list() is a common function for locks and leases, and
>>> this is only going to be relevant from __break_lease().
>>>
>>> Can you move this handling into a separate function that is called
>>> before the relevant locks_dispose_list() call in __break_lease()?
>> will fix in v5.
>>
>> -Dai
>>
> That may not work actually, since we may end up with a timed out lease
> on a dispose list in a different codepath.
>
> I just sent this patch:
>
>       https://lore.kernel.org/linux-nfs/20251119-dir-deleg-ro-v8-2-81b6cf5485c6@kernel.org/T/#u
>
> Assuming that goes in, then I think calling ->lm_breaker_timedout()
> from lease_dispose_list() should be OK.

Thanks Jeff, I will wait for your patch to go in then rework on this patch.

-Dai

>
>>>>    			locks_free_lease(file_lease(flc));
>>>> -		else
>>>> +		} else
>>>>    			locks_free_lock(file_lock(flc));
>>>>    	}
>>>>    }
>>>> @@ -1482,8 +1488,10 @@ static void time_out_leases(struct inode *inode, struct list_head *dispose)
>>>>    		trace_time_out_leases(inode, fl);
>>>>    		if (past_time(fl->fl_downgrade_time))
>>>>    			lease_modify(fl, F_RDLCK, dispose);
>>>> -		if (past_time(fl->fl_break_time))
>>>> +		if (past_time(fl->fl_break_time)) {
>>>>    			lease_modify(fl, F_UNLCK, dispose);
>>>> +			fl->c.flc_flags |= FL_BREAKER_TIMEDOUT;
>>>> +		}
>>>>    	}
>>>>    }
>>>>    
>>>> diff --git a/include/linux/filelock.h b/include/linux/filelock.h
>>>> index c2ce8ba05d06..06ccd6b66012 100644
>>>> --- a/include/linux/filelock.h
>>>> +++ b/include/linux/filelock.h
>>>> @@ -17,6 +17,7 @@
>>>>    #define FL_OFDLCK	1024	/* lock is "owned" by struct file */
>>>>    #define FL_LAYOUT	2048	/* outstanding pNFS layout */
>>>>    #define FL_RECLAIM	4096	/* reclaiming from a reboot server */
>>>> +#define	FL_BREAKER_TIMEDOUT	8192	/* lease breaker timed out */
>>>>    
>>>>    #define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)
>>>>    
>>>> @@ -49,6 +50,7 @@ struct lease_manager_operations {
>>>>    	int (*lm_change)(struct file_lease *, int, struct list_head *);
>>>>    	void (*lm_setup)(struct file_lease *, void **);
>>>>    	bool (*lm_breaker_owns_lease)(struct file_lease *);
>>>> +	void (*lm_breaker_timedout)(struct file_lease *fl);
>>>>    };
>>>>    
>>>>    struct lock_manager {