Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <025801d5bf24$aa242100$fe6c6300$@gmail.com> <D82A1590-FAA3-47C5-B198-937ED88EF71C@oracle.com>
 <084f01d5cfba$bc5c4d10$3514e730$@gmail.com> <49e7b99bd1451a0dbb301915f655c73b3d9354df.camel@netapp.com>
 <a62e45ba968fe845fa757174efc0cab93c490d54.camel@hammerspace.com>
 <CALbTx=GxuUmWu9Og_pv8TPbB0ZnOCA3vMqtyG4e18-4+zkY8=A@mail.gmail.com> <197269d91db31284dce51b831bfbe2231ac870d4.camel@hammerspace.com>
In-Reply-To: <197269d91db31284dce51b831bfbe2231ac870d4.camel@hammerspace.com>
From:   Robert Milkowski <rmilkowski@gmail.com>
Date:   Mon, 27 Jan 2020 17:26:16 +0000
Message-ID: <CALbTx=E7Hk4bVtSehjo4TPykTZsP+fSR0zeHFWyqd4XtHDyL4g@mail.gmail.com>
Subject: Re: [PATCH v3] NFSv4.0: nfs4_do_fsinfo() should not do implicit lease renewals
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "Anna.Schumaker@netapp.com" <Anna.Schumaker@netapp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "chuck.lever@oracle.com" <chuck.lever@oracle.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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