Received-SPF: pass (google.com: domain of linux-nfs+bounces-2767-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Benjamin Coddington <bcodding@redhat.com>
To: Dan Aloni <dan.aloni@vastdata.com>
Cc: linux-nfs@vger.kernel.org, Chuck Lever <chuck.lever@oracle.com>
Subject: Re: mount options not propagating to NFSACL and NSM RPC clients
Date: Thu, 11 Apr 2024 14:20:43 -0400
Message-ID: <34B155CC-6E09-4C53-9CC0-3B9F0D2C97B9@redhat.com>
In-Reply-To: <20240411163628.jtnhu3pxgcskvel6@gmail.com>
References: <20231105154857.ryakhmgaptq3hb6b@gmail.com>
 <80B8993C-645D-4748-93B3-88415E165B87@redhat.com>
 <20231129132034.lz3hag5xy2oaojwq@gmail.com>
 <8FDECCA5-80E0-4CB4-B790-4039102916F0@redhat.com>
 <20240410143944.srhfeq6owfvdxcci@gmail.com>
 <5D6491EA-53CF-488C-B1D6-A77A8CDFFDC8@redhat.com>
 <20240411163628.jtnhu3pxgcskvel6@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On 11 Apr 2024, at 12:36, Dan Aloni wrote:

> On 2024-04-11 11:20:14, Benjamin Coddington wrote:
>>> So looks like the request is not being retransmitted. Just to be sure=
,
>>> if I cause the nfsd to drop the regular NFS3 prog I/Os like ACCESS an=
d
>>> LOOKUP, I only get the expected 5 seconds delay following a successfu=
l
>>> retry.
>>>
>>> Seems we only have an issue with the NFS3ACL prog.
>>
>> It looks like the client_acl program gets created with
>> rpc_bind_new_program() which doesn't setup the timeouts/retry strategy=
, and
>> there's nothing after the setup to do it either.
>>
>> I think the problem has existed since 331702337f2b2..  I think this sh=
ould
>> fix it up, would you like to test it?
>
> Please allow me to propose a different change, which I already tested.
>
> Looks to me that the 2012 change that I mentioned below is actually whe=
n
> the problem started happening, when the `kmemdup` call was removed, so
> since then we are simply just missing a field copy, and we are taking
> the timeout from transport-based default timeout globals instead.
>
> Also, I have a hunch that we need to do something different, because of=

> the following code in `rpc_new_client`:
>
>     clnt->cl_rtt =3D &clnt->cl_rtt_default;
>     rpc_init_rtt(&clnt->cl_rtt_default, clnt->cl_timeout->to_initval);
>
> Only setting `clnt->cl_timeout` _after_ client clone does not seem to b=
e
> right if there are `cl_rtt_default` calculations that depend on it. So
> may as well need to call `rpc_init_rtt` too.
>
> --
>
> From 55737f82a9bb3e490836d10491995c8082ebcf11 Mon Sep 17 00:00:00 2001
> From: Dan Aloni <dan.aloni@vastdata.com>
> Date: Thu, 11 Apr 2024 18:30:56 +0300
> Subject: [PATCH] sunrpc: fix NFSACL RPC retry on soft mount
>
> It used to be quite awhile ago since 1b63a75180c6 ('SUNRPC: Refactor
> rpc_clone_client()'), in 2012, that `cl_timeout` was copied in so that
> all mount parameters propagate to NFSACL clients. However since that
> change, if mount options as follows are given:
>
>     soft,timeo=3D50,retrans=3D16,vers=3D3
>
> The resultant NFSACL client receives:
>
>     cl_softrtry: 1
>     cl_timeout: to_initval=3D60000, to_maxval=3D60000, to_increment=3D0=
, to_retries=3D2, to_exponential=3D0
>
> These values lead to NFSACL operations not being retried under the
> condition of transient network outages with soft mount. Instead, getacl=

> call fails after 60 seconds with EIO.
>
> The simple fix is to copy `cl_timeout` and make sure `cl_rtt_default`
> is initialized from it.

Ah, good catch, and I think this means that the normal IO client
(server->client) hasn't been properly setting up its RTT estimator as wel=
l?

> Cc: Chuck Lever <chuck.lever@oracle.com>
> Cc: Benjamin Coddington <bcodding@redhat.com>
> Link: https://lore.kernel.org/all/20231105154857.ryakhmgaptq3hb6b@gmail=
=2Ecom/T/
> Fixes: 1b63a75180c6 ('SUNRPC: Refactor rpc_clone_client()')
> Signed-off-by: Dan Aloni <dan.aloni@vastdata.com>
> ---
>  net/sunrpc/clnt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index cda0935a68c9..75faf1f05a14 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -669,6 +669,9 @@ static struct rpc_clnt *__rpc_clone_client(struct r=
pc_create_args *args,
>  	new->cl_chatty =3D clnt->cl_chatty;
>  	new->cl_principal =3D clnt->cl_principal;
>  	new->cl_max_connect =3D clnt->cl_max_connect;
> +	new->cl_timeout =3D clnt->cl_timeout;
> +	rpc_init_rtt(&clnt->cl_rtt_default, clnt->cl_timeout->to_initval);
> +

So we'll end up doing rpc_init_rtt twice, first in rpc_new_client then he=
re.
Maybe we should be passing cl_timeout on .timeout from the parent in the
rpc_clone_client()'s rpc_create_args?

=2E. anyway - I'll stop "helping", you've clearly got this in hand.  :)

Ben