diff mbox series

[5/5] cifs: enforce nosharesock when multichannel is used

Message ID 20240201111530.17194-5-sprasad@microsoft.com
State New
Headers show
Series [1/5] cifs: avoid redundant calls to disable multichannel | expand

Commit Message

Shyam Prasad N Feb. 1, 2024, 11:15 a.m. UTC 
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(+)

Comments

Shyam Prasad N Feb. 6, 2024, 6:13 a.m. UTC | #1 
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.
diff mbox series

Patch

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;