Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals

[Trond Myklebust <trondmy@hammerspace.com>]

On Wed, 2020-01-22 at 19:10 +0000, Schumaker, Anna wrote:
> Hi Robert,
> 
> On Mon, 2020-01-20 at 17:55 +0000, Robert Milkowski wrote:
> > > -----Original Message-----
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > Sent: 30 December 2019 15:37
> > > To: Robert Milkowski <rmilkowski@gmail.com>
> > > Cc: Linux NFS Mailing List <linux-nfs@vger.kernel.org>; Trond
> > > Myklebust
> > > <trond.myklebust@hammerspace.com>; Anna Schumaker
> > > <anna.schumaker@netapp.com>; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do
> > > implicit
> > > lease renewals
> > > 
> > > 
> > > 
> > > > On Dec 30, 2019, at 10:20 AM, Robert Milkowski <
> > > > rmilkowski@gmail.com>
> > > wrote:
> > > > From: Robert Milkowski <rmilkowski@gmail.com>
> > > > 
> > > > Currently, each time nfs4_do_fsinfo() is called it will do an
> > > > implicit
> > > > NFS4 lease renewal, which is not compliant with the NFS4
> > > specification.
> > > > This can result in a lease being expired by an NFS server.
> > > > 
> > > > Commit 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing
> > > > leases")
> > > > introduced implicit client lease renewal in nfs4_do_fsinfo(),
> > > > which
> > > > can result in the NFSv4.0 lease to expire on a server side, and
> > > > servers returning NFS4ERR_EXPIRED or NFS4ERR_STALE_CLIENTID.
> > > > 
> > > > This can easily be reproduced by frequently unmounting a sub-
> > > > mount,
> > > > then stat'ing it to get it mounted again, which will delay or
> > > > even
> > > > completely prevent client from sending RENEW operations if no
> > > > other
> > > > NFS operations are issued. Eventually nfs server will expire
> > > > client's
> > > > lease and return an error on file access or next RENEW.
> > > > 
> > > > This can also happen when a sub-mount is automatically
> > > > unmounted due
> > > > to inactivity (after nfs_mountpoint_expiry_timeout), then it is
> > > > mounted again via stat(). This can result in a short window
> > > > during
> > > > which client's lease will expire on a server but not on a
> > > > client.
> > > > This specific case was observed on production systems.
> > > > 
> > > > This patch makes an explicit lease renewal instead of an
> > > > implicit one,
> > > > by adding RENEW to a compound operation issued by
> > > > nfs4_do_fsinfo(),
> > > > similarly to NFSv4.1 which adds SEQUENCE operation.
> > > > 
> > > > Fixes: 83ca7f5ab31f ("NFS: Avoid PUTROOTFH when managing
> > > > leases")
> > > > Signed-off-by: Robert Milkowski <rmilkowski@gmail.com>
> > > 
> > > Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > 
> > 
> > How do we progress it further?
> 
> Thanks for following up! I have the patch included in my linux-next
> branch for
> the next merge window.

NACK. This is the wrong way to solve the problem. Lease renewal in
NFSv4 does not need to be tied to fsinfo. It creates an unnecessary
extra error condition that has absolutely nothing to do with the
functionality of retrieving per-filesystem attributes.

All that needs to be done here is to move the setting of clp-
>cl_last_renewal _out_ of nfs4_set_lease_period(), and just have
nfs4_proc_setclientid_confirm() and nfs4_update_session() call
do_renew_lease().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com