Received-SPF: pass (google.com: domain of linux-nfs+bounces-2770-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 11 Apr 2024 22:46:29 +0300
From: Dan Aloni <dan.aloni@vastdata.com>
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "bcodding@redhat.com" <bcodding@redhat.com>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"chuck.lever@oracle.com" <chuck.lever@oracle.com>
Subject: Re: mount options not propagating to NFSACL and NSM RPC clients
Message-ID: <20240411194629.mmrl5nrmkzffwgfq@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>
 <8357f330100e840094b0193dc5e324e1d1b25a7e.camel@hammerspace.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8357f330100e840094b0193dc5e324e1d1b25a7e.camel@hammerspace.com>

On 2024-04-11 19:08:13, Trond Myklebust wrote:
> > 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
> > rpc_create_args *args,
> > ?	new->cl_chatty = clnt->cl_chatty;
> > ?	new->cl_principal = clnt->cl_principal;
> > ?	new->cl_max_connect = clnt->cl_max_connect;
> > +	new->cl_timeout = clnt->cl_timeout;
> > +	rpc_init_rtt(&clnt->cl_rtt_default, clnt->cl_timeout-
> > >to_initval);
> > +
> > ?	return new;
> > 
> 
> That ends up clobbering any timeout value that gets set in the
> rpc_create_args, and open codes something that is?already being done in
> the call to rpc_new_client().
> 
> IOW: I'd much rather see us default the value of args->timeout to that
> of clnt->cl_timeout.

Makes sense. I confirm the following fix works for NFSACL and preserves
the current hard mount behavior for NSM. Other callers to
`rpc_bind_new_program` are unaffected by this.

However I am not sure if restoring this behavior is desired for the
other call sites of `__rpc_clone_client`, which seems to be NFSv4.x
users, so I left that part out. Should that be done in separate patches?


From 8715ef1d574970176e9ac87a7e826ad74f8b910d 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=50,retrans=16,vers=3

The resultant NFSACL client receives:

    cl_softrtry: 1
    cl_timeout: to_initval=60000, to_maxval=60000, to_increment=0, to_retries=2, to_exponential=0

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 pass the existing client's `cl_timeout` as the new
client timeout.

Cc: Chuck Lever <chuck.lever@oracle.com>
Cc: Benjamin Coddington <bcodding@redhat.com>
Cc: Trond Myklebust <trondmy@hammerspace.com>
Link: https://lore.kernel.org/all/20231105154857.ryakhmgaptq3hb6b@gmail.com/T/
Fixes: 1b63a75180c6 ('SUNRPC: Refactor rpc_clone_client()')
Signed-off-by: Dan Aloni <dan.aloni@vastdata.com>
---
 net/sunrpc/clnt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index cda0935a68c9..07ffd4ee695a 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1068,6 +1068,7 @@ struct rpc_clnt *rpc_bind_new_program(struct rpc_clnt *old,
 		.version	= vers,
 		.authflavor	= old->cl_auth->au_flavor,
 		.cred		= old->cl_cred,
+		.timeout	= old->cl_timeout,
 	};
 	struct rpc_clnt *clnt;
 	int err;
-- 
2.39.3


-- 
Dan Aloni