Re: [PATCH v5 1/1] NFSD: Enforce timeout on layout recall and integrate lease manager fencing

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2026-02-06 at 10:17 -0800, Dai Ngo wrote:
> On 2/6/26 6:28 AM, Jeff Layton wrote:
> > On Thu, 2026-02-05 at 12:29 -0800, Dai Ngo wrote:
> > > When a layout conflict triggers a recall, enforcing a timeout is
> > > necessary to prevent excessive nfsd threads from being blocked in
> > > __break_lease ensuring the server continues servicing incoming
> > > requests efficiently.
> > > 
> > > This patch introduces a new function to lease_manager_operations:
> > > 
> > > lm_breaker_timedout: Invoked when a lease recall times out and is
> > > about to be disposed of. This function enables the lease manager
> > > to inform the caller whether the file_lease should remain on the
> > > flc_list or be disposed of.
> > > 
> > > For the NFSD lease manager, this function now handles layout recall
> > > timeouts. If the layout type supports fencing and the client has not
> > > been fenced, a fence operation is triggered to prevent the client
> > > from accessing the block device.
> > > 
> > > While the fencing operation is in progress, the conflicting file_lease
> > > remains on the flc_list until fencing is complete. This guarantees
> > > that no other clients can access the file, and the client with
> > > exclusive access is properly blocked before disposal.
> > > 
> > Fair point. However...
> > 
> > > Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> > > ---
> > >   Documentation/filesystems/locking.rst |   2 +
> > >   fs/locks.c                            |  15 +++-
> > >   fs/nfsd/blocklayout.c                 |  41 ++++++++--
> > >   fs/nfsd/nfs4layouts.c                 | 113 +++++++++++++++++++++++++-
> > >   fs/nfsd/nfs4state.c                   |   1 +
> > >   fs/nfsd/pnfs.h                        |   2 +-
> > >   fs/nfsd/state.h                       |   8 ++
> > >   include/linux/filelock.h              |   1 +
> > >   8 files changed, 169 insertions(+), 14 deletions(-)
> > > 
> > > v2:
> > >      . Update Subject line to include fencing operation.
> > >      . Allow conflicting lease to remain on flc_list until fencing
> > >        is complete.
> > >      . Use system worker to perform fencing operation asynchronously.
> > >      . Use nfs4_stid.sc_count to ensure layout stateid remains
> > >        valid before starting the fencing operation, nfs4_stid.sc_count
> > >        is released after fencing operation is complete.
> > >      . Rework nfsd4_scsi_fence_client to:
> > >           . wait until fencing to complete before exiting.
> > >           . wait until fencing in progress to complete before
> > >             checking the NFSD_MDS_PR_FENCED flag.
> > >      . Remove lm_need_to_retry from lease_manager_operations.
> > > v3:
> > >      . correct locking requirement in locking.rst.
> > >      . add max retry count to fencing operation.
> > >      . add missing nfs4_put_stid in nfsd4_layout_fence_worker.
> > >      . remove special-casing of FL_LAYOUT in lease_modify.
> > >      . remove lease_want_dispose.
> > >      . move lm_breaker_timedout call to time_out_leases.
> > > v4:
> > >      . only increment ls_fence_retry_cnt after successfully
> > >        schedule new work in nfsd4_layout_lm_breaker_timedout.
> > > v5:
> > >      . take reference count on layout stateid before starting
> > >        fence worker.
> > >      . restore comments in nfsd4_scsi_fence_client and the
> > >        code that check for specific errors.
> > >      . cancel fence worker before freeing layout stateid.
> > >      . increase fence retry from 5 to 20.
> > > 
> > > NOTE:
> > >      I experimented with having the fence worker handle lease
> > >      disposal after fencing the client. However, this requires
> > >      the lease code to export the lease_dispose_list function,
> > >      and for the fence worker to acquire the flc_lock in order
> > >      to perform the disposal. This approach adds unnecessary
> > >      complexity and reduces code clarity, as it exposes internal
> > >      lease code details to the nfsd worker, which should not
> > >      be the case.
> > > 
> > >      Instead, the lm_breaker_timedout operation should simply
> > >      notify the lease code about how to handle a lease that
> > >      times out during a lease break, rather than directly
> > >      manipulating the lease list.
> > > 
> > Ok, fair point.
> > 
> > > diff --git a/Documentation/filesystems/locking.rst b/Documentation/filesystems/locking.rst
> > > index 04c7691e50e0..79bee9ae8bc3 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);
> > > +        bool (*lm_breaker_timedout)(struct file_lease *);
> > >   
> > >   locking rules:
> > >   
> > > @@ -417,6 +418,7 @@ lm_breaker_owns_lease:	yes     	no			no
> > >   lm_lock_expirable	yes		no			no
> > >   lm_expire_lock		no		no			yes
> > >   lm_open_conflict	yes		no			no
> > > +lm_breaker_timedout     yes             no                      no
> > >   ======================	=============	=================	=========
> > >   
> > >   buffer_head
> > > diff --git a/fs/locks.c b/fs/locks.c
> > > index 46f229f740c8..0e77423cf000 100644
> > > --- a/fs/locks.c
> > > +++ b/fs/locks.c
> > > @@ -1524,6 +1524,7 @@ static void time_out_leases(struct inode *inode, struct list_head *dispose)
> > >   {
> > >   	struct file_lock_context *ctx = inode->i_flctx;
> > >   	struct file_lease *fl, *tmp;
> > > +	bool remove = true;
> > >   
> > >   	lockdep_assert_held(&ctx->flc_lock);
> > >   
> > > @@ -1531,8 +1532,18 @@ 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))
> > > -			lease_modify(fl, F_UNLCK, dispose);
> > > +
> > > +		if (past_time(fl->fl_break_time)) {
> > > +			/*
> > > +			 * Consult the lease manager when a lease break times
> > > +			 * out to determine whether the lease should be disposed
> > > +			 * of.
> > > +			 */
> > > +			if (fl->fl_lmops && fl->fl_lmops->lm_breaker_timedout)
> > > +				remove = fl->fl_lmops->lm_breaker_timedout(fl);
> > > +			if (remove)
> > > +				lease_modify(fl, F_UNLCK, dispose);
> > When remove is false, and lease_modify() doesn't happen (i.e., the
> > common case where we queue the wq job), when do you actually remove the
> > lease?
> 
> The lease is removed when the fence worker completes the fencing operation
> and set ls_fenced to true. When __break_lease/time_out_leases calls
> lm_breaker_timedout again, nfsd4_layout_lm_breaker_timedout returns true
> since ls_fenced is now set.
> 
> > 
> > Are you just assuming that after the client is fenced, that the layout
> > stateid's refcount will go to zero? I'm curious what drives that
> > process, if so.
> 
> No, after completing the fence operation, the fenced worker drops the
> reference count on the layout stateid by calling nfs4_put_stid(). If
> the reference drops to 0 then the layout stateid is freed at this
> point, otherwise it will be freed when the CB_RECALL callback times
> out.
> 

In principle the stateid could stick around for a while after the fence
has occurred. It would be better to unlock the lease as soon as the
fencing is done, so that tasks waiting on it can proceed (a'la
kernel_setlease() with F_UNLCK).

-- 
Jeff Layton <jlayton@kernel.org>