Re: [PATCH 1/2] cifs: client: force multichannel=off when max_channels=1

[Enzo Matsumiya]

On 09/22, Henrique Carvalho wrote:
> On 9/22/25 1:14 PM, Steve French wrote:
> > . >Do we even need ->multichannel flag at all?
> >
> > Yes - especially in the future.   The goal is for the user to have
> > three options:
> > 1) (preferred) "multichannel" (max channels will be dynamically
> > selected and can change) the client gets to choose how many channels
> > to connect to based on what it sees in the output of the most recent
> > query interfaces call (it can change on the fly as server dynamically
> > adds and removes channels or networks become temporarily unreachable)
>
> I'm guessing this would be required while we are transitioning from
> setting channels dynamically to having multichannel on by default, as
> you commented below. Because once we have it on by default, I don't
> think there is a point in having the flag.

Exactly.

> > 2) "max_channels="   This is for the case where user wants to choose
> > the number of channels rather than have the client automatically
> > (hopefully soon in the future) choose it for you
> > 3) if server has mchan bugs, allow client to mount with no
> > multichannel (or equivalent max_channels=1)
> >
> > But ... "remount" also has to work for the three above (and currently
> > doesn't) and this is what I am hoping the recent patches can fix (?)
> > but haven't tested them enough yet

I was talking more in the context of code, that it could use some
refactoring/improvements -- I also think such functionality (hence the
patches) are necessary.

There's no sense for me, as a user, to specify e.g.:
  # mount.cifs -o ...,multichannel,max_channels=2 ...

Was there only a single option for this, it would be less confusing and
wouldn't require this patch here.


The below patch (PoC) is an idea I had that would make things much
clearer for users -- have 'multipath' mount option be either a flag or a
value option, e.g.:

  # mount.cifs -o ...,multichannel //srv/share /mnt/test
  # findmnt -t cifs -u | grep -Eo 'max_channels=[0-9]+'
  max_channels=2
  # umount /mnt/test
  # mount.cifs -o ...,multichannel=4 //srv/share /mnt/test
  # findmnt -t cifs -u | grep -Eo 'max_channels=[0-9]+'
  max_channels=4


Cheers,

Enzo

---------------------

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index dd12f3eb61dc..ad9a588c7103 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -2487,7 +2487,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 	spin_lock(&ses->chan_lock);
 	ses->chans[0].server = server;
 	ses->chan_count = 1;
-	ses->chan_max = ctx->multichannel ? ctx->max_channels:1;
+	ses->chan_max = ctx->max_channels;
 	ses->chans_need_reconnect = 1;
 	spin_unlock(&ses->chan_lock);
 
diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 072383899e81..ceb19a58b743 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -165,7 +165,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
 	fsparam_u32("max_cached_dirs", Opt_max_cached_dirs),
 	fsparam_u32("handletimeout", Opt_handletimeout),
 	fsparam_u64("snapshot", Opt_snapshot),
-	fsparam_u32("max_channels", Opt_max_channels),
+	fsparam_u32("multichannel", Opt_multichannel),
 
 	/* Mount options which take string value */
 	fsparam_string("source", Opt_source),
@@ -1252,14 +1252,16 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		ctx->nodelete = 1;
 		break;
 	case Opt_multichannel:
-		if (result.negated) {
-			ctx->multichannel = false;
-			ctx->max_channels = 1;
-		} else {
-			ctx->multichannel = true;
-			/* if number of channels not specified, default to 2 */
-			if (ctx->max_channels < 2)
+		if (param->type == fs_value_is_flag) {
+			if (!result.negated && ctx->max_channels < 2)
 				ctx->max_channels = 2;
+		} else {
+			if (result.uint_32 < 1 || result.uint_32 > CIFS_MAX_CHANNELS) {
+				cifs_errorf(fc, "%s: Invalid max_channels value, needs to be 1-%d\n",
+					 __func__, CIFS_MAX_CHANNELS);
+				goto cifs_parse_mount_err;
+			}
+			ctx->max_channels = result.uint_32;
 		}
 		break;
 	case Opt_uid:
@@ -1395,17 +1397,6 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		}
 		ctx->max_credits = result.uint_32;
 		break;
-	case Opt_max_channels:
-		if (result.uint_32 < 1 || result.uint_32 > CIFS_MAX_CHANNELS) {
-			cifs_errorf(fc, "%s: Invalid max_channels value, needs to be 1-%d\n",
-				 __func__, CIFS_MAX_CHANNELS);
-			goto cifs_parse_mount_err;
-		}
-		ctx->max_channels = result.uint_32;
-		/* If more than one channel requested ... they want multichan */
-		if (result.uint_32 > 1)
-			ctx->multichannel = true;
-		break;
 	case Opt_max_cached_dirs:
 		if (result.uint_32 < 1) {
 			cifs_errorf(fc, "%s: Invalid max_cached_dirs, needs to be 1 or more\n",
@@ -1901,7 +1892,6 @@ int smb3_init_fs_context(struct fs_context *fc)
 	ctx->echo_interval = SMB_ECHO_INTERVAL_DEFAULT;
 
 	/* default to no multichannel (single server connection) */
-	ctx->multichannel = false;
 	ctx->max_channels = 1;
 
 	ctx->backupuid_specified = false; /* no backup intent for a user */
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index b0fec6b9a23b..ff75a7cc11dd 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -175,7 +175,6 @@ enum cifs_param {
 	Opt_max_credits,
 	Opt_max_cached_dirs,
 	Opt_snapshot,
-	Opt_max_channels,
 	Opt_handletimeout,
 
 	/* Mount options which take string value */
@@ -293,7 +292,6 @@ struct smb3_fs_context {
 	bool resilient:1; /* noresilient not required since not fored for CA */
 	bool domainauto:1;
 	bool rdma:1;
-	bool multichannel:1;
 	bool use_client_guid:1;
 	/* reuse existing guid for multichannel */
 	u8 client_guid[SMB2_CLIENT_GUID_SIZE];