On Mon, 16 Dec 2019 at 18:58, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> -On Mon, 2019-12-16 at 18:43 +0000, Robert Milkowski wrote:
> > > From: Trond Myklebust <trondmy@xxxxxxxxxxxxxxx>
> > ...
> > > NACK. The above argument only applies to legacy minor version 0
> > > setups, and does not apply to NFSv4.1 or newer.
> >
> > Correct. However many sites still use v4.0.
> >
>
> That's not a good reason to break code that works just fine for
> NFSv4.1.
>
Of course not, that's not what I meant and I misunderstood your nack too.
> It would be better to move the initialisation of clp->cl_last_renewal
> into nfs4_init_clientid() and nfs41_init_clientid() (after the calls to
> nfs4_proc_setclientid_confirm() and nfs4_proc_create_session()
> respectively).
>
This could be done but this is potentially a separate change, as in
nfs4_do_fsinfo() we still need to
make sure we do not implicitly renew lease for v4.0, so I think the
patch needs to be modified as:
...
+ /* no implicit lease renewal allowed here for v4.0 */
+ if (server->nfs_client->cl_minorversion == 0
&& server->nfs_client->cl_last_renewal != 0)
+ last_renewal =
server->nfs_client->cl_last_renewal;
nfs4_set_lease_period(server->nfs_client,
fsinfo->lease_time * HZ,
- now);
+ last_renewal);
...
This way it won't affect newer nfs versions (I don't think it affected
them in any bad way, still it was unintended and not correct).
Agree with the above change?
btw: Chuck, thanks for the hint regarding the SEQUENCE op on v4.1+
