Hi,
On Linux v6.6-14500-g1c41041124bd, I added a sysfs file for debugging
`/sys/kernel/debug/sunrpc/rpc_clnt/*/info`, and noticed that when
passing the following mount options: `soft,timeo=50,retrans=16,vers=3`,
NFSACL and NSM seem to take the defaults from somewhere else (xprt).
Specifically, locking operation behave as if in a hard mount with
these mount options.
Is it intentional?
cl_prog: 100000
cl_softrtry: 1
cl_timeout: to_initval=10000, to_maxval=10000, to_increment=0, to_retries=2, to_exponential=0
cl_prog: 100003
cl_softrtry: 1
cl_timeout: to_initval=5000, to_maxval=85000, to_increment=5000, to_retries=16, to_exponential=0
cl_prog: 100000
cl_softrtry: 1
cl_timeout: to_initval=10000, to_maxval=10000, to_increment=0, to_retries=2, to_exponential=0
cl_prog: 100021
cl_softrtry: 0
cl_timeout: to_initval=10000, to_maxval=60000, to_increment=10000, to_retries=5, to_exponential=0
cl_prog: 100003
cl_softrtry: 1
cl_timeout: to_initval=5000, to_maxval=85000, to_increment=5000, to_retries=16, to_exponential=0
cl_prog: 100227
cl_softrtry: 1
cl_timeout: to_initval=60000, to_maxval=60000, to_increment=0, to_retries=2, to_exponential=0
Also, here's the patch I used to expose this information:
-- >8 --
Subject: [PATCH] sunrpc: add per-client 'info' node in debugfs
---
net/sunrpc/debugfs.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 75 insertions(+)
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index a176d5a0b0ee..61a2cc01815d 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -142,6 +142,77 @@ static int do_xprt_debugfs(struct rpc_clnt *clnt, struct rpc_xprt *xprt, void *n
return 0;
}
+static int
+clnt_info_show(struct seq_file *seq, void *v)
+{
+ struct rpc_clnt *clnt = seq->private;
+ const struct rpc_timeout *timeout = clnt->cl_timeout;
+
+ seq_printf(seq, "cl_count: %d\n", refcount_read(&clnt->cl_count));
+ seq_printf(seq, "cl_pid: %d\n", atomic_read(&clnt->cl_pid));
+ seq_printf(seq, "cl_prog: %d\n", clnt->cl_prog);
+ seq_printf(seq, "cl_vers: %d\n", clnt->cl_vers);
+ seq_printf(seq, "cl_maxproc: %d\n", clnt->cl_maxproc);
+
+ seq_printf(seq, "cl_softrtry: %d\n", clnt->cl_softrtry);
+ seq_printf(seq, "cl_softerr: %d\n", clnt->cl_softerr);
+ seq_printf(seq, "cl_discrtry: %d\n", clnt->cl_discrtry);
+ seq_printf(seq, "cl_noretranstimeo: %d\n", clnt->cl_noretranstimeo);
+ seq_printf(seq, "cl_autobind: %d\n", clnt->cl_autobind);
+ seq_printf(seq, "cl_chatty: %d\n", clnt->cl_chatty);
+ seq_printf(seq, "cl_shutdown: %d\n", clnt->cl_shutdown);
+
+ seq_printf(seq, "cl_xprtsec: %d\n", clnt->cl_xprtsec.policy);
+ seq_printf(seq, "cl_rtt: timeo=%ld\n", clnt->cl_rtt->timeo);
+ seq_printf(seq, "cl_timeout: "
+ "to_initval=%ld, to_maxval=%ld, to_increment=%ld, "
+ "to_retries=%d, to_exponential=%d\n",
+ timeout->to_initval, timeout->to_maxval,
+ timeout->to_increment,
+ timeout->to_retries, timeout->to_exponential);
+
+ seq_printf(seq, "cl_max_connect: %d\n", clnt->cl_max_connect);
+ seq_printf(seq, "cl_nodename: ");
+ seq_write(seq, clnt->cl_nodename, clnt->cl_nodelen);
+ seq_printf(seq, "\n");
+
+ return 0;
+}
+
+static int
+clnt_info_open(struct inode *inode, struct file *filp)
+{
+ int ret;
+ struct rpc_clnt *clnt = inode->i_private;
+
+ ret = single_open(filp, clnt_info_show, clnt);
+
+ if (!ret && !refcount_inc_not_zero(&clnt->cl_count)) {
+ single_release(inode, filp);
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+static int
+clnt_info_release(struct inode *inode, struct file *filp)
+{
+ struct seq_file *seq = filp->private_data;
+ struct rpc_clnt *clnt = seq->private;
+
+ rpc_release_client(clnt);
+ return seq_release(inode, filp);
+}
+
+static const struct file_operations info_fops = {
+ .owner = THIS_MODULE,
+ .open = clnt_info_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = clnt_info_release,
+};
+
void
rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
{
@@ -160,6 +231,10 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
debugfs_create_file("tasks", S_IFREG | 0400, clnt->cl_debugfs, clnt,
&tasks_fops);
+ /* make info file */
+ debugfs_create_file("info", S_IFREG | 0400, clnt->cl_debugfs, clnt,
+ &info_fops);
+
rpc_clnt_iterate_for_each_xprt(clnt, do_xprt_debugfs, &xprtnum);
}
--
2.39.3
--
Dan Aloni
On 2023-11-07 08:46:54, Benjamin Coddington wrote:
> Hi Dan,
>
> On 5 Nov 2023, at 10:48, Dan Aloni wrote:
>
> > Hi,
> >
> > On Linux v6.6-14500-g1c41041124bd, I added a sysfs file for debugging
> > `/sys/kernel/debug/sunrpc/rpc_clnt/*/info`, and noticed that when
> > passing the following mount options: `soft,timeo=50,retrans=16,vers=3`,
> > NFSACL and NSM seem to take the defaults from somewhere else (xprt).
> > Specifically, locking operation behave as if in a hard mount with
> > these mount options.
> >
> > Is it intentional?
>
> Yes, it usually is intentional. The various rpc clients that make parts of
> NFS work don't all inherit the mount flags due to reasons about how the
> system should behave as a whole. I think that you can find usually find the
> reasoning the git logs around "struct rpc_create_args".
>
> Are you getting a system hung up in a lock operation?
Actually my concern is the NFSACL prog. With `cl_softrtrt == 1` and
`to_initval == to_maxval`, does it mean retires will not happen
regardless of `to_retries` and `to_increment`?
I encountered a situation where the NFSACL program did not retry but
could have had, whereas NFS3 did successfully. Not sure regarding NSM,
but it seems to me that it would make sense at least for NFSACL to
behave the same as NFS3.
--
Dan Aloni
On 29 Nov 2023, at 8:20, Dan Aloni wrote:
> On 2023-11-07 08:46:54, Benjamin Coddington wrote:
>> Hi Dan,
>>
>> On 5 Nov 2023, at 10:48, Dan Aloni wrote:
>>
>>> Hi,
>>>
>>> On Linux v6.6-14500-g1c41041124bd, I added a sysfs file for debugging
>>> `/sys/kernel/debug/sunrpc/rpc_clnt/*/info`, and noticed that when
>>> passing the following mount options: `soft,timeo=50,retrans=16,vers=3`,
>>> NFSACL and NSM seem to take the defaults from somewhere else (xprt).
>>> Specifically, locking operation behave as if in a hard mount with
>>> these mount options.
>>>
>>> Is it intentional?
>>
>> Yes, it usually is intentional. The various rpc clients that make parts of
>> NFS work don't all inherit the mount flags due to reasons about how the
>> system should behave as a whole. I think that you can find usually find the
>> reasoning the git logs around "struct rpc_create_args".
>>
>> Are you getting a system hung up in a lock operation?
>
> Actually my concern is the NFSACL prog. With `cl_softrtrt == 1` and
> `to_initval == to_maxval`, does it mean retires will not happen
> regardless of `to_retries` and `to_increment`?
Possibly? I'm not exactly certain of what should happen in that case.
> I encountered a situation where the NFSACL program did not retry but
> could have had, whereas NFS3 did successfully. Not sure regarding NSM,
> but it seems to me that it would make sense at least for NFSACL to
> behave the same as NFS3.
I agree, but I could be missing something -- maybe its a bug. There's the
sunrpc:rpc_timeout_status tracepoint that might be helpful. If you turn
that up can you see rpc_check_timeout() getting called from
call_transmit_status()?
Sorry, I know this isn't a lot of help so far.
Ben
On 2023-11-30 09:30:52, Benjamin Coddington wrote:
> > Actually my concern is the NFSACL prog. With `cl_softrtrt == 1` and
> > `to_initval == to_maxval`, does it mean retires will not happen
> > regardless of `to_retries` and `to_increment`?
>
> Possibly? I'm not exactly certain of what should happen in that case.
>
> > I encountered a situation where the NFSACL program did not retry but
> > could have had, whereas NFS3 did successfully. Not sure regarding NSM,
> > but it seems to me that it would make sense at least for NFSACL to
> > behave the same as NFS3.
>
> I agree, but I could be missing something -- maybe its a bug. There's the
> sunrpc:rpc_timeout_status tracepoint that might be helpful. If you turn
> that up can you see rpc_check_timeout() getting called from
> call_transmit_status()?
Sorry, took awhile to get a test working while busy on other stuff.
So it looks really like a bug, here are the details.
Server: nfsd with extra fault injection code that calls `svc_drop()` only once
on a single NFS GETACL request.
Client: Linux v6.8, NFS mount with `soft,timeo=50,retrans=16,vers=3`.
I trace client execution with the following:
sudo perf trace -e sunrpc:rpc_task_timeout -e sunrpc:xprt_retransmit
A simple `ls -l` gets stuck and shows an IO failure:
[root@client export]# ls -l
ls: file: Input/output error
total 0
-rw-r--r-- 1 root root 0 Apr 10 10:02 file
I get a single event out of the tracing above:
```
kthreadd/7926 sunrpc:rpc_task_timeout(task_id: 203, client_id: 6, xprt_id: 3, action: 0xffffffffc0accc60, runstate: 22, flags: 35456)
```
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 and
LOOKUP, I only get the expected 5 seconds delay following a successful
retry.
Seems we only have an issue with the NFS3ACL prog.
--
Dan Aloni
On 10 Apr 2024, at 10:39, Dan Aloni wrote:
> On 2023-11-30 09:30:52, Benjamin Coddington wrote:
>>> Actually my concern is the NFSACL prog. With `cl_softrtrt == 1` and
>>> `to_initval == to_maxval`, does it mean retires will not happen
>>> regardless of `to_retries` and `to_increment`?
>>
>> Possibly? I'm not exactly certain of what should happen in that case.
>>
>>> I encountered a situation where the NFSACL program did not retry but
>>> could have had, whereas NFS3 did successfully. Not sure regarding NSM,
>>> but it seems to me that it would make sense at least for NFSACL to
>>> behave the same as NFS3.
>>
>> I agree, but I could be missing something -- maybe its a bug. There's the
>> sunrpc:rpc_timeout_status tracepoint that might be helpful. If you turn
>> that up can you see rpc_check_timeout() getting called from
>> call_transmit_status()?
>
> Sorry, took awhile to get a test working while busy on other stuff.
>
> So it looks really like a bug, here are the details.
>
> Server: nfsd with extra fault injection code that calls `svc_drop()` only once
> on a single NFS GETACL request.
> Client: Linux v6.8, NFS mount with `soft,timeo=50,retrans=16,vers=3`.
>
> I trace client execution with the following:
>
> sudo perf trace -e sunrpc:rpc_task_timeout -e sunrpc:xprt_retransmit
>
> A simple `ls -l` gets stuck and shows an IO failure:
>
> [root@client export]# ls -l
> ls: file: Input/output error
> total 0
> -rw-r--r-- 1 root root 0 Apr 10 10:02 file
>
> I get a single event out of the tracing above:
>
> ```
> kthreadd/7926 sunrpc:rpc_task_timeout(task_id: 203, client_id: 6, xprt_id: 3, action: 0xffffffffc0accc60, runstate: 22, flags: 35456)
> ```
>
> 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 and
> LOOKUP, I only get the expected 5 seconds delay following a successful
> 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 should
fix it up, would you like to test it?
Ben
---
From 54a35f530d78a8042dc6fb8ff522f5a4a33fa50b Mon Sep 17 00:00:00 2001
Message-ID: <54a35f530d78a8042dc6fb8ff522f5a4a33fa50b.1712848680.git.bcodding@redhat.com>
From: Benjamin Coddington <[email protected]>
Date: Thu, 11 Apr 2024 11:03:06 -0400
Subject: [PATCH] NFS: Set v3 ACL client default timeout values
It appears the client_acl rpc_clnt doesn't take any timeout values, so
retries for the ACL procedures are never attempted. Set them based on the
mount's default timeout values.
Reported-by: Dan Aloni <[email protected]>
Fixes: 331702337f2b ("NFS: Support per-mountpoint timeout parameters.")
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/nfs3client.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/nfs/nfs3client.c b/fs/nfs/nfs3client.c
index b0c8a39c2bbd..66bb1f56c5d9 100644
--- a/fs/nfs/nfs3client.c
+++ b/fs/nfs/nfs3client.c
@@ -33,6 +33,7 @@ static void nfs_init_server_aclclient(struct nfs_server *server)
if (IS_ERR(server->client_acl))
goto out_noacl;
+ server->client_acl->cl_timeout = &server->client->cl_timeout_default;
nfs_sysfs_link_rpc_client(server, server->client_acl, NULL);
/* No errors! Assume that Sun nfsacls are supported */
--
2.44.0
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 and
> > LOOKUP, I only get the expected 5 seconds delay following a successful
> > 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 should
> 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 when
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 = &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 be
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 <[email protected]>
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 copy `cl_timeout` and make sure `cl_rtt_default`
is initialized from it.
Cc: Chuck Lever <[email protected]>
Cc: Benjamin Coddington <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/T/
Fixes: 1b63a75180c6 ('SUNRPC: Refactor rpc_clone_client()')
Signed-off-by: Dan Aloni <[email protected]>
---
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 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;
out_err:
--
2.39.3
--
Dan Aloni
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 and
>>> LOOKUP, I only get the expected 5 seconds delay following a successful
>>> 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 should
>> 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 when
> 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 = &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 be
> 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 <[email protected]>
> 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 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 well?
> Cc: Chuck Lever <[email protected]>
> Cc: Benjamin Coddington <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/T/
> Fixes: 1b63a75180c6 ('SUNRPC: Refactor rpc_clone_client()')
> Signed-off-by: Dan Aloni <[email protected]>
> ---
> 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 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);
> +
So we'll end up doing rpc_init_rtt twice, first in rpc_new_client then here.
Maybe we should be passing cl_timeout on .timeout from the parent in the
rpc_clone_client()'s rpc_create_args?
.. anyway - I'll stop "helping", you've clearly got this in hand. :)
Ben
On Thu, 2024-04-11 at 19:36 +0300, 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 and
> > > LOOKUP, I only get the expected 5 seconds delay following a
> > > successful
> > > 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
> > should
> > 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
> when
> 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 = &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
> be
> 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 <[email protected]>
> 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 copy `cl_timeout` and make sure `cl_rtt_default`
> is initialized from it.
>
> Cc: Chuck Lever <[email protected]>
> Cc: Benjamin Coddington <[email protected]>
> Link:
> https://lore.kernel.org/all/[email protected]/T/
> Fixes: 1b63a75180c6 ('SUNRPC: Refactor rpc_clone_client()')
> Signed-off-by: Dan Aloni <[email protected]>
> ---
> 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
> 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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
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 <[email protected]>
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 <[email protected]>
Cc: Benjamin Coddington <[email protected]>
Cc: Trond Myklebust <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/T/
Fixes: 1b63a75180c6 ('SUNRPC: Refactor rpc_clone_client()')
Signed-off-by: Dan Aloni <[email protected]>
---
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