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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH 8/8] sunrpc: Drop the connection when the server drops a
 request
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <32be2fbebcd4e30567f54c830e23a0ef35a4844e.camel@hammerspace.com>
Date:   Wed, 11 Mar 2020 11:26:18 -0400
Cc:     "dwysocha@redhat.com" <dwysocha@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Bruce Fields <bfields@redhat.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <8B37E4E5-1F62-4EA7-8F40-88FBB30C1738@oracle.com>
References: <20200301232145.1465430-1-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-2-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-3-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-4-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-5-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-6-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-7-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-8-trond.myklebust@hammerspace.com>
 <20200301232145.1465430-9-trond.myklebust@hammerspace.com>
 <CALF+zOkJPkYaXjCn-tj0dPCQUAjA05zyzxm5fzwoB-XP0SGYvw@mail.gmail.com>
 <32be2fbebcd4e30567f54c830e23a0ef35a4844e.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Mar 2, 2020, at 3:12 PM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Mon, 2020-03-02 at 11:27 -0500, David Wysochanski wrote:
>> On Sun, Mar 1, 2020 at 6:25 PM Trond Myklebust <trondmy@gmail.com>
>> wrote:
>>> If a server wants to drop a request, then it should also drop the
>>> connection, in order to let the client know.
>>>=20
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>> net/sunrpc/svc_xprt.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>=20
>>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>>> index de3c077733a7..83a527e56c87 100644
>>> --- a/net/sunrpc/svc_xprt.c
>>> +++ b/net/sunrpc/svc_xprt.c
>>> @@ -873,6 +873,13 @@ int svc_recv(struct svc_rqst *rqstp, long
>>> timeout)
>>> }
>>> EXPORT_SYMBOL_GPL(svc_recv);
>>>=20
>>> +static void svc_drop_connection(struct svc_xprt *xprt)
>>> +{
>>> +       if (test_bit(XPT_TEMP, &xprt->xpt_flags) &&
>>> +           !test_and_set_bit(XPT_CLOSE, &xprt->xpt_flags))
>>> +               svc_xprt_enqueue(xprt);
>>> +}
>>> +
>>> /*
>>>  * Drop request
>>>  */
>>> @@ -880,6 +887,8 @@ void svc_drop(struct svc_rqst *rqstp)
>>> {
>>>        trace_svc_drop(rqstp);
>>>        dprintk("svc: xprt %p dropped request\n", rqstp->rq_xprt);
>>> +       /* Close the connection when dropping a request */
>>> +       svc_drop_connection(rqstp->rq_xprt);
>>>        svc_xprt_release(rqstp);
>>> }
>>> EXPORT_SYMBOL_GPL(svc_drop);
>>> @@ -1148,6 +1157,7 @@ static void svc_revisit(struct
>>> cache_deferred_req *dreq, int too_many)
>>>        if (too_many || test_bit(XPT_DEAD, &xprt->xpt_flags)) {
>>>                spin_unlock(&xprt->xpt_lock);
>>>                dprintk("revisit canceled\n");
>>> +               svc_drop_connection(xprt);
>>>                svc_xprt_put(xprt);
>>>                trace_svc_drop_deferred(dr);
>>>                kfree(dr);
>>> --
>>> 2.24.1
>>>=20
>>=20
>> Trond, back in 2014 you had this NFSv4 only patch that took a more
>> surgical approach:
>> https://marc.info/?l=3Dlinux-nfs&m=3D141414531832768&w=3D2
>>=20
>> It looks like discussion died out on it after it was ineffective to
>> solve a different problem.
>> Is there a reason why you don't want to do that approach now?
>>=20
>=20
> Let me resend this patch with a better proposal.

Hey Trond, any progress here?


> I think the main 2
> problems here are really
>=20
>   1. the svc_revisit() case, where we cancel the revisit. That case
>      affects all versions of NFS, and can lead to performance issues.
>   2. the NFSv2,v3,v4.0 replay cache, where dropping the replay (e.g.
>      after a connection breakage) can cause a performance hit, and for
>      something like TCP, which has long (usually 60 second) timeouts =
it
>      could cause the replay to be delayed until after the reply gets
>      kicked out of the cache. This is the case where NFSv4.0 can =
probably
>      end up hanging, since the replay won't be forthcoming until a new
>      connection breakage occurs.
>=20
> I think (1) is best served by a patch like this one.
> Perhaps (2) is better served by adopting the svc_defer() mechanism?
>=20
> Hmm... Perhaps 2 patches are in order...

--
Chuck Lever