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

[Jeff Layton <jlayton@kernel.org>]

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.

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

-- 
Jeff Layton <jlayton@kernel.org>