* [PATCH 2/5] cifs: change tcon status when need_reconnect is set on it
2024-02-01 11:15 [PATCH 1/5] cifs: avoid redundant calls to disable multichannel nspmangalore
@ 2024-02-01 11:15 ` nspmangalore
2024-02-01 11:15 ` [PATCH 3/5] cifs: do not search for channel if server is terminating nspmangalore
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: nspmangalore @ 2024-02-01 11:15 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
When a tcon is marked for need_reconnect, the intention
is to have it reconnected.
This change adjusts tcon->status in cifs_tree_connect
when need_reconnect is set. Also, this change has a minor
correction in resetting need_reconnect on success. It makes
sure that it is done with tc_lock held.
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/connect.c | 5 +++++
fs/smb/client/dfs.c | 7 ++++++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index c5cf88de32b7..28cacdd90bbf 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -4230,6 +4230,11 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
/* only send once per connect */
spin_lock(&tcon->tc_lock);
+
+ /* if tcon is marked for needing reconnect, update state */
+ if (tcon->need_reconnect)
+ tcon->status = TID_NEED_TCON;
+
if (tcon->status == TID_GOOD) {
spin_unlock(&tcon->tc_lock);
return 0;
diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
index a8a1d386da65..449c59830039 100644
--- a/fs/smb/client/dfs.c
+++ b/fs/smb/client/dfs.c
@@ -565,6 +565,11 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
/* only send once per connect */
spin_lock(&tcon->tc_lock);
+
+ /* if tcon is marked for needing reconnect, update state */
+ if (tcon->need_reconnect)
+ tcon->status = TID_NEED_TCON;
+
if (tcon->status == TID_GOOD) {
spin_unlock(&tcon->tc_lock);
return 0;
@@ -625,8 +630,8 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
spin_lock(&tcon->tc_lock);
if (tcon->status == TID_IN_TCON)
tcon->status = TID_GOOD;
- spin_unlock(&tcon->tc_lock);
tcon->need_reconnect = false;
+ spin_unlock(&tcon->tc_lock);
}
return rc;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/5] cifs: do not search for channel if server is terminating
2024-02-01 11:15 [PATCH 1/5] cifs: avoid redundant calls to disable multichannel nspmangalore
2024-02-01 11:15 ` [PATCH 2/5] cifs: change tcon status when need_reconnect is set on it nspmangalore
@ 2024-02-01 11:15 ` nspmangalore
2024-02-01 11:15 ` [PATCH 4/5] cifs: failure to add channel on iface should bump up weight nspmangalore
2024-02-01 11:15 ` [PATCH 5/5] cifs: enforce nosharesock when multichannel is used nspmangalore
3 siblings, 0 replies; 6+ messages in thread
From: nspmangalore @ 2024-02-01 11:15 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
In order to scale down the channels, the following sequence
of operations happen:
1. server struct is marked for terminate
2. the channel is deallocated in the ses->chans array
3. at a later point the cifsd thread actually terminates the server
Between 2 and 3, there can be calls to find the channel for
a server struct. When that happens, there can be an ugly warning
that's logged. But this is expected.
So this change does two things:
1. in cifs_ses_get_chan_index, if server->terminate is set, return
2. always make sure server->terminate is set with chan_lock held
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/sess.c | 4 ++++
fs/smb/client/smb2pdu.c | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index cde81042bebd..3d2548c35c9d 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -75,6 +75,10 @@ cifs_ses_get_chan_index(struct cifs_ses *ses,
{
unsigned int i;
+ /* if the channel is waiting for termination */
+ if (server->terminate)
+ return CIFS_INVAL_CHAN_INDEX;
+
for (i = 0; i < ses->chan_count; i++) {
if (ses->chans[i].server == server)
return i;
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index 75713559e69f..cf5665f6f220 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -178,6 +178,7 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
}
ses->chans[chan_index].server = NULL;
+ server->terminate = true;
spin_unlock(&ses->chan_lock);
/*
@@ -188,7 +189,6 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
*/
cifs_put_tcp_session(server, from_reconnect);
- server->terminate = true;
cifs_signal_cifsd_for_reconnect(server, false);
/* mark primary server as needing reconnect */
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/5] cifs: failure to add channel on iface should bump up weight
2024-02-01 11:15 [PATCH 1/5] cifs: avoid redundant calls to disable multichannel nspmangalore
2024-02-01 11:15 ` [PATCH 2/5] cifs: change tcon status when need_reconnect is set on it nspmangalore
2024-02-01 11:15 ` [PATCH 3/5] cifs: do not search for channel if server is terminating nspmangalore
@ 2024-02-01 11:15 ` nspmangalore
2024-02-01 11:15 ` [PATCH 5/5] cifs: enforce nosharesock when multichannel is used nspmangalore
3 siblings, 0 replies; 6+ messages in thread
From: nspmangalore @ 2024-02-01 11:15 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
After the interface selection policy change to do a weighted
round robin, each iface maintains a weight_fulfilled. When the
weight_fulfilled reaches the total weight for the iface, we know
that the weights can be reset and ifaces can be allocated from
scratch again.
During channel allocation failures on a particular channel,
weight_fulfilled is not incremented. If a few interfaces are
inactive, we could end up in a situation where the active
interfaces are all allocated for the total_weight, and inactive
ones are all that remain. This can cause a situation where
no more channels can be allocated further.
This change fixes it by increasing weight_fulfilled, even when
channel allocation failure happens. This could mean that if
there are temporary failures in channel allocation, the iface
weights may not strictly be adhered to. But that's still okay.
Fixes: a6d8fb54a515 ("cifs: distribute channels across interfaces based on speed")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/sess.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index 3d2548c35c9d..ed4bd88dd528 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -273,6 +273,8 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
&iface->sockaddr,
rc);
kref_put(&iface->refcount, release_iface);
+ /* failure to add chan should increase weight */
+ iface->weight_fulfilled++;
continue;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 5/5] cifs: enforce nosharesock when multichannel is used
2024-02-01 11:15 [PATCH 1/5] cifs: avoid redundant calls to disable multichannel nspmangalore
` (2 preceding siblings ...)
2024-02-01 11:15 ` [PATCH 4/5] cifs: failure to add channel on iface should bump up weight nspmangalore
@ 2024-02-01 11:15 ` nspmangalore
2024-02-06 6:13 ` Shyam Prasad N
3 siblings, 1 reply; 6+ messages in thread
From: nspmangalore @ 2024-02-01 11:15 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm; +Cc: Shyam Prasad N
From: Shyam Prasad N <sprasad@microsoft.com>
In the current architecture, multiple sessions can
share the primary channel, but secondary channels for
each session is not shared.
This can create two problems when primary channel is
shared among several sessions. For one, there could be
uneven utilization of channels due to this skew.
Another major issue is how a cifsd thread can get to
the channel for a secondary channel. The process is
already cumbersome. We also need to find the right
session for the server struct.
To avoid both the problems, this change marks even the
primary channel as nosharesock. Secondary channels are
marked as nosharesock anyway.
We can remove this when we fix the mchan architecture
to share all channels.
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
fs/smb/client/fs_context.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 82eafe0815dc..e7543574ea9e 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1043,6 +1043,8 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
ctx->max_channels = 1;
} else {
ctx->multichannel = true;
+ /* enforce nosharesock */
+ ctx->nosharesock = true;
/* if number of channels not specified, default to 2 */
if (ctx->max_channels < 2)
ctx->max_channels = 2;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 5/5] cifs: enforce nosharesock when multichannel is used
2024-02-01 11:15 ` [PATCH 5/5] cifs: enforce nosharesock when multichannel is used nspmangalore
@ 2024-02-06 6:13 ` Shyam Prasad N
0 siblings, 0 replies; 6+ messages in thread
From: Shyam Prasad N @ 2024-02-06 6:13 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, bharathsm; +Cc: Shyam Prasad N
On Thu, Feb 1, 2024 at 4:45 PM <nspmangalore@gmail.com> wrote:
>
> From: Shyam Prasad N <sprasad@microsoft.com>
>
> In the current architecture, multiple sessions can
> share the primary channel, but secondary channels for
> each session is not shared.
>
> This can create two problems when primary channel is
> shared among several sessions. For one, there could be
> uneven utilization of channels due to this skew.
>
> Another major issue is how a cifsd thread can get to
> the channel for a secondary channel. The process is
> already cumbersome. We also need to find the right
> session for the server struct.
>
> To avoid both the problems, this change marks even the
> primary channel as nosharesock. Secondary channels are
> marked as nosharesock anyway.
>
> We can remove this when we fix the mchan architecture
> to share all channels.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
> fs/smb/client/fs_context.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 82eafe0815dc..e7543574ea9e 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1043,6 +1043,8 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
> ctx->max_channels = 1;
> } else {
> ctx->multichannel = true;
> + /* enforce nosharesock */
> + ctx->nosharesock = true;
> /* if number of channels not specified, default to 2 */
> if (ctx->max_channels < 2)
> ctx->max_channels = 2;
> --
> 2.34.1
>
This was discussed offline with Steve. And we decided it's best not to
use this one.
This patch may unnecessarily increase socket utilization for mounts
that could otherwise have shared the sessions.
Our multichannel implementation makes it difficult to solve this problem.
I'll send another patch for now to work around the problem in another
way. (the cumbersome method as mentioned above)
However, please note that the perf skew problem can still happen with
that change. Maybe we can address that in the next kernel version.
--
Regards,
Shyam
^ permalink raw reply [flat|nested] 6+ messages in thread