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

[Robert Milkowski <rmilkowski@gmail.com>]

> >
> > > 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().
> > >
> >
> > This would also require nfs4_setup_state_renewal() to call
> > do_renew_lease() I think - at least it currently calls
> > nfs4_set_lease_period().
>
> This is the mechanism we're replacing. There is no need for a call to
> do_renew_lease() in nfs4_setup_state_renewal().

ok

>
> > Also, iirc fsinfo() not setting cl_last_renewal leads to
> > cl_last_renewal initialization issues under some circumstances.
> >
> > Then the RFC 7530 in section 16.34.5 states:
> > "SETCLIENTID_CONFIRM does not establish or renew a lease.", so
> > calling
> > do_renew_lease() from nfs4_setclientid_confirm() doesn't seem to be
> > ok.
>
> So just move the do_renew_lease() to nfs4_proc_setclientid(). The
> successful combination SETCLIENTID+SETCLIENTID_CONFIRM definitely
> _does_ establish a lease.
>

ok

> > I'm not sure if is is valid to do implicit lease renewal in
> > nfs4_update_session() either...
>
> Ditto. Move to the exchange_id call.
>

ok

> >
> > Anyway, the patch would be something like (haven't tested it yet):
> >
...
> > @@ -6146,6 +6143,10 @@ int nfs4_proc_setclientid_confirm(struct
> > nfs_client *clp,
> >                 clp->cl_clientid);
> >         status = rpc_call_sync(clp->cl_rpcclient, &msg,
> >                                RPC_TASK_TIMEOUT |
> > RPC_TASK_NO_ROUND_ROBIN);
> > +       if(status == 0) {
> > +               unsigned long now = jiffies;
>
> The variable 'now' needs to be set before the RPC call in order to
> avoid overestimating the remaining lease period.
>

yes, thx.

I will get a new patch tested and submitted here this week.

-- 
Robert Milkowski