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

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

[rajasimandalos]

From: Rajasi Mandal <rajasimandal@microsoft.com>

Previously, specifying both multichannel and max_channels=1 as mount
options would leave multichannel enabled, even though it is not
meaningful when only one channel is allowed. This led to confusion and
inconsistent behavior, as the client would advertise multichannel
capability but never establish secondary channels.

Fix this by forcing multichannel to false whenever max_channels=1,
ensuring the mount configuration is consistent and matches user intent.
This prevents the client from advertising or attempting multichannel
support when it is not possible.

Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
---
 fs/smb/client/fs_context.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 072383899e81..43552b44f613 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
 		goto cifs_parse_mount_err;
 	}
 
+	/*
+	 * Multichannel is not meaningful if max_channels is 1.
+	 * Force multichannel to false to ensure consistent configuration.
+	 */
+	if (ctx->multichannel && ctx->max_channels == 1)
+		ctx->multichannel = false;
+
 	return 0;
 
  cifs_parse_mount_err:
-- 
2.43.0

[PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[rajasimandalos]

From: Rajasi Mandal <rajasimandal@microsoft.com>

Previously, the client did not properly update the session's channel
state when multichannel or max_channels mount options were changed
during remount. This led to inconsistent behavior and prevented
enabling or disabling multichannel support without a full
unmount/remount.

Enable dynamic reconfiguration of multichannel and max_channels
options during remount by introducing smb3_sync_ses_chan_max() to
safely update the session's chan_max field, and smb3_sync_ses_channels()
to synchronize the session's channels with the new configuration.
Replace cifs_disable_secondary_channels() with
cifs_decrease_secondary_channels(), which now takes a from_reconfigure
argument for more flexible channel cleanup. Update the remount logic
to detect changes in multichannel or max_channels and trigger the
appropriate session/channel updates.

With this change, users can safely change multichannel and
max_channels options on remount, and the client will correctly adjust
the session's channel state to match the new configuration.

Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
---
 fs/smb/client/cifsproto.h  |  2 +-
 fs/smb/client/fs_context.c | 29 ++++++++++++++++++
 fs/smb/client/fs_context.h |  2 +-
 fs/smb/client/sess.c       | 35 +++++++++++++++-------
 fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
 fs/smb/client/smb2pdu.h    |  2 ++
 6 files changed, 105 insertions(+), 25 deletions(-)

diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index e8fba98690ce..ec3118457b26 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -667,7 +667,7 @@ bool
 cifs_chan_is_iface_active(struct cifs_ses *ses,
 			  struct TCP_Server_Info *server);
 void
-cifs_disable_secondary_channels(struct cifs_ses *ses);
+cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
 void
 cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
 int
diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
index 43552b44f613..96e80c70f25d 100644
--- a/fs/smb/client/fs_context.c
+++ b/fs/smb/client/fs_context.c
@@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
 	return 0;
 }
 
+/**
+ * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
+ * @ses: pointer to the old CIFS session structure
+ * @max_channels: new maximum number of channels to allow
+ *
+ * Updates the session's chan_max field to the new value, protecting the update
+ * with the session's channel lock. This should be called whenever the maximum
+ * allowed channels for a session changes (e.g., after a remount or reconfigure).
+ */
+void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
+{
+	spin_lock(&ses->chan_lock);
+	ses->chan_max = max_channels;
+	spin_unlock(&ses->chan_lock);
+}
+
 static int smb3_reconfigure(struct fs_context *fc)
 {
 	struct smb3_fs_context *ctx = smb3_fc2context(fc);
@@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
 		ses->password2 = new_password2;
 	}
 
+	/*
+	 * If multichannel or max_channels has changed, update the session's channels accordingly.
+	 * This may add or remove channels to match the new configuration.
+	 */
+	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
+		(ctx->max_channels != cifs_sb->ctx->max_channels)) {
+		//Synchronize ses->chan_max with the new mount context
+		smb3_sync_ses_chan_max(ses, ctx->max_channels);
+		//Now update the session's channels to match the new configuration
+		rc = smb3_sync_ses_channels(cifs_sb);
+	}
+
 	mutex_unlock(&ses->session_mutex);
 
 	STEAL_STRING(cifs_sb, ctx, domainname);
@@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
 	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
 	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
 	smb3_update_mnt_flags(cifs_sb);
+
 #ifdef CONFIG_CIFS_DFS_UPCALL
 	if (!rc)
 		rc = dfs_cache_remount_fs(cifs_sb);
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index b0fec6b9a23b..a75185858285 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -371,7 +371,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
 extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
 extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
 extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
-
+extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
 /*
  * max deferred close timeout (jiffies) - 2^30
  */
diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
index 0a8c2fcc9ded..42b5481c884a 100644
--- a/fs/smb/client/sess.c
+++ b/fs/smb/client/sess.c
@@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
 	return new_chan_count - old_chan_count;
 }
 
-/*
- * called when multichannel is disabled by the server.
- * this always gets called from smb2_reconnect
- * and cannot get called in parallel threads.
+/**
+ * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
+ * @ses: pointer to the CIFS session structure
+ * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
+ *
+ * This function disables and cleans up extra secondary channels for a CIFS session.
+ * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
+ * Otherwise, it disables all but the primary channel.
  */
-void
-cifs_disable_secondary_channels(struct cifs_ses *ses)
+void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
 {
 	int i, chan_count;
 	struct TCP_Server_Info *server;
@@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
 	if (chan_count == 1)
 		goto done;
 
-	ses->chan_count = 1;
-
-	/* for all secondary channels reset the need reconnect bit */
-	ses->chans_need_reconnect &= 1;
+	// Update the chan_count to the new maximum
+	if (from_reconfigure)
+		ses->chan_count = ses->chan_max;
+	else
+		ses->chan_count = 1;
 
-	for (i = 1; i < chan_count; i++) {
+	for (i = ses->chan_max ; i < chan_count; i++) {
 		iface = ses->chans[i].iface;
 		server = ses->chans[i].server;
 
@@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
 		spin_lock(&ses->chan_lock);
 	}
 
+	/* For extra secondary channels, reset the need reconnect bit */
+	if (ses->chan_count == 1) {
+		cifs_server_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
+		ses->chans_need_reconnect &= 1;
+	} else {
+		cifs_server_dbg(VFS, "Disable extra secondary channels\n");
+		ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
+	}
+
 done:
 	spin_unlock(&ses->chan_lock);
 }
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index c3b9d3f6210f..bf9a8dc0e8fc 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
 static int
 cifs_chan_skip_or_disable(struct cifs_ses *ses,
 			  struct TCP_Server_Info *server,
-			  bool from_reconnect)
+			  bool from_reconnect, bool from_reconfigure)
 {
 	struct TCP_Server_Info *pserver;
 	unsigned int chan_index;
@@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
 		return -EHOSTDOWN;
 	}
 
-	cifs_server_dbg(VFS,
-		"server does not support multichannel anymore. Disable all other channels\n");
-	cifs_disable_secondary_channels(ses);
+	cifs_decrease_secondary_channels(ses, from_reconfigure);
 
+	return 0;
+}
+
+/**
+ * smb3_sync_ses_channels - Synchronize session channels
+ * with new configuration (cifs_sb_info version)
+ * @cifs_sb: pointer to the CIFS superblock info structure
+ * Returns 0 on success or -EINVAL if scaling is already in progress.
+ */
+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
+{
+	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
+	struct smb3_fs_context *ctx = cifs_sb->ctx;
+	bool from_reconnect = false;
+
+	/* Prevent concurrent scaling operations */
+	spin_lock(&ses->ses_lock);
+	if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
+		spin_unlock(&ses->ses_lock);
+		return -EINVAL;
+	}
+	ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
+	spin_unlock(&ses->ses_lock);
+
+	/*
+	 * If the old max_channels is less than the new chan_max,
+	 * try to add channels to reach the new maximum.
+	 * Otherwise, disable or skip extra channels to match the new configuration.
+	 */
+	if (ctx->max_channels < ses->chan_max) {
+		mutex_unlock(&ses->session_mutex);
+		cifs_try_adding_channels(ses);
+		mutex_lock(&ses->session_mutex);
+	} else {
+		cifs_chan_skip_or_disable(ses, ses->server, from_reconnect, true);
+	}
+
+	/* Clear scaling flag after operation */
+	spin_lock(&ses->ses_lock);
+	ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
+	spin_unlock(&ses->ses_lock);
 
 	return 0;
 }
@@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	if (ses->chan_count > 1 &&
 	    !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
 		rc = cifs_chan_skip_or_disable(ses, server,
-					       from_reconnect);
+					       from_reconnect, false);
 		if (rc) {
 			mutex_unlock(&ses->session_mutex);
 			goto out;
@@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 			 */
 
 			rc = cifs_chan_skip_or_disable(ses, server,
-						       from_reconnect);
+						       from_reconnect, false);
 			goto skip_add_channels;
 		} else if (rc)
 			cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
@@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
 		req->SecurityMode = 0;
 
 	req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
-	if (ses->chan_max > 1)
-		req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
+	req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
 
 	/* ClientGUID must be zero for SMB2.02 dialect */
 	if (server->vals->protocol_id == SMB20_PROT_ID)
@@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (!pneg_inbuf)
 		return -ENOMEM;
 
-	pneg_inbuf->Capabilities =
-			cpu_to_le32(server->vals->req_capabilities);
-	if (tcon->ses->chan_max > 1)
-		pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
+	pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities);
+	pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
 
 	memcpy(pneg_inbuf->Guid, server->client_guid,
 					SMB2_CLIENT_GUID_SIZE);
diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
index 3c09a58dfd07..d3f63a4ef426 100644
--- a/fs/smb/client/smb2pdu.h
+++ b/fs/smb/client/smb2pdu.h
@@ -420,6 +420,8 @@ struct smb2_create_ea_ctx {
 	struct smb2_file_full_ea_info ea;
 } __packed;
 
+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb);
+
 #define SMB2_WSL_XATTR_UID		"$LXUID"
 #define SMB2_WSL_XATTR_GID		"$LXGID"
 #define SMB2_WSL_XATTR_MODE		"$LXMOD"
-- 
2.43.0

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Enzo Matsumiya]

On 09/22, rajasimandalos@gmail.com wrote:
>From: Rajasi Mandal <rajasimandal@microsoft.com>
>
>Previously, the client did not properly update the session's channel
>state when multichannel or max_channels mount options were changed
>during remount. This led to inconsistent behavior and prevented
>enabling or disabling multichannel support without a full
>unmount/remount.
>
>Enable dynamic reconfiguration of multichannel and max_channels
>options during remount by introducing smb3_sync_ses_chan_max() to
>safely update the session's chan_max field, and smb3_sync_ses_channels()
>to synchronize the session's channels with the new configuration.
>Replace cifs_disable_secondary_channels() with
>cifs_decrease_secondary_channels(), which now takes a from_reconfigure
>argument for more flexible channel cleanup. Update the remount logic
>to detect changes in multichannel or max_channels and trigger the
>appropriate session/channel updates.
>
>With this change, users can safely change multichannel and
>max_channels options on remount, and the client will correctly adjust
>the session's channel state to match the new configuration.
>
>Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>---
> fs/smb/client/cifsproto.h  |  2 +-
> fs/smb/client/fs_context.c | 29 ++++++++++++++++++
> fs/smb/client/fs_context.h |  2 +-
> fs/smb/client/sess.c       | 35 +++++++++++++++-------
> fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
> fs/smb/client/smb2pdu.h    |  2 ++
> 6 files changed, 105 insertions(+), 25 deletions(-)

I think the fix is necessary, and the implementation works (at least
with the simple case I tested).  I just think we now have too many
functions related to channel adding/removing/updating and they're all
too similar.  IMHO they could all be merged into a single "update
channels" one.

Do you think it's possible to do that?  Probably would require a bit
more work, but at least we would end up with a centralized place to deal
with channel management.

>diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>index e8fba98690ce..ec3118457b26 100644
>--- a/fs/smb/client/cifsproto.h
>+++ b/fs/smb/client/cifsproto.h
>@@ -667,7 +667,7 @@ bool
> cifs_chan_is_iface_active(struct cifs_ses *ses,
> 			  struct TCP_Server_Info *server);
> void
>-cifs_disable_secondary_channels(struct cifs_ses *ses);
>+cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
> void
> cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
> int
>diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>index 43552b44f613..96e80c70f25d 100644
>--- a/fs/smb/client/fs_context.c
>+++ b/fs/smb/client/fs_context.c
>@@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
> 	return 0;
> }
>
>+/**
>+ * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
>+ * @ses: pointer to the old CIFS session structure
>+ * @max_channels: new maximum number of channels to allow
>+ *
>+ * Updates the session's chan_max field to the new value, protecting the update
>+ * with the session's channel lock. This should be called whenever the maximum
>+ * allowed channels for a session changes (e.g., after a remount or reconfigure).
>+ */
>+void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
>+{
>+	spin_lock(&ses->chan_lock);
>+	ses->chan_max = max_channels;
>+	spin_unlock(&ses->chan_lock);
>+}
>+
> static int smb3_reconfigure(struct fs_context *fc)
> {
> 	struct smb3_fs_context *ctx = smb3_fc2context(fc);
>@@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
> 		ses->password2 = new_password2;
> 	}
>
>+	/*
>+	 * If multichannel or max_channels has changed, update the session's channels accordingly.
>+	 * This may add or remove channels to match the new configuration.
>+	 */
>+	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
>+		(ctx->max_channels != cifs_sb->ctx->max_channels)) {
>+		//Synchronize ses->chan_max with the new mount context
>+		smb3_sync_ses_chan_max(ses, ctx->max_channels);
>+		//Now update the session's channels to match the new configuration
>+		rc = smb3_sync_ses_channels(cifs_sb);
>+	}
>+
> 	mutex_unlock(&ses->session_mutex);
>
> 	STEAL_STRING(cifs_sb, ctx, domainname);
>@@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
> 	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
> 	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
> 	smb3_update_mnt_flags(cifs_sb);
>+
> #ifdef CONFIG_CIFS_DFS_UPCALL
> 	if (!rc)
> 		rc = dfs_cache_remount_fs(cifs_sb);
>diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
>index b0fec6b9a23b..a75185858285 100644
>--- a/fs/smb/client/fs_context.h
>+++ b/fs/smb/client/fs_context.h
>@@ -371,7 +371,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
> extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
> extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
> extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
>-
>+extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
> /*
>  * max deferred close timeout (jiffies) - 2^30
>  */
>diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
>index 0a8c2fcc9ded..42b5481c884a 100644
>--- a/fs/smb/client/sess.c
>+++ b/fs/smb/client/sess.c
>@@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
> 	return new_chan_count - old_chan_count;
> }
>
>-/*
>- * called when multichannel is disabled by the server.
>- * this always gets called from smb2_reconnect
>- * and cannot get called in parallel threads.
>+/**
>+ * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
>+ * @ses: pointer to the CIFS session structure
>+ * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
>+ *
>+ * This function disables and cleans up extra secondary channels for a CIFS session.
>+ * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
>+ * Otherwise, it disables all but the primary channel.
>  */
>-void
>-cifs_disable_secondary_channels(struct cifs_ses *ses)
>+void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
> {
> 	int i, chan_count;
> 	struct TCP_Server_Info *server;
>@@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
> 	if (chan_count == 1)
> 		goto done;
>
>-	ses->chan_count = 1;
>-
>-	/* for all secondary channels reset the need reconnect bit */
>-	ses->chans_need_reconnect &= 1;
>+	// Update the chan_count to the new maximum
>+	if (from_reconfigure)
>+		ses->chan_count = ses->chan_max;
>+	else
>+		ses->chan_count = 1;
>
>-	for (i = 1; i < chan_count; i++) {
>+	for (i = ses->chan_max ; i < chan_count; i++) {
> 		iface = ses->chans[i].iface;
> 		server = ses->chans[i].server;
>
>@@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
> 		spin_lock(&ses->chan_lock);
> 	}
>
>+	/* For extra secondary channels, reset the need reconnect bit */
>+	if (ses->chan_count == 1) {
>+		cifs_server_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
>+		ses->chans_need_reconnect &= 1;
>+	} else {
>+		cifs_server_dbg(VFS, "Disable extra secondary channels\n");
>+		ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
>+	}
>+
> done:
> 	spin_unlock(&ses->chan_lock);
> }
>diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
>index c3b9d3f6210f..bf9a8dc0e8fc 100644
>--- a/fs/smb/client/smb2pdu.c
>+++ b/fs/smb/client/smb2pdu.c
>@@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
> static int
> cifs_chan_skip_or_disable(struct cifs_ses *ses,
> 			  struct TCP_Server_Info *server,
>-			  bool from_reconnect)
>+			  bool from_reconnect, bool from_reconfigure)
> {
> 	struct TCP_Server_Info *pserver;
> 	unsigned int chan_index;
>@@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
> 		return -EHOSTDOWN;
> 	}
>
>-	cifs_server_dbg(VFS,
>-		"server does not support multichannel anymore. Disable all other channels\n");
>-	cifs_disable_secondary_channels(ses);
>+	cifs_decrease_secondary_channels(ses, from_reconfigure);
>
>+	return 0;
>+}
>+
>+/**
>+ * smb3_sync_ses_channels - Synchronize session channels
>+ * with new configuration (cifs_sb_info version)
>+ * @cifs_sb: pointer to the CIFS superblock info structure
>+ * Returns 0 on success or -EINVAL if scaling is already in progress.
>+ */
>+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
>+{
>+	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
>+	struct smb3_fs_context *ctx = cifs_sb->ctx;
>+	bool from_reconnect = false;
>+
>+	/* Prevent concurrent scaling operations */
>+	spin_lock(&ses->ses_lock);
>+	if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
>+		spin_unlock(&ses->ses_lock);
>+		return -EINVAL;
>+	}
>+	ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
>+	spin_unlock(&ses->ses_lock);
>+
>+	/*
>+	 * If the old max_channels is less than the new chan_max,
>+	 * try to add channels to reach the new maximum.
>+	 * Otherwise, disable or skip extra channels to match the new configuration.
>+	 */
>+	if (ctx->max_channels < ses->chan_max) {
>+		mutex_unlock(&ses->session_mutex);
>+		cifs_try_adding_channels(ses);
>+		mutex_lock(&ses->session_mutex);
>+	} else {
>+		cifs_chan_skip_or_disable(ses, ses->server, from_reconnect, true);
>+	}
>+
>+	/* Clear scaling flag after operation */
>+	spin_lock(&ses->ses_lock);
>+	ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>+	spin_unlock(&ses->ses_lock);
>
> 	return 0;
> }
>@@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
> 	if (ses->chan_count > 1 &&
> 	    !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
> 		rc = cifs_chan_skip_or_disable(ses, server,
>-					       from_reconnect);
>+					       from_reconnect, false);
> 		if (rc) {
> 			mutex_unlock(&ses->session_mutex);
> 			goto out;
>@@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
> 			 */
>
> 			rc = cifs_chan_skip_or_disable(ses, server,
>-						       from_reconnect);
>+						       from_reconnect, false);
> 			goto skip_add_channels;
> 		} else if (rc)
> 			cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",

(for all hunks above) Can we stick to /* */ comments instead of //
please?

>@@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
> 		req->SecurityMode = 0;
>
> 	req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>-	if (ses->chan_max > 1)
>-		req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>+	req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>
> 	/* ClientGUID must be zero for SMB2.02 dialect */
> 	if (server->vals->protocol_id == SMB20_PROT_ID)
>@@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> 	if (!pneg_inbuf)
> 		return -ENOMEM;
>
>-	pneg_inbuf->Capabilities =
>-			cpu_to_le32(server->vals->req_capabilities);
>-	if (tcon->ses->chan_max > 1)
>-		pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>+	pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>+	pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);

This effectively makes query_interfaces worker run even with
max_channels=1 -- just a heads up because it didn't look like your
original intention.


Cheers,

Enzo

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Shyam Prasad N]

On Mon, Sep 22, 2025 at 8:43 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 09/22, rajasimandalos@gmail.com wrote:
> >From: Rajasi Mandal <rajasimandal@microsoft.com>
> >
> >Previously, the client did not properly update the session's channel
> >state when multichannel or max_channels mount options were changed
> >during remount. This led to inconsistent behavior and prevented
> >enabling or disabling multichannel support without a full
> >unmount/remount.
> >
> >Enable dynamic reconfiguration of multichannel and max_channels
> >options during remount by introducing smb3_sync_ses_chan_max() to
> >safely update the session's chan_max field, and smb3_sync_ses_channels()
> >to synchronize the session's channels with the new configuration.
> >Replace cifs_disable_secondary_channels() with
> >cifs_decrease_secondary_channels(), which now takes a from_reconfigure
> >argument for more flexible channel cleanup. Update the remount logic
> >to detect changes in multichannel or max_channels and trigger the
> >appropriate session/channel updates.
> >
> >With this change, users can safely change multichannel and
> >max_channels options on remount, and the client will correctly adjust
> >the session's channel state to match the new configuration.
> >
> >Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> >---
> > fs/smb/client/cifsproto.h  |  2 +-
> > fs/smb/client/fs_context.c | 29 ++++++++++++++++++
> > fs/smb/client/fs_context.h |  2 +-
> > fs/smb/client/sess.c       | 35 +++++++++++++++-------
> > fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
> > fs/smb/client/smb2pdu.h    |  2 ++
> > 6 files changed, 105 insertions(+), 25 deletions(-)
>
> I think the fix is necessary, and the implementation works (at least
> with the simple case I tested).  I just think we now have too many
> functions related to channel adding/removing/updating and they're all
> too similar.  IMHO they could all be merged into a single "update
> channels" one.
>
> Do you think it's possible to do that?  Probably would require a bit
> more work, but at least we would end up with a centralized place to deal
> with channel management.

That's a good option. Will explore this with Rajasi.

>
> >diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> >index e8fba98690ce..ec3118457b26 100644
> >--- a/fs/smb/client/cifsproto.h
> >+++ b/fs/smb/client/cifsproto.h
> >@@ -667,7 +667,7 @@ bool
> > cifs_chan_is_iface_active(struct cifs_ses *ses,
> >                         struct TCP_Server_Info *server);
> > void
> >-cifs_disable_secondary_channels(struct cifs_ses *ses);
> >+cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
> > void
> > cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
> > int
> >diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> >index 43552b44f613..96e80c70f25d 100644
> >--- a/fs/smb/client/fs_context.c
> >+++ b/fs/smb/client/fs_context.c
> >@@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
> >       return 0;
> > }
> >
> >+/**
> >+ * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
> >+ * @ses: pointer to the old CIFS session structure
> >+ * @max_channels: new maximum number of channels to allow
> >+ *
> >+ * Updates the session's chan_max field to the new value, protecting the update
> >+ * with the session's channel lock. This should be called whenever the maximum
> >+ * allowed channels for a session changes (e.g., after a remount or reconfigure).
> >+ */
> >+void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
> >+{
> >+      spin_lock(&ses->chan_lock);
> >+      ses->chan_max = max_channels;
> >+      spin_unlock(&ses->chan_lock);
> >+}
> >+
> > static int smb3_reconfigure(struct fs_context *fc)
> > {
> >       struct smb3_fs_context *ctx = smb3_fc2context(fc);
> >@@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
> >               ses->password2 = new_password2;
> >       }
> >
> >+      /*
> >+       * If multichannel or max_channels has changed, update the session's channels accordingly.
> >+       * This may add or remove channels to match the new configuration.
> >+       */
> >+      if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
> >+              (ctx->max_channels != cifs_sb->ctx->max_channels)) {
> >+              //Synchronize ses->chan_max with the new mount context
> >+              smb3_sync_ses_chan_max(ses, ctx->max_channels);
> >+              //Now update the session's channels to match the new configuration
> >+              rc = smb3_sync_ses_channels(cifs_sb);
> >+      }
> >+
> >       mutex_unlock(&ses->session_mutex);
> >
> >       STEAL_STRING(cifs_sb, ctx, domainname);
> >@@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
> >       smb3_cleanup_fs_context_contents(cifs_sb->ctx);
> >       rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
> >       smb3_update_mnt_flags(cifs_sb);
> >+
> > #ifdef CONFIG_CIFS_DFS_UPCALL
> >       if (!rc)
> >               rc = dfs_cache_remount_fs(cifs_sb);
> >diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> >index b0fec6b9a23b..a75185858285 100644
> >--- a/fs/smb/client/fs_context.h
> >+++ b/fs/smb/client/fs_context.h
> >@@ -371,7 +371,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
> > extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
> > extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
> > extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
> >-
> >+extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
> > /*
> >  * max deferred close timeout (jiffies) - 2^30
> >  */
> >diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
> >index 0a8c2fcc9ded..42b5481c884a 100644
> >--- a/fs/smb/client/sess.c
> >+++ b/fs/smb/client/sess.c
> >@@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
> >       return new_chan_count - old_chan_count;
> > }
> >
> >-/*
> >- * called when multichannel is disabled by the server.
> >- * this always gets called from smb2_reconnect
> >- * and cannot get called in parallel threads.
> >+/**
> >+ * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
> >+ * @ses: pointer to the CIFS session structure
> >+ * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
> >+ *
> >+ * This function disables and cleans up extra secondary channels for a CIFS session.
> >+ * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
> >+ * Otherwise, it disables all but the primary channel.
> >  */
> >-void
> >-cifs_disable_secondary_channels(struct cifs_ses *ses)
> >+void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
> > {
> >       int i, chan_count;
> >       struct TCP_Server_Info *server;
> >@@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
> >       if (chan_count == 1)
> >               goto done;
> >
> >-      ses->chan_count = 1;
> >-
> >-      /* for all secondary channels reset the need reconnect bit */
> >-      ses->chans_need_reconnect &= 1;
> >+      // Update the chan_count to the new maximum
> >+      if (from_reconfigure)
> >+              ses->chan_count = ses->chan_max;
> >+      else
> >+              ses->chan_count = 1;
> >
> >-      for (i = 1; i < chan_count; i++) {
> >+      for (i = ses->chan_max ; i < chan_count; i++) {
> >               iface = ses->chans[i].iface;
> >               server = ses->chans[i].server;
> >
> >@@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
> >               spin_lock(&ses->chan_lock);
> >       }
> >
> >+      /* For extra secondary channels, reset the need reconnect bit */
> >+      if (ses->chan_count == 1) {
> >+              cifs_server_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
> >+              ses->chans_need_reconnect &= 1;
> >+      } else {
> >+              cifs_server_dbg(VFS, "Disable extra secondary channels\n");
> >+              ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
> >+      }
> >+
> > done:
> >       spin_unlock(&ses->chan_lock);
> > }
> >diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> >index c3b9d3f6210f..bf9a8dc0e8fc 100644
> >--- a/fs/smb/client/smb2pdu.c
> >+++ b/fs/smb/client/smb2pdu.c
> >@@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
> > static int
> > cifs_chan_skip_or_disable(struct cifs_ses *ses,
> >                         struct TCP_Server_Info *server,
> >-                        bool from_reconnect)
> >+                        bool from_reconnect, bool from_reconfigure)
> > {
> >       struct TCP_Server_Info *pserver;
> >       unsigned int chan_index;
> >@@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
> >               return -EHOSTDOWN;
> >       }
> >
> >-      cifs_server_dbg(VFS,
> >-              "server does not support multichannel anymore. Disable all other channels\n");
> >-      cifs_disable_secondary_channels(ses);
> >+      cifs_decrease_secondary_channels(ses, from_reconfigure);
> >
> >+      return 0;
> >+}
> >+
> >+/**
> >+ * smb3_sync_ses_channels - Synchronize session channels
> >+ * with new configuration (cifs_sb_info version)
> >+ * @cifs_sb: pointer to the CIFS superblock info structure
> >+ * Returns 0 on success or -EINVAL if scaling is already in progress.
> >+ */
> >+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
> >+{
> >+      struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
> >+      struct smb3_fs_context *ctx = cifs_sb->ctx;
> >+      bool from_reconnect = false;
> >+
> >+      /* Prevent concurrent scaling operations */
> >+      spin_lock(&ses->ses_lock);
> >+      if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
> >+              spin_unlock(&ses->ses_lock);
> >+              return -EINVAL;
> >+      }
> >+      ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
> >+      spin_unlock(&ses->ses_lock);
> >+
> >+      /*
> >+       * If the old max_channels is less than the new chan_max,
> >+       * try to add channels to reach the new maximum.
> >+       * Otherwise, disable or skip extra channels to match the new configuration.
> >+       */
> >+      if (ctx->max_channels < ses->chan_max) {
> >+              mutex_unlock(&ses->session_mutex);
> >+              cifs_try_adding_channels(ses);
> >+              mutex_lock(&ses->session_mutex);
> >+      } else {
> >+              cifs_chan_skip_or_disable(ses, ses->server, from_reconnect, true);
> >+      }
> >+
> >+      /* Clear scaling flag after operation */
> >+      spin_lock(&ses->ses_lock);
> >+      ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
> >+      spin_unlock(&ses->ses_lock);
> >
> >       return 0;
> > }
> >@@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
> >       if (ses->chan_count > 1 &&
> >           !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
> >               rc = cifs_chan_skip_or_disable(ses, server,
> >-                                             from_reconnect);
> >+                                             from_reconnect, false);
> >               if (rc) {
> >                       mutex_unlock(&ses->session_mutex);
> >                       goto out;
> >@@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
> >                        */
> >
> >                       rc = cifs_chan_skip_or_disable(ses, server,
> >-                                                     from_reconnect);
> >+                                                     from_reconnect, false);
> >                       goto skip_add_channels;
> >               } else if (rc)
> >                       cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
>
> (for all hunks above) Can we stick to /* */ comments instead of //
> please?
>
> >@@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
> >               req->SecurityMode = 0;
> >
> >       req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
> >-      if (ses->chan_max > 1)
> >-              req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> >+      req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> >
> >       /* ClientGUID must be zero for SMB2.02 dialect */
> >       if (server->vals->protocol_id == SMB20_PROT_ID)
> >@@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> >       if (!pneg_inbuf)
> >               return -ENOMEM;
> >
> >-      pneg_inbuf->Capabilities =
> >-                      cpu_to_le32(server->vals->req_capabilities);
> >-      if (tcon->ses->chan_max > 1)
> >-              pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> >+      pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities);
> >+      pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>
> This effectively makes query_interfaces worker run even with
> max_channels=1 -- just a heads up because it didn't look like your
> original intention.
>
I discussed this with Rajasi during my review. We should probably have
a comment in the code somewhere about this behaviour.
We could disable the worker when multichannel gets disabled, and
enable it when multichannel gets enabled.
However, that needs careful changes. I didn't think that a worker to
refresh server interfaces every ten minutes is such a big deal. So I
was okay with this.
Perhaps we can fix this in a future patch.

>
> Cheers,
>
> Enzo
>


-- 
Regards,
Shyam

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Enzo Matsumiya]

On 09/23, RAJASI MANDAL wrote:
>> (for all hunks above) Can we stick to /* */ comments instead of //
>please?
>
>Yes sure I will do that.
>
>> This effectively makes query_interfaces worker run even with
>max_channels=1 -- just a heads up because it didn't look like your
>original intention.
>
>This change ensures that the multichannel option can be enabled during
>remounts, even if it wasn't specified during the initial mount.
>During the initial mount, if the user does not pass the multichannel
>option, the client does not advertise multichannel capability to the
>server.
>Since remounts do not trigger a fresh negotiation with the server, it's not
>possible to enable multichannel at that stage if it was previously omitted.
>
>To address this, the client is modified to *advertise multichannel
>capability by default*, regardless of whether the option was explicitly
>provided during the initial mount.

Yes, and I agree with that change.

I was just mentioning that because cifs_get_tcon() starts the
query_interface worker based on that flag (and not on ses->chan_max, for
example):

         if (ses->server->dialect >= SMB30_PROT_ID &&
             (ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
                 /* schedule query interfaces poll */
                 queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
                                    (SMB_INTERFACE_POLL_INTERVAL * HZ));
	}

IMO, like I replied to Shyam, is that the worker should only run if the
user requested multichannel.

I'd suggest changing it to "if (ses->chan_max > 1) ...", and then also
add a call to queue_delayed_work() on reconfigure when chan_max changes.


Cheers,

Enzo

>On Mon, Sep 22, 2025 at 8:43 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
>> On 09/22, rajasimandalos@gmail.com wrote:
>> >From: Rajasi Mandal <rajasimandal@microsoft.com>
>> >
>> >Previously, the client did not properly update the session's channel
>> >state when multichannel or max_channels mount options were changed
>> >during remount. This led to inconsistent behavior and prevented
>> >enabling or disabling multichannel support without a full
>> >unmount/remount.
>> >
>> >Enable dynamic reconfiguration of multichannel and max_channels
>> >options during remount by introducing smb3_sync_ses_chan_max() to
>> >safely update the session's chan_max field, and smb3_sync_ses_channels()
>> >to synchronize the session's channels with the new configuration.
>> >Replace cifs_disable_secondary_channels() with
>> >cifs_decrease_secondary_channels(), which now takes a from_reconfigure
>> >argument for more flexible channel cleanup. Update the remount logic
>> >to detect changes in multichannel or max_channels and trigger the
>> >appropriate session/channel updates.
>> >
>> >With this change, users can safely change multichannel and
>> >max_channels options on remount, and the client will correctly adjust
>> >the session's channel state to match the new configuration.
>> >
>> >Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>> >---
>> > fs/smb/client/cifsproto.h  |  2 +-
>> > fs/smb/client/fs_context.c | 29 ++++++++++++++++++
>> > fs/smb/client/fs_context.h |  2 +-
>> > fs/smb/client/sess.c       | 35 +++++++++++++++-------
>> > fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
>> > fs/smb/client/smb2pdu.h    |  2 ++
>> > 6 files changed, 105 insertions(+), 25 deletions(-)
>>
>> I think the fix is necessary, and the implementation works (at least
>> with the simple case I tested).  I just think we now have too many
>> functions related to channel adding/removing/updating and they're all
>> too similar.  IMHO they could all be merged into a single "update
>> channels" one.
>>
>> Do you think it's possible to do that?  Probably would require a bit
>> more work, but at least we would end up with a centralized place to deal
>> with channel management.
>>
>> >diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> >index e8fba98690ce..ec3118457b26 100644
>> >--- a/fs/smb/client/cifsproto.h
>> >+++ b/fs/smb/client/cifsproto.h
>> >@@ -667,7 +667,7 @@ bool
>> > cifs_chan_is_iface_active(struct cifs_ses *ses,
>> >                         struct TCP_Server_Info *server);
>> > void
>> >-cifs_disable_secondary_channels(struct cifs_ses *ses);
>> >+cifs_decrease_secondary_channels(struct cifs_ses *ses, bool
>> from_reconfigure);
>> > void
>> > cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info
>> *server);
>> > int
>> >diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> >index 43552b44f613..96e80c70f25d 100644
>> >--- a/fs/smb/client/fs_context.c
>> >+++ b/fs/smb/client/fs_context.c
>> >@@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct
>> cifs_sb_info *cifs_sb, struct cifs_se
>> >       return 0;
>> > }
>> >
>> >+/**
>> >+ * smb3_sync_ses_chan_max - Synchronize the session's maximum channel
>> count
>> >+ * @ses: pointer to the old CIFS session structure
>> >+ * @max_channels: new maximum number of channels to allow
>> >+ *
>> >+ * Updates the session's chan_max field to the new value, protecting the
>> update
>> >+ * with the session's channel lock. This should be called whenever the
>> maximum
>> >+ * allowed channels for a session changes (e.g., after a remount or
>> reconfigure).
>> >+ */
>> >+void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int
>> max_channels)
>> >+{
>> >+      spin_lock(&ses->chan_lock);
>> >+      ses->chan_max = max_channels;
>> >+      spin_unlock(&ses->chan_lock);
>> >+}
>> >+
>> > static int smb3_reconfigure(struct fs_context *fc)
>> > {
>> >       struct smb3_fs_context *ctx = smb3_fc2context(fc);
>> >@@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
>> >               ses->password2 = new_password2;
>> >       }
>> >
>> >+      /*
>> >+       * If multichannel or max_channels has changed, update the
>> session's channels accordingly.
>> >+       * This may add or remove channels to match the new configuration.
>> >+       */
>> >+      if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
>> >+              (ctx->max_channels != cifs_sb->ctx->max_channels)) {
>> >+              //Synchronize ses->chan_max with the new mount context
>> >+              smb3_sync_ses_chan_max(ses, ctx->max_channels);
>> >+              //Now update the session's channels to match the new
>> configuration
>> >+              rc = smb3_sync_ses_channels(cifs_sb);
>> >+      }
>> >+
>> >       mutex_unlock(&ses->session_mutex);
>> >
>> >       STEAL_STRING(cifs_sb, ctx, domainname);
>> >@@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
>> >       smb3_cleanup_fs_context_contents(cifs_sb->ctx);
>> >       rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
>> >       smb3_update_mnt_flags(cifs_sb);
>> >+
>> > #ifdef CONFIG_CIFS_DFS_UPCALL
>> >       if (!rc)
>> >               rc = dfs_cache_remount_fs(cifs_sb);
>> >diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
>> >index b0fec6b9a23b..a75185858285 100644
>> >--- a/fs/smb/client/fs_context.h
>> >+++ b/fs/smb/client/fs_context.h
>> >@@ -371,7 +371,7 @@ static inline struct smb3_fs_context
>> *smb3_fc2context(const struct fs_context *f
>> > extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct
>> smb3_fs_context *ctx);
>> > extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb,
>> struct cifs_ses *ses);
>> > extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
>> >-
>> >+extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int
>> max_channels);
>> > /*
>> >  * max deferred close timeout (jiffies) - 2^30
>> >  */
>> >diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
>> >index 0a8c2fcc9ded..42b5481c884a 100644
>> >--- a/fs/smb/client/sess.c
>> >+++ b/fs/smb/client/sess.c
>> >@@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
>> >       return new_chan_count - old_chan_count;
>> > }
>> >
>> >-/*
>> >- * called when multichannel is disabled by the server.
>> >- * this always gets called from smb2_reconnect
>> >- * and cannot get called in parallel threads.
>> >+/**
>> >+ * cifs_decrease_secondary_channels - Reduce the number of active
>> secondary channels
>> >+ * @ses: pointer to the CIFS session structure
>> >+ * @from_reconfigure: if true, only reduce to chan_max; if false, reduce
>> to a single channel
>> >+ *
>> >+ * This function disables and cleans up extra secondary channels for a
>> CIFS session.
>> >+ * If called during reconfiguration, it reduces the channel count to the
>> new maximum (chan_max).
>> >+ * Otherwise, it disables all but the primary channel.
>> >  */
>> >-void
>> >-cifs_disable_secondary_channels(struct cifs_ses *ses)
>> >+void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool
>> from_reconfigure)
>> > {
>> >       int i, chan_count;
>> >       struct TCP_Server_Info *server;
>> >@@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses
>> *ses)
>> >       if (chan_count == 1)
>> >               goto done;
>> >
>> >-      ses->chan_count = 1;
>> >-
>> >-      /* for all secondary channels reset the need reconnect bit */
>> >-      ses->chans_need_reconnect &= 1;
>> >+      // Update the chan_count to the new maximum
>> >+      if (from_reconfigure)
>> >+              ses->chan_count = ses->chan_max;
>> >+      else
>> >+              ses->chan_count = 1;
>> >
>> >-      for (i = 1; i < chan_count; i++) {
>> >+      for (i = ses->chan_max ; i < chan_count; i++) {
>> >               iface = ses->chans[i].iface;
>> >               server = ses->chans[i].server;
>> >
>> >@@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>> >               spin_lock(&ses->chan_lock);
>> >       }
>> >
>> >+      /* For extra secondary channels, reset the need reconnect bit */
>> >+      if (ses->chan_count == 1) {
>> >+              cifs_server_dbg(VFS, "server does not support multichannel
>> anymore. Disable all other channels\n");
>> >+              ses->chans_need_reconnect &= 1;
>> >+      } else {
>> >+              cifs_server_dbg(VFS, "Disable extra secondary channels\n");
>> >+              ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
>> >+      }
>> >+
>> > done:
>> >       spin_unlock(&ses->chan_lock);
>> > }
>> >diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
>> >index c3b9d3f6210f..bf9a8dc0e8fc 100644
>> >--- a/fs/smb/client/smb2pdu.c
>> >+++ b/fs/smb/client/smb2pdu.c
>> >@@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16
>> smb2_cmd,
>> > static int
>> > cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >                         struct TCP_Server_Info *server,
>> >-                        bool from_reconnect)
>> >+                        bool from_reconnect, bool from_reconfigure)
>> > {
>> >       struct TCP_Server_Info *pserver;
>> >       unsigned int chan_index;
>> >@@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >               return -EHOSTDOWN;
>> >       }
>> >
>> >-      cifs_server_dbg(VFS,
>> >-              "server does not support multichannel anymore. Disable all
>> other channels\n");
>> >-      cifs_disable_secondary_channels(ses);
>> >+      cifs_decrease_secondary_channels(ses, from_reconfigure);
>> >
>> >+      return 0;
>> >+}
>> >+
>> >+/**
>> >+ * smb3_sync_ses_channels - Synchronize session channels
>> >+ * with new configuration (cifs_sb_info version)
>> >+ * @cifs_sb: pointer to the CIFS superblock info structure
>> >+ * Returns 0 on success or -EINVAL if scaling is already in progress.
>> >+ */
>> >+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
>> >+{
>> >+      struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
>> >+      struct smb3_fs_context *ctx = cifs_sb->ctx;
>> >+      bool from_reconnect = false;
>> >+
>> >+      /* Prevent concurrent scaling operations */
>> >+      spin_lock(&ses->ses_lock);
>> >+      if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
>> >+              spin_unlock(&ses->ses_lock);
>> >+              return -EINVAL;
>> >+      }
>> >+      ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
>> >+      spin_unlock(&ses->ses_lock);
>> >+
>> >+      /*
>> >+       * If the old max_channels is less than the new chan_max,
>> >+       * try to add channels to reach the new maximum.
>> >+       * Otherwise, disable or skip extra channels to match the new
>> configuration.
>> >+       */
>> >+      if (ctx->max_channels < ses->chan_max) {
>> >+              mutex_unlock(&ses->session_mutex);
>> >+              cifs_try_adding_channels(ses);
>> >+              mutex_lock(&ses->session_mutex);
>> >+      } else {
>> >+              cifs_chan_skip_or_disable(ses, ses->server,
>> from_reconnect, true);
>> >+      }
>> >+
>> >+      /* Clear scaling flag after operation */
>> >+      spin_lock(&ses->ses_lock);
>> >+      ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>> >+      spin_unlock(&ses->ses_lock);
>> >
>> >       return 0;
>> > }
>> >@@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon
>> *tcon,
>> >       if (ses->chan_count > 1 &&
>> >           !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>> >               rc = cifs_chan_skip_or_disable(ses, server,
>> >-                                             from_reconnect);
>> >+                                             from_reconnect, false);
>> >               if (rc) {
>> >                       mutex_unlock(&ses->session_mutex);
>> >                       goto out;
>> >@@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon
>> *tcon,
>> >                        */
>> >
>> >                       rc = cifs_chan_skip_or_disable(ses, server,
>> >-                                                     from_reconnect);
>> >+                                                     from_reconnect,
>> false);
>> >                       goto skip_add_channels;
>> >               } else if (rc)
>> >                       cifs_dbg(FYI, "%s: failed to query server
>> interfaces: %d\n",
>>
>> (for all hunks above) Can we stick to /* */ comments instead of //
>> please?
>>
>> >@@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
>> >               req->SecurityMode = 0;
>> >
>> >       req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>> >-      if (ses->chan_max > 1)
>> >-              req->Capabilities |=
>> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >+      req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >
>> >       /* ClientGUID must be zero for SMB2.02 dialect */
>> >       if (server->vals->protocol_id == SMB20_PROT_ID)
>> >@@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int
>> xid, struct cifs_tcon *tcon)
>> >       if (!pneg_inbuf)
>> >               return -ENOMEM;
>> >
>> >-      pneg_inbuf->Capabilities =
>> >-                      cpu_to_le32(server->vals->req_capabilities);
>> >-      if (tcon->ses->chan_max > 1)
>> >-              pneg_inbuf->Capabilities |=
>> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >+      pneg_inbuf->Capabilities =
>> cpu_to_le32(server->vals->req_capabilities);
>> >+      pneg_inbuf->Capabilities |=
>> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>>
>> This effectively makes query_interfaces worker run even with
>> max_channels=1 -- just a heads up because it didn't look like your
>> original intention.
>>
>>
>> Cheers,
>>
>> Enzo
>>

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Enzo Matsumiya]

On 09/23, Steve French wrote:
>> IMO, like I replied to Shyam, is that the worker should only run if the user
>requested multichannel.
>
>I would like it (query interfaces) to run at least once (at mount time )so
>debug data is populated properly for user, but don't mind either way about
>periodic queries after mount or not eg if it is run periodically every few
>hours would be no performance issue, but slight value for debugging and
>fail over eg if interface 1 is mounted but doesn't become available in a
>few hours and causes reconnect which fails, client might be able to use
>information from query interfaces to reconnect to interface 2?!

This goes completely against to what a user requested.

Why would you care about debugging multichannel or concerned about
connecting to another interface if you didn't even request multichannel
in first place?

Sure we can automate/autodetect things, but we should do so according to
what the user requested.

And if we want to do multichannel debugging, it should be
created/enhanced in cifs_debug.c

>Thanks,
>
>Steve
>
>On Tue, Sep 23, 2025, 12:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
>> On 09/23, RAJASI MANDAL wrote:
>> >> (for all hunks above) Can we stick to /* */ comments instead of //
>> >please?
>> >
>> >Yes sure I will do that.
>> >
>> >> This effectively makes query_interfaces worker run even with
>> >max_channels=1 -- just a heads up because it didn't look like your
>> >original intention.
>> >
>> >This change ensures that the multichannel option can be enabled during
>> >remounts, even if it wasn't specified during the initial mount.
>> >During the initial mount, if the user does not pass the multichannel
>> >option, the client does not advertise multichannel capability to the
>> >server.
>> >Since remounts do not trigger a fresh negotiation with the server, it's
>> not
>> >possible to enable multichannel at that stage if it was previously
>> omitted.
>> >
>> >To address this, the client is modified to *advertise multichannel
>> >capability by default*, regardless of whether the option was explicitly
>> >provided during the initial mount.
>>
>> Yes, and I agree with that change.
>>
>> I was just mentioning that because cifs_get_tcon() starts the
>> query_interface worker based on that flag (and not on ses->chan_max, for
>> example):
>>
>>          if (ses->server->dialect >= SMB30_PROT_ID &&
>>              (ses->server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>>                  /* schedule query interfaces poll */
>>                  queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
>>                                     (SMB_INTERFACE_POLL_INTERVAL * HZ));
>>         }
>>
>> IMO, like I replied to Shyam, is that the worker should only run if the
>> user requested multichannel.
>>
>> I'd suggest changing it to "if (ses->chan_max > 1) ...", and then also
>> add a call to queue_delayed_work() on reconfigure when chan_max changes.
>>
>>
>> Cheers,
>>
>> Enzo
>>
>> >On Mon, Sep 22, 2025 at 8:43 PM Enzo Matsumiya <ematsumiya@suse.de>
>> wrote:
>> >
>> >> On 09/22, rajasimandalos@gmail.com wrote:
>> >> >From: Rajasi Mandal <rajasimandal@microsoft.com>
>> >> >
>> >> >Previously, the client did not properly update the session's channel
>> >> >state when multichannel or max_channels mount options were changed
>> >> >during remount. This led to inconsistent behavior and prevented
>> >> >enabling or disabling multichannel support without a full
>> >> >unmount/remount.
>> >> >
>> >> >Enable dynamic reconfiguration of multichannel and max_channels
>> >> >options during remount by introducing smb3_sync_ses_chan_max() to
>> >> >safely update the session's chan_max field, and
>> smb3_sync_ses_channels()
>> >> >to synchronize the session's channels with the new configuration.
>> >> >Replace cifs_disable_secondary_channels() with
>> >> >cifs_decrease_secondary_channels(), which now takes a from_reconfigure
>> >> >argument for more flexible channel cleanup. Update the remount logic
>> >> >to detect changes in multichannel or max_channels and trigger the
>> >> >appropriate session/channel updates.
>> >> >
>> >> >With this change, users can safely change multichannel and
>> >> >max_channels options on remount, and the client will correctly adjust
>> >> >the session's channel state to match the new configuration.
>> >> >
>> >> >Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>> >> >---
>> >> > fs/smb/client/cifsproto.h  |  2 +-
>> >> > fs/smb/client/fs_context.c | 29 ++++++++++++++++++
>> >> > fs/smb/client/fs_context.h |  2 +-
>> >> > fs/smb/client/sess.c       | 35 +++++++++++++++-------
>> >> > fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
>> >> > fs/smb/client/smb2pdu.h    |  2 ++
>> >> > 6 files changed, 105 insertions(+), 25 deletions(-)
>> >>
>> >> I think the fix is necessary, and the implementation works (at least
>> >> with the simple case I tested).  I just think we now have too many
>> >> functions related to channel adding/removing/updating and they're all
>> >> too similar.  IMHO they could all be merged into a single "update
>> >> channels" one.
>> >>
>> >> Do you think it's possible to do that?  Probably would require a bit
>> >> more work, but at least we would end up with a centralized place to deal
>> >> with channel management.
>> >>
>> >> >diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> >> >index e8fba98690ce..ec3118457b26 100644
>> >> >--- a/fs/smb/client/cifsproto.h
>> >> >+++ b/fs/smb/client/cifsproto.h
>> >> >@@ -667,7 +667,7 @@ bool
>> >> > cifs_chan_is_iface_active(struct cifs_ses *ses,
>> >> >                         struct TCP_Server_Info *server);
>> >> > void
>> >> >-cifs_disable_secondary_channels(struct cifs_ses *ses);
>> >> >+cifs_decrease_secondary_channels(struct cifs_ses *ses, bool
>> >> from_reconfigure);
>> >> > void
>> >> > cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info
>> >> *server);
>> >> > int
>> >> >diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> >> >index 43552b44f613..96e80c70f25d 100644
>> >> >--- a/fs/smb/client/fs_context.c
>> >> >+++ b/fs/smb/client/fs_context.c
>> >> >@@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct
>> >> cifs_sb_info *cifs_sb, struct cifs_se
>> >> >       return 0;
>> >> > }
>> >> >
>> >> >+/**
>> >> >+ * smb3_sync_ses_chan_max - Synchronize the session's maximum channel
>> >> count
>> >> >+ * @ses: pointer to the old CIFS session structure
>> >> >+ * @max_channels: new maximum number of channels to allow
>> >> >+ *
>> >> >+ * Updates the session's chan_max field to the new value, protecting
>> the
>> >> update
>> >> >+ * with the session's channel lock. This should be called whenever the
>> >> maximum
>> >> >+ * allowed channels for a session changes (e.g., after a remount or
>> >> reconfigure).
>> >> >+ */
>> >> >+void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int
>> >> max_channels)
>> >> >+{
>> >> >+      spin_lock(&ses->chan_lock);
>> >> >+      ses->chan_max = max_channels;
>> >> >+      spin_unlock(&ses->chan_lock);
>> >> >+}
>> >> >+
>> >> > static int smb3_reconfigure(struct fs_context *fc)
>> >> > {
>> >> >       struct smb3_fs_context *ctx = smb3_fc2context(fc);
>> >> >@@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context
>> *fc)
>> >> >               ses->password2 = new_password2;
>> >> >       }
>> >> >
>> >> >+      /*
>> >> >+       * If multichannel or max_channels has changed, update the
>> >> session's channels accordingly.
>> >> >+       * This may add or remove channels to match the new
>> configuration.
>> >> >+       */
>> >> >+      if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
>> >> >+              (ctx->max_channels != cifs_sb->ctx->max_channels)) {
>> >> >+              //Synchronize ses->chan_max with the new mount context
>> >> >+              smb3_sync_ses_chan_max(ses, ctx->max_channels);
>> >> >+              //Now update the session's channels to match the new
>> >> configuration
>> >> >+              rc = smb3_sync_ses_channels(cifs_sb);
>> >> >+      }
>> >> >+
>> >> >       mutex_unlock(&ses->session_mutex);
>> >> >
>> >> >       STEAL_STRING(cifs_sb, ctx, domainname);
>> >> >@@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context
>> *fc)
>> >> >       smb3_cleanup_fs_context_contents(cifs_sb->ctx);
>> >> >       rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
>> >> >       smb3_update_mnt_flags(cifs_sb);
>> >> >+
>> >> > #ifdef CONFIG_CIFS_DFS_UPCALL
>> >> >       if (!rc)
>> >> >               rc = dfs_cache_remount_fs(cifs_sb);
>> >> >diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
>> >> >index b0fec6b9a23b..a75185858285 100644
>> >> >--- a/fs/smb/client/fs_context.h
>> >> >+++ b/fs/smb/client/fs_context.h
>> >> >@@ -371,7 +371,7 @@ static inline struct smb3_fs_context
>> >> *smb3_fc2context(const struct fs_context *f
>> >> > extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct
>> >> smb3_fs_context *ctx);
>> >> > extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info
>> *cifs_sb,
>> >> struct cifs_ses *ses);
>> >> > extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
>> >> >-
>> >> >+extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int
>> >> max_channels);
>> >> > /*
>> >> >  * max deferred close timeout (jiffies) - 2^30
>> >> >  */
>> >> >diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
>> >> >index 0a8c2fcc9ded..42b5481c884a 100644
>> >> >--- a/fs/smb/client/sess.c
>> >> >+++ b/fs/smb/client/sess.c
>> >> >@@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses
>> *ses)
>> >> >       return new_chan_count - old_chan_count;
>> >> > }
>> >> >
>> >> >-/*
>> >> >- * called when multichannel is disabled by the server.
>> >> >- * this always gets called from smb2_reconnect
>> >> >- * and cannot get called in parallel threads.
>> >> >+/**
>> >> >+ * cifs_decrease_secondary_channels - Reduce the number of active
>> >> secondary channels
>> >> >+ * @ses: pointer to the CIFS session structure
>> >> >+ * @from_reconfigure: if true, only reduce to chan_max; if false,
>> reduce
>> >> to a single channel
>> >> >+ *
>> >> >+ * This function disables and cleans up extra secondary channels for a
>> >> CIFS session.
>> >> >+ * If called during reconfiguration, it reduces the channel count to
>> the
>> >> new maximum (chan_max).
>> >> >+ * Otherwise, it disables all but the primary channel.
>> >> >  */
>> >> >-void
>> >> >-cifs_disable_secondary_channels(struct cifs_ses *ses)
>> >> >+void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool
>> >> from_reconfigure)
>> >> > {
>> >> >       int i, chan_count;
>> >> >       struct TCP_Server_Info *server;
>> >> >@@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses
>> >> *ses)
>> >> >       if (chan_count == 1)
>> >> >               goto done;
>> >> >
>> >> >-      ses->chan_count = 1;
>> >> >-
>> >> >-      /* for all secondary channels reset the need reconnect bit */
>> >> >-      ses->chans_need_reconnect &= 1;
>> >> >+      // Update the chan_count to the new maximum
>> >> >+      if (from_reconfigure)
>> >> >+              ses->chan_count = ses->chan_max;
>> >> >+      else
>> >> >+              ses->chan_count = 1;
>> >> >
>> >> >-      for (i = 1; i < chan_count; i++) {
>> >> >+      for (i = ses->chan_max ; i < chan_count; i++) {
>> >> >               iface = ses->chans[i].iface;
>> >> >               server = ses->chans[i].server;
>> >> >
>> >> >@@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses
>> *ses)
>> >> >               spin_lock(&ses->chan_lock);
>> >> >       }
>> >> >
>> >> >+      /* For extra secondary channels, reset the need reconnect bit */
>> >> >+      if (ses->chan_count == 1) {
>> >> >+              cifs_server_dbg(VFS, "server does not support
>> multichannel
>> >> anymore. Disable all other channels\n");
>> >> >+              ses->chans_need_reconnect &= 1;
>> >> >+      } else {
>> >> >+              cifs_server_dbg(VFS, "Disable extra secondary
>> channels\n");
>> >> >+              ses->chans_need_reconnect &= ((1UL << ses->chan_max) -
>> 1);
>> >> >+      }
>> >> >+
>> >> > done:
>> >> >       spin_unlock(&ses->chan_lock);
>> >> > }
>> >> >diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
>> >> >index c3b9d3f6210f..bf9a8dc0e8fc 100644
>> >> >--- a/fs/smb/client/smb2pdu.c
>> >> >+++ b/fs/smb/client/smb2pdu.c
>> >> >@@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16
>> >> smb2_cmd,
>> >> > static int
>> >> > cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >> >                         struct TCP_Server_Info *server,
>> >> >-                        bool from_reconnect)
>> >> >+                        bool from_reconnect, bool from_reconfigure)
>> >> > {
>> >> >       struct TCP_Server_Info *pserver;
>> >> >       unsigned int chan_index;
>> >> >@@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >> >               return -EHOSTDOWN;
>> >> >       }
>> >> >
>> >> >-      cifs_server_dbg(VFS,
>> >> >-              "server does not support multichannel anymore. Disable
>> all
>> >> other channels\n");
>> >> >-      cifs_disable_secondary_channels(ses);
>> >> >+      cifs_decrease_secondary_channels(ses, from_reconfigure);
>> >> >
>> >> >+      return 0;
>> >> >+}
>> >> >+
>> >> >+/**
>> >> >+ * smb3_sync_ses_channels - Synchronize session channels
>> >> >+ * with new configuration (cifs_sb_info version)
>> >> >+ * @cifs_sb: pointer to the CIFS superblock info structure
>> >> >+ * Returns 0 on success or -EINVAL if scaling is already in progress.
>> >> >+ */
>> >> >+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
>> >> >+{
>> >> >+      struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
>> >> >+      struct smb3_fs_context *ctx = cifs_sb->ctx;
>> >> >+      bool from_reconnect = false;
>> >> >+
>> >> >+      /* Prevent concurrent scaling operations */
>> >> >+      spin_lock(&ses->ses_lock);
>> >> >+      if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
>> >> >+              spin_unlock(&ses->ses_lock);
>> >> >+              return -EINVAL;
>> >> >+      }
>> >> >+      ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
>> >> >+      spin_unlock(&ses->ses_lock);
>> >> >+
>> >> >+      /*
>> >> >+       * If the old max_channels is less than the new chan_max,
>> >> >+       * try to add channels to reach the new maximum.
>> >> >+       * Otherwise, disable or skip extra channels to match the new
>> >> configuration.
>> >> >+       */
>> >> >+      if (ctx->max_channels < ses->chan_max) {
>> >> >+              mutex_unlock(&ses->session_mutex);
>> >> >+              cifs_try_adding_channels(ses);
>> >> >+              mutex_lock(&ses->session_mutex);
>> >> >+      } else {
>> >> >+              cifs_chan_skip_or_disable(ses, ses->server,
>> >> from_reconnect, true);
>> >> >+      }
>> >> >+
>> >> >+      /* Clear scaling flag after operation */
>> >> >+      spin_lock(&ses->ses_lock);
>> >> >+      ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>> >> >+      spin_unlock(&ses->ses_lock);
>> >> >
>> >> >       return 0;
>> >> > }
>> >> >@@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct
>> cifs_tcon
>> >> *tcon,
>> >> >       if (ses->chan_count > 1 &&
>> >> >           !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>> >> >               rc = cifs_chan_skip_or_disable(ses, server,
>> >> >-                                             from_reconnect);
>> >> >+                                             from_reconnect, false);
>> >> >               if (rc) {
>> >> >                       mutex_unlock(&ses->session_mutex);
>> >> >                       goto out;
>> >> >@@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct
>> cifs_tcon
>> >> *tcon,
>> >> >                        */
>> >> >
>> >> >                       rc = cifs_chan_skip_or_disable(ses, server,
>> >> >-                                                     from_reconnect);
>> >> >+                                                     from_reconnect,
>> >> false);
>> >> >                       goto skip_add_channels;
>> >> >               } else if (rc)
>> >> >                       cifs_dbg(FYI, "%s: failed to query server
>> >> interfaces: %d\n",
>> >>
>> >> (for all hunks above) Can we stick to /* */ comments instead of //
>> >> please?
>> >>
>> >> >@@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
>> >> >               req->SecurityMode = 0;
>> >> >
>> >> >       req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>> >> >-      if (ses->chan_max > 1)
>> >> >-              req->Capabilities |=
>> >> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >+      req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >
>> >> >       /* ClientGUID must be zero for SMB2.02 dialect */
>> >> >       if (server->vals->protocol_id == SMB20_PROT_ID)
>> >> >@@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int
>> >> xid, struct cifs_tcon *tcon)
>> >> >       if (!pneg_inbuf)
>> >> >               return -ENOMEM;
>> >> >
>> >> >-      pneg_inbuf->Capabilities =
>> >> >-                      cpu_to_le32(server->vals->req_capabilities);
>> >> >-      if (tcon->ses->chan_max > 1)
>> >> >-              pneg_inbuf->Capabilities |=
>> >> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >+      pneg_inbuf->Capabilities =
>> >> cpu_to_le32(server->vals->req_capabilities);
>> >> >+      pneg_inbuf->Capabilities |=
>> >> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >>
>> >> This effectively makes query_interfaces worker run even with
>> >> max_channels=1 -- just a heads up because it didn't look like your
>> >> original intention.
>> >>
>> >>
>> >> Cheers,
>> >>
>> >> Enzo
>> >>
>>
>>

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Enzo Matsumiya]

On 09/23, Steve French wrote:
>The interface info in debug data that we get at mount time I have found
>useful for debugging non multichannel cases. I don't have strong opinions
>about whether requerying it occasionally in non multichannel case is of
>value but clearly has been useful to get it at mount time to populate Debug
>data

Steve, sorry, are you reading what I'm proposing/saying here?

mount.cifs -o multichannel,...
-> fills debug data, starts query_interface, try/add chans, yadda yadda


mount.cifs -o nomultichannel,...
-> do nothing related to multichannel


What's so wrong/problematic with having it that way?

>Thanks,
>
>Steve
>
>On Tue, Sep 23, 2025, 1:01 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
>> On 09/23, Steve French wrote:
>> >> IMO, like I replied to Shyam, is that the worker should only run if the
>> user
>> >requested multichannel.
>> >
>> >I would like it (query interfaces) to run at least once (at mount time )so
>> >debug data is populated properly for user, but don't mind either way about
>> >periodic queries after mount or not eg if it is run periodically every few
>> >hours would be no performance issue, but slight value for debugging and
>> >fail over eg if interface 1 is mounted but doesn't become available in a
>> >few hours and causes reconnect which fails, client might be able to use
>> >information from query interfaces to reconnect to interface 2?!
>>
>> This goes completely against to what a user requested.
>>
>> Why would you care about debugging multichannel or concerned about
>> connecting to another interface if you didn't even request multichannel
>> in first place?
>>
>> Sure we can automate/autodetect things, but we should do so according to
>> what the user requested.
>>
>> And if we want to do multichannel debugging, it should be
>> created/enhanced in cifs_debug.c
>>
>> >Thanks,
>> >
>> >Steve
>> >
>> >On Tue, Sep 23, 2025, 12:32 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>> >
>> >> On 09/23, RAJASI MANDAL wrote:
>> >> >> (for all hunks above) Can we stick to /* */ comments instead of //
>> >> >please?
>> >> >
>> >> >Yes sure I will do that.
>> >> >
>> >> >> This effectively makes query_interfaces worker run even with
>> >> >max_channels=1 -- just a heads up because it didn't look like your
>> >> >original intention.
>> >> >
>> >> >This change ensures that the multichannel option can be enabled during
>> >> >remounts, even if it wasn't specified during the initial mount.
>> >> >During the initial mount, if the user does not pass the multichannel
>> >> >option, the client does not advertise multichannel capability to the
>> >> >server.
>> >> >Since remounts do not trigger a fresh negotiation with the server, it's
>> >> not
>> >> >possible to enable multichannel at that stage if it was previously
>> >> omitted.
>> >> >
>> >> >To address this, the client is modified to *advertise multichannel
>> >> >capability by default*, regardless of whether the option was explicitly
>> >> >provided during the initial mount.
>> >>
>> >> Yes, and I agree with that change.
>> >>
>> >> I was just mentioning that because cifs_get_tcon() starts the
>> >> query_interface worker based on that flag (and not on ses->chan_max, for
>> >> example):
>> >>
>> >>          if (ses->server->dialect >= SMB30_PROT_ID &&
>> >>              (ses->server->capabilities &
>> SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>> >>                  /* schedule query interfaces poll */
>> >>                  queue_delayed_work(cifsiod_wq, &tcon->query_interfaces,
>> >>                                     (SMB_INTERFACE_POLL_INTERVAL * HZ));
>> >>         }
>> >>
>> >> IMO, like I replied to Shyam, is that the worker should only run if the
>> >> user requested multichannel.
>> >>
>> >> I'd suggest changing it to "if (ses->chan_max > 1) ...", and then also
>> >> add a call to queue_delayed_work() on reconfigure when chan_max changes.
>> >>
>> >>
>> >> Cheers,
>> >>
>> >> Enzo
>> >>
>> >> >On Mon, Sep 22, 2025 at 8:43 PM Enzo Matsumiya <ematsumiya@suse.de>
>> >> wrote:
>> >> >
>> >> >> On 09/22, rajasimandalos@gmail.com wrote:
>> >> >> >From: Rajasi Mandal <rajasimandal@microsoft.com>
>> >> >> >
>> >> >> >Previously, the client did not properly update the session's channel
>> >> >> >state when multichannel or max_channels mount options were changed
>> >> >> >during remount. This led to inconsistent behavior and prevented
>> >> >> >enabling or disabling multichannel support without a full
>> >> >> >unmount/remount.
>> >> >> >
>> >> >> >Enable dynamic reconfiguration of multichannel and max_channels
>> >> >> >options during remount by introducing smb3_sync_ses_chan_max() to
>> >> >> >safely update the session's chan_max field, and
>> >> smb3_sync_ses_channels()
>> >> >> >to synchronize the session's channels with the new configuration.
>> >> >> >Replace cifs_disable_secondary_channels() with
>> >> >> >cifs_decrease_secondary_channels(), which now takes a
>> from_reconfigure
>> >> >> >argument for more flexible channel cleanup. Update the remount logic
>> >> >> >to detect changes in multichannel or max_channels and trigger the
>> >> >> >appropriate session/channel updates.
>> >> >> >
>> >> >> >With this change, users can safely change multichannel and
>> >> >> >max_channels options on remount, and the client will correctly
>> adjust
>> >> >> >the session's channel state to match the new configuration.
>> >> >> >
>> >> >> >Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>> >> >> >---
>> >> >> > fs/smb/client/cifsproto.h  |  2 +-
>> >> >> > fs/smb/client/fs_context.c | 29 ++++++++++++++++++
>> >> >> > fs/smb/client/fs_context.h |  2 +-
>> >> >> > fs/smb/client/sess.c       | 35 +++++++++++++++-------
>> >> >> > fs/smb/client/smb2pdu.c    | 60
>> ++++++++++++++++++++++++++++++--------
>> >> >> > fs/smb/client/smb2pdu.h    |  2 ++
>> >> >> > 6 files changed, 105 insertions(+), 25 deletions(-)
>> >> >>
>> >> >> I think the fix is necessary, and the implementation works (at least
>> >> >> with the simple case I tested).  I just think we now have too many
>> >> >> functions related to channel adding/removing/updating and they're all
>> >> >> too similar.  IMHO they could all be merged into a single "update
>> >> >> channels" one.
>> >> >>
>> >> >> Do you think it's possible to do that?  Probably would require a bit
>> >> >> more work, but at least we would end up with a centralized place to
>> deal
>> >> >> with channel management.
>> >> >>
>> >> >> >diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> >> >> >index e8fba98690ce..ec3118457b26 100644
>> >> >> >--- a/fs/smb/client/cifsproto.h
>> >> >> >+++ b/fs/smb/client/cifsproto.h
>> >> >> >@@ -667,7 +667,7 @@ bool
>> >> >> > cifs_chan_is_iface_active(struct cifs_ses *ses,
>> >> >> >                         struct TCP_Server_Info *server);
>> >> >> > void
>> >> >> >-cifs_disable_secondary_channels(struct cifs_ses *ses);
>> >> >> >+cifs_decrease_secondary_channels(struct cifs_ses *ses, bool
>> >> >> from_reconfigure);
>> >> >> > void
>> >> >> > cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info
>> >> >> *server);
>> >> >> > int
>> >> >> >diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> >> >> >index 43552b44f613..96e80c70f25d 100644
>> >> >> >--- a/fs/smb/client/fs_context.c
>> >> >> >+++ b/fs/smb/client/fs_context.c
>> >> >> >@@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct
>> >> >> cifs_sb_info *cifs_sb, struct cifs_se
>> >> >> >       return 0;
>> >> >> > }
>> >> >> >
>> >> >> >+/**
>> >> >> >+ * smb3_sync_ses_chan_max - Synchronize the session's maximum
>> channel
>> >> >> count
>> >> >> >+ * @ses: pointer to the old CIFS session structure
>> >> >> >+ * @max_channels: new maximum number of channels to allow
>> >> >> >+ *
>> >> >> >+ * Updates the session's chan_max field to the new value,
>> protecting
>> >> the
>> >> >> update
>> >> >> >+ * with the session's channel lock. This should be called whenever
>> the
>> >> >> maximum
>> >> >> >+ * allowed channels for a session changes (e.g., after a remount or
>> >> >> reconfigure).
>> >> >> >+ */
>> >> >> >+void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int
>> >> >> max_channels)
>> >> >> >+{
>> >> >> >+      spin_lock(&ses->chan_lock);
>> >> >> >+      ses->chan_max = max_channels;
>> >> >> >+      spin_unlock(&ses->chan_lock);
>> >> >> >+}
>> >> >> >+
>> >> >> > static int smb3_reconfigure(struct fs_context *fc)
>> >> >> > {
>> >> >> >       struct smb3_fs_context *ctx = smb3_fc2context(fc);
>> >> >> >@@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context
>> >> *fc)
>> >> >> >               ses->password2 = new_password2;
>> >> >> >       }
>> >> >> >
>> >> >> >+      /*
>> >> >> >+       * If multichannel or max_channels has changed, update the
>> >> >> session's channels accordingly.
>> >> >> >+       * This may add or remove channels to match the new
>> >> configuration.
>> >> >> >+       */
>> >> >> >+      if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
>> >> >> >+              (ctx->max_channels != cifs_sb->ctx->max_channels)) {
>> >> >> >+              //Synchronize ses->chan_max with the new mount
>> context
>> >> >> >+              smb3_sync_ses_chan_max(ses, ctx->max_channels);
>> >> >> >+              //Now update the session's channels to match the new
>> >> >> configuration
>> >> >> >+              rc = smb3_sync_ses_channels(cifs_sb);
>> >> >> >+      }
>> >> >> >+
>> >> >> >       mutex_unlock(&ses->session_mutex);
>> >> >> >
>> >> >> >       STEAL_STRING(cifs_sb, ctx, domainname);
>> >> >> >@@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context
>> >> *fc)
>> >> >> >       smb3_cleanup_fs_context_contents(cifs_sb->ctx);
>> >> >> >       rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
>> >> >> >       smb3_update_mnt_flags(cifs_sb);
>> >> >> >+
>> >> >> > #ifdef CONFIG_CIFS_DFS_UPCALL
>> >> >> >       if (!rc)
>> >> >> >               rc = dfs_cache_remount_fs(cifs_sb);
>> >> >> >diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
>> >> >> >index b0fec6b9a23b..a75185858285 100644
>> >> >> >--- a/fs/smb/client/fs_context.h
>> >> >> >+++ b/fs/smb/client/fs_context.h
>> >> >> >@@ -371,7 +371,7 @@ static inline struct smb3_fs_context
>> >> >> *smb3_fc2context(const struct fs_context *f
>> >> >> > extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx,
>> struct
>> >> >> smb3_fs_context *ctx);
>> >> >> > extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info
>> >> *cifs_sb,
>> >> >> struct cifs_ses *ses);
>> >> >> > extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
>> >> >> >-
>> >> >> >+extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned
>> int
>> >> >> max_channels);
>> >> >> > /*
>> >> >> >  * max deferred close timeout (jiffies) - 2^30
>> >> >> >  */
>> >> >> >diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
>> >> >> >index 0a8c2fcc9ded..42b5481c884a 100644
>> >> >> >--- a/fs/smb/client/sess.c
>> >> >> >+++ b/fs/smb/client/sess.c
>> >> >> >@@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses
>> >> *ses)
>> >> >> >       return new_chan_count - old_chan_count;
>> >> >> > }
>> >> >> >
>> >> >> >-/*
>> >> >> >- * called when multichannel is disabled by the server.
>> >> >> >- * this always gets called from smb2_reconnect
>> >> >> >- * and cannot get called in parallel threads.
>> >> >> >+/**
>> >> >> >+ * cifs_decrease_secondary_channels - Reduce the number of active
>> >> >> secondary channels
>> >> >> >+ * @ses: pointer to the CIFS session structure
>> >> >> >+ * @from_reconfigure: if true, only reduce to chan_max; if false,
>> >> reduce
>> >> >> to a single channel
>> >> >> >+ *
>> >> >> >+ * This function disables and cleans up extra secondary channels
>> for a
>> >> >> CIFS session.
>> >> >> >+ * If called during reconfiguration, it reduces the channel count
>> to
>> >> the
>> >> >> new maximum (chan_max).
>> >> >> >+ * Otherwise, it disables all but the primary channel.
>> >> >> >  */
>> >> >> >-void
>> >> >> >-cifs_disable_secondary_channels(struct cifs_ses *ses)
>> >> >> >+void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool
>> >> >> from_reconfigure)
>> >> >> > {
>> >> >> >       int i, chan_count;
>> >> >> >       struct TCP_Server_Info *server;
>> >> >> >@@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct
>> cifs_ses
>> >> >> *ses)
>> >> >> >       if (chan_count == 1)
>> >> >> >               goto done;
>> >> >> >
>> >> >> >-      ses->chan_count = 1;
>> >> >> >-
>> >> >> >-      /* for all secondary channels reset the need reconnect bit */
>> >> >> >-      ses->chans_need_reconnect &= 1;
>> >> >> >+      // Update the chan_count to the new maximum
>> >> >> >+      if (from_reconfigure)
>> >> >> >+              ses->chan_count = ses->chan_max;
>> >> >> >+      else
>> >> >> >+              ses->chan_count = 1;
>> >> >> >
>> >> >> >-      for (i = 1; i < chan_count; i++) {
>> >> >> >+      for (i = ses->chan_max ; i < chan_count; i++) {
>> >> >> >               iface = ses->chans[i].iface;
>> >> >> >               server = ses->chans[i].server;
>> >> >> >
>> >> >> >@@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses
>> >> *ses)
>> >> >> >               spin_lock(&ses->chan_lock);
>> >> >> >       }
>> >> >> >
>> >> >> >+      /* For extra secondary channels, reset the need reconnect
>> bit */
>> >> >> >+      if (ses->chan_count == 1) {
>> >> >> >+              cifs_server_dbg(VFS, "server does not support
>> >> multichannel
>> >> >> anymore. Disable all other channels\n");
>> >> >> >+              ses->chans_need_reconnect &= 1;
>> >> >> >+      } else {
>> >> >> >+              cifs_server_dbg(VFS, "Disable extra secondary
>> >> channels\n");
>> >> >> >+              ses->chans_need_reconnect &= ((1UL << ses->chan_max)
>> -
>> >> 1);
>> >> >> >+      }
>> >> >> >+
>> >> >> > done:
>> >> >> >       spin_unlock(&ses->chan_lock);
>> >> >> > }
>> >> >> >diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
>> >> >> >index c3b9d3f6210f..bf9a8dc0e8fc 100644
>> >> >> >--- a/fs/smb/client/smb2pdu.c
>> >> >> >+++ b/fs/smb/client/smb2pdu.c
>> >> >> >@@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16
>> >> >> smb2_cmd,
>> >> >> > static int
>> >> >> > cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >> >> >                         struct TCP_Server_Info *server,
>> >> >> >-                        bool from_reconnect)
>> >> >> >+                        bool from_reconnect, bool from_reconfigure)
>> >> >> > {
>> >> >> >       struct TCP_Server_Info *pserver;
>> >> >> >       unsigned int chan_index;
>> >> >> >@@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses
>> *ses,
>> >> >> >               return -EHOSTDOWN;
>> >> >> >       }
>> >> >> >
>> >> >> >-      cifs_server_dbg(VFS,
>> >> >> >-              "server does not support multichannel anymore.
>> Disable
>> >> all
>> >> >> other channels\n");
>> >> >> >-      cifs_disable_secondary_channels(ses);
>> >> >> >+      cifs_decrease_secondary_channels(ses, from_reconfigure);
>> >> >> >
>> >> >> >+      return 0;
>> >> >> >+}
>> >> >> >+
>> >> >> >+/**
>> >> >> >+ * smb3_sync_ses_channels - Synchronize session channels
>> >> >> >+ * with new configuration (cifs_sb_info version)
>> >> >> >+ * @cifs_sb: pointer to the CIFS superblock info structure
>> >> >> >+ * Returns 0 on success or -EINVAL if scaling is already in
>> progress.
>> >> >> >+ */
>> >> >> >+int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
>> >> >> >+{
>> >> >> >+      struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
>> >> >> >+      struct smb3_fs_context *ctx = cifs_sb->ctx;
>> >> >> >+      bool from_reconnect = false;
>> >> >> >+
>> >> >> >+      /* Prevent concurrent scaling operations */
>> >> >> >+      spin_lock(&ses->ses_lock);
>> >> >> >+      if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
>> >> >> >+              spin_unlock(&ses->ses_lock);
>> >> >> >+              return -EINVAL;
>> >> >> >+      }
>> >> >> >+      ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
>> >> >> >+      spin_unlock(&ses->ses_lock);
>> >> >> >+
>> >> >> >+      /*
>> >> >> >+       * If the old max_channels is less than the new chan_max,
>> >> >> >+       * try to add channels to reach the new maximum.
>> >> >> >+       * Otherwise, disable or skip extra channels to match the new
>> >> >> configuration.
>> >> >> >+       */
>> >> >> >+      if (ctx->max_channels < ses->chan_max) {
>> >> >> >+              mutex_unlock(&ses->session_mutex);
>> >> >> >+              cifs_try_adding_channels(ses);
>> >> >> >+              mutex_lock(&ses->session_mutex);
>> >> >> >+      } else {
>> >> >> >+              cifs_chan_skip_or_disable(ses, ses->server,
>> >> >> from_reconnect, true);
>> >> >> >+      }
>> >> >> >+
>> >> >> >+      /* Clear scaling flag after operation */
>> >> >> >+      spin_lock(&ses->ses_lock);
>> >> >> >+      ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>> >> >> >+      spin_unlock(&ses->ses_lock);
>> >> >> >
>> >> >> >       return 0;
>> >> >> > }
>> >> >> >@@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct
>> >> cifs_tcon
>> >> >> *tcon,
>> >> >> >       if (ses->chan_count > 1 &&
>> >> >> >           !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL))
>> {
>> >> >> >               rc = cifs_chan_skip_or_disable(ses, server,
>> >> >> >-                                             from_reconnect);
>> >> >> >+                                             from_reconnect,
>> false);
>> >> >> >               if (rc) {
>> >> >> >                       mutex_unlock(&ses->session_mutex);
>> >> >> >                       goto out;
>> >> >> >@@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct
>> >> cifs_tcon
>> >> >> *tcon,
>> >> >> >                        */
>> >> >> >
>> >> >> >                       rc = cifs_chan_skip_or_disable(ses, server,
>> >> >> >-
>>  from_reconnect);
>> >> >> >+
>>  from_reconnect,
>> >> >> false);
>> >> >> >                       goto skip_add_channels;
>> >> >> >               } else if (rc)
>> >> >> >                       cifs_dbg(FYI, "%s: failed to query server
>> >> >> interfaces: %d\n",
>> >> >>
>> >> >> (for all hunks above) Can we stick to /* */ comments instead of //
>> >> >> please?
>> >> >>
>> >> >> >@@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
>> >> >> >               req->SecurityMode = 0;
>> >> >> >
>> >> >> >       req->Capabilities =
>> cpu_to_le32(server->vals->req_capabilities);
>> >> >> >-      if (ses->chan_max > 1)
>> >> >> >-              req->Capabilities |=
>> >> >> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >> >+      req->Capabilities |=
>> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >> >
>> >> >> >       /* ClientGUID must be zero for SMB2.02 dialect */
>> >> >> >       if (server->vals->protocol_id == SMB20_PROT_ID)
>> >> >> >@@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned
>> int
>> >> >> xid, struct cifs_tcon *tcon)
>> >> >> >       if (!pneg_inbuf)
>> >> >> >               return -ENOMEM;
>> >> >> >
>> >> >> >-      pneg_inbuf->Capabilities =
>> >> >> >-                      cpu_to_le32(server->vals->req_capabilities);
>> >> >> >-      if (tcon->ses->chan_max > 1)
>> >> >> >-              pneg_inbuf->Capabilities |=
>> >> >> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >> >+      pneg_inbuf->Capabilities =
>> >> >> cpu_to_le32(server->vals->req_capabilities);
>> >> >> >+      pneg_inbuf->Capabilities |=
>> >> >> cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >> >>
>> >> >> This effectively makes query_interfaces worker run even with
>> >> >> max_channels=1 -- just a heads up because it didn't look like your
>> >> >> original intention.
>> >> >>
>> >> >>
>> >> >> Cheers,
>> >> >>
>> >> >> Enzo
>> >> >>
>> >>
>> >>
>>

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Henrique Carvalho]

Hi Rajasi,

On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
> From: Rajasi Mandal <rajasimandal@microsoft.com>
> 
> Previously, the client did not properly update the session's channel
> state when multichannel or max_channels mount options were changed
> during remount. This led to inconsistent behavior and prevented
> enabling or disabling multichannel support without a full
> unmount/remount.
> 
> Enable dynamic reconfiguration of multichannel and max_channels
> options during remount by introducing smb3_sync_ses_chan_max() to
> safely update the session's chan_max field, and smb3_sync_ses_channels()
> to synchronize the session's channels with the new configuration.
> Replace cifs_disable_secondary_channels() with
> cifs_decrease_secondary_channels(), which now takes a from_reconfigure
> argument for more flexible channel cleanup. Update the remount logic
> to detect changes in multichannel or max_channels and trigger the
> appropriate session/channel updates.
> 
> With this change, users can safely change multichannel and
> max_channels options on remount, and the client will correctly adjust
> the session's channel state to match the new configuration.
> 
> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> ---
>  fs/smb/client/cifsproto.h  |  2 +-
>  fs/smb/client/fs_context.c | 29 ++++++++++++++++++
>  fs/smb/client/fs_context.h |  2 +-
>  fs/smb/client/sess.c       | 35 +++++++++++++++-------
>  fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
>  fs/smb/client/smb2pdu.h    |  2 ++
>  6 files changed, 105 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index e8fba98690ce..ec3118457b26 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -667,7 +667,7 @@ bool
>  cifs_chan_is_iface_active(struct cifs_ses *ses,
>  			  struct TCP_Server_Info *server);
>  void
> -cifs_disable_secondary_channels(struct cifs_ses *ses);
> +cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
>  void
>  cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
>  int
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 43552b44f613..96e80c70f25d 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
>  	return 0;
>  }
>  
> +/**
> + * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
> + * @ses: pointer to the old CIFS session structure
> + * @max_channels: new maximum number of channels to allow
> + *
> + * Updates the session's chan_max field to the new value, protecting the update
> + * with the session's channel lock. This should be called whenever the maximum
> + * allowed channels for a session changes (e.g., after a remount or reconfigure).
> + */
> +void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
> +{
> +	spin_lock(&ses->chan_lock);
> +	ses->chan_max = max_channels;
> +	spin_unlock(&ses->chan_lock);
> +}
> +

The other writer of chan_max is when creating a session. Is this lock
really avoiding a race here?

>  static int smb3_reconfigure(struct fs_context *fc)
>  {
>  	struct smb3_fs_context *ctx = smb3_fc2context(fc);
> @@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
>  		ses->password2 = new_password2;
>  	}
>  
> +	/*
> +	 * If multichannel or max_channels has changed, update the session's channels accordingly.
> +	 * This may add or remove channels to match the new configuration.
> +	 */
> +	if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
> +		(ctx->max_channels != cifs_sb->ctx->max_channels)) {
> +		//Synchronize ses->chan_max with the new mount context
> +		smb3_sync_ses_chan_max(ses, ctx->max_channels);
> +		//Now update the session's channels to match the new configuration
> +		rc = smb3_sync_ses_channels(cifs_sb);
> +	}
> +
>  	mutex_unlock(&ses->session_mutex);
>  
>  	STEAL_STRING(cifs_sb, ctx, domainname);
> @@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
>  	smb3_cleanup_fs_context_contents(cifs_sb->ctx);
>  	rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
>  	smb3_update_mnt_flags(cifs_sb);
> +
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>  	if (!rc)
>  		rc = dfs_cache_remount_fs(cifs_sb);
> diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> index b0fec6b9a23b..a75185858285 100644
> --- a/fs/smb/client/fs_context.h
> +++ b/fs/smb/client/fs_context.h
> @@ -371,7 +371,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
>  extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
>  extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
>  extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
> -
> +extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
>  /*
>   * max deferred close timeout (jiffies) - 2^30
>   */
> diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
> index 0a8c2fcc9ded..42b5481c884a 100644
> --- a/fs/smb/client/sess.c
> +++ b/fs/smb/client/sess.c
> @@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
>  	return new_chan_count - old_chan_count;
>  }
>  
> -/*
> - * called when multichannel is disabled by the server.
> - * this always gets called from smb2_reconnect
> - * and cannot get called in parallel threads.
> +/**
> + * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
> + * @ses: pointer to the CIFS session structure
> + * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
> + *
> + * This function disables and cleans up extra secondary channels for a CIFS session.
> + * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
> + * Otherwise, it disables all but the primary channel.
>   */
> -void
> -cifs_disable_secondary_channels(struct cifs_ses *ses)
> +void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
>  {

Maybe you could get rid of from_reconfigure parameter if you just set
chan_max to 1 before calling cifs_decrease_secondary_channels when this
function is not called from smb3_reconfigure. What do you think?

>  	int i, chan_count;
>  	struct TCP_Server_Info *server;
> @@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>  	if (chan_count == 1)
>  		goto done;
>  
> -	ses->chan_count = 1;
> -
> -	/* for all secondary channels reset the need reconnect bit */
> -	ses->chans_need_reconnect &= 1;
> +	// Update the chan_count to the new maximum
> +	if (from_reconfigure)
> +		ses->chan_count = ses->chan_max;
> +	else
> +		ses->chan_count = 1;
>  
> -	for (i = 1; i < chan_count; i++) {
> +	for (i = ses->chan_max ; i < chan_count; i++) {
>  		iface = ses->chans[i].iface;
>  		server = ses->chans[i].server;
>  
> @@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>  		spin_lock(&ses->chan_lock);
>  	}
>  
> +	/* For extra secondary channels, reset the need reconnect bit */
> +	if (ses->chan_count == 1) {
> +		cifs_server_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
> +		ses->chans_need_reconnect &= 1;
> +	} else {
> +		cifs_server_dbg(VFS, "Disable extra secondary channels\n");
> +		ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
> +	}
> +
>  done:
>  	spin_unlock(&ses->chan_lock);
>  }
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index c3b9d3f6210f..bf9a8dc0e8fc 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
>  static int
>  cifs_chan_skip_or_disable(struct cifs_ses *ses,
>  			  struct TCP_Server_Info *server,
> -			  bool from_reconnect)
> +			  bool from_reconnect, bool from_reconfigure)
>  {
>  	struct TCP_Server_Info *pserver;
>  	unsigned int chan_index;
> @@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
>  		return -EHOSTDOWN;
>  	}
>  
> -	cifs_server_dbg(VFS,
> -		"server does not support multichannel anymore. Disable all other channels\n");
> -	cifs_disable_secondary_channels(ses);
> +	cifs_decrease_secondary_channels(ses, from_reconfigure);
>  
> +	return 0;
> +}
> +
> +/**
> + * smb3_sync_ses_channels - Synchronize session channels
> + * with new configuration (cifs_sb_info version)
> + * @cifs_sb: pointer to the CIFS superblock info structure
> + * Returns 0 on success or -EINVAL if scaling is already in progress.
> + */
> +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
> +{
> +	struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
> +	struct smb3_fs_context *ctx = cifs_sb->ctx;
> +	bool from_reconnect = false;
> +
> +	/* Prevent concurrent scaling operations */
> +	spin_lock(&ses->ses_lock);
> +	if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
> +		spin_unlock(&ses->ses_lock);
> +		return -EINVAL;
> +	}
> +	ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
> +	spin_unlock(&ses->ses_lock);
> +
> +	/*
> +	 * If the old max_channels is less than the new chan_max,
> +	 * try to add channels to reach the new maximum.
> +	 * Otherwise, disable or skip extra channels to match the new configuration.
> +	 */
> +	if (ctx->max_channels < ses->chan_max) {
> +		mutex_unlock(&ses->session_mutex);
> +		cifs_try_adding_channels(ses);
> +		mutex_lock(&ses->session_mutex);
> +	} else {

Maybe you can avoid entering any cifs_chan_skip_or_disable if
ctx->max_channels == ses->chan_max. There is a cost of holding locks
inside of it.

> +		cifs_chan_skip_or_disable(ses, ses->server, from_reconnect, true);
> +	}
> +
> +	/* Clear scaling flag after operation */
> +	spin_lock(&ses->ses_lock);
> +	ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
> +	spin_unlock(&ses->ses_lock);
>  
>  	return 0;
>  }
> @@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>  	if (ses->chan_count > 1 &&
>  	    !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>  		rc = cifs_chan_skip_or_disable(ses, server,
> -					       from_reconnect);
> +					       from_reconnect, false);
>  		if (rc) {
>  			mutex_unlock(&ses->session_mutex);
>  			goto out;
> @@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>  			 */
>  
>  			rc = cifs_chan_skip_or_disable(ses, server,
> -						       from_reconnect);
> +						       from_reconnect, false);
>  			goto skip_add_channels;
>  		} else if (rc)
>  			cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
> @@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
>  		req->SecurityMode = 0;
>  
>  	req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
> -	if (ses->chan_max > 1)
> -		req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> +	req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>  
>  	/* ClientGUID must be zero for SMB2.02 dialect */
>  	if (server->vals->protocol_id == SMB20_PROT_ID)
> @@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>  	if (!pneg_inbuf)
>  		return -ENOMEM;
>  
> -	pneg_inbuf->Capabilities =
> -			cpu_to_le32(server->vals->req_capabilities);
> -	if (tcon->ses->chan_max > 1)
> -		pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> +	pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities);
> +	pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>  
>  	memcpy(pneg_inbuf->Guid, server->client_guid,
>  					SMB2_CLIENT_GUID_SIZE);
> diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
> index 3c09a58dfd07..d3f63a4ef426 100644
> --- a/fs/smb/client/smb2pdu.h
> +++ b/fs/smb/client/smb2pdu.h
> @@ -420,6 +420,8 @@ struct smb2_create_ea_ctx {
>  	struct smb2_file_full_ea_info ea;
>  } __packed;
>  
> +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb);
> +
>  #define SMB2_WSL_XATTR_UID		"$LXUID"
>  #define SMB2_WSL_XATTR_GID		"$LXGID"
>  #define SMB2_WSL_XATTR_MODE		"$LXMOD"

I also agree with Enzo that we could have an update_channels that
centralizes the logic of rescaling channels, but that could also come in
another patch as Steve suggested.

-- 
Henrique
SUSE Labs

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Shyam Prasad N]

On Tue, Sep 23, 2025 at 2:52 AM Henrique Carvalho
<henrique.carvalho@suse.com> wrote:
>
> Hi Rajasi,
>
> On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
> > From: Rajasi Mandal <rajasimandal@microsoft.com>
> >
> > Previously, the client did not properly update the session's channel
> > state when multichannel or max_channels mount options were changed
> > during remount. This led to inconsistent behavior and prevented
> > enabling or disabling multichannel support without a full
> > unmount/remount.
> >
> > Enable dynamic reconfiguration of multichannel and max_channels
> > options during remount by introducing smb3_sync_ses_chan_max() to
> > safely update the session's chan_max field, and smb3_sync_ses_channels()
> > to synchronize the session's channels with the new configuration.
> > Replace cifs_disable_secondary_channels() with
> > cifs_decrease_secondary_channels(), which now takes a from_reconfigure
> > argument for more flexible channel cleanup. Update the remount logic
> > to detect changes in multichannel or max_channels and trigger the
> > appropriate session/channel updates.
> >
> > With this change, users can safely change multichannel and
> > max_channels options on remount, and the client will correctly adjust
> > the session's channel state to match the new configuration.
> >
> > Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> > ---
> >  fs/smb/client/cifsproto.h  |  2 +-
> >  fs/smb/client/fs_context.c | 29 ++++++++++++++++++
> >  fs/smb/client/fs_context.h |  2 +-
> >  fs/smb/client/sess.c       | 35 +++++++++++++++-------
> >  fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
> >  fs/smb/client/smb2pdu.h    |  2 ++
> >  6 files changed, 105 insertions(+), 25 deletions(-)
> >
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index e8fba98690ce..ec3118457b26 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -667,7 +667,7 @@ bool
> >  cifs_chan_is_iface_active(struct cifs_ses *ses,
> >                         struct TCP_Server_Info *server);
> >  void
> > -cifs_disable_secondary_channels(struct cifs_ses *ses);
> > +cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
> >  void
> >  cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
> >  int
> > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> > index 43552b44f613..96e80c70f25d 100644
> > --- a/fs/smb/client/fs_context.c
> > +++ b/fs/smb/client/fs_context.c
> > @@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
> >       return 0;
> >  }
> >
> > +/**
> > + * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
> > + * @ses: pointer to the old CIFS session structure
> > + * @max_channels: new maximum number of channels to allow
> > + *
> > + * Updates the session's chan_max field to the new value, protecting the update
> > + * with the session's channel lock. This should be called whenever the maximum
> > + * allowed channels for a session changes (e.g., after a remount or reconfigure).
> > + */
> > +void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
> > +{
> > +     spin_lock(&ses->chan_lock);
> > +     ses->chan_max = max_channels;
> > +     spin_unlock(&ses->chan_lock);
> > +}
> > +
>
> The other writer of chan_max is when creating a session. Is this lock
> really avoiding a race here?

It's not just the writers, but also readers that we protect with this
lock. I don't have major objections to the locking here.

>
> >  static int smb3_reconfigure(struct fs_context *fc)
> >  {
> >       struct smb3_fs_context *ctx = smb3_fc2context(fc);
> > @@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
> >               ses->password2 = new_password2;
> >       }
> >
> > +     /*
> > +      * If multichannel or max_channels has changed, update the session's channels accordingly.
> > +      * This may add or remove channels to match the new configuration.
> > +      */
> > +     if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
> > +             (ctx->max_channels != cifs_sb->ctx->max_channels)) {
> > +             //Synchronize ses->chan_max with the new mount context
> > +             smb3_sync_ses_chan_max(ses, ctx->max_channels);
> > +             //Now update the session's channels to match the new configuration
> > +             rc = smb3_sync_ses_channels(cifs_sb);
> > +     }
> > +
> >       mutex_unlock(&ses->session_mutex);
> >
> >       STEAL_STRING(cifs_sb, ctx, domainname);
> > @@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
> >       smb3_cleanup_fs_context_contents(cifs_sb->ctx);
> >       rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
> >       smb3_update_mnt_flags(cifs_sb);
> > +
> >  #ifdef CONFIG_CIFS_DFS_UPCALL
> >       if (!rc)
> >               rc = dfs_cache_remount_fs(cifs_sb);
> > diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> > index b0fec6b9a23b..a75185858285 100644
> > --- a/fs/smb/client/fs_context.h
> > +++ b/fs/smb/client/fs_context.h
> > @@ -371,7 +371,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
> >  extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
> >  extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
> >  extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
> > -
> > +extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
> >  /*
> >   * max deferred close timeout (jiffies) - 2^30
> >   */
> > diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
> > index 0a8c2fcc9ded..42b5481c884a 100644
> > --- a/fs/smb/client/sess.c
> > +++ b/fs/smb/client/sess.c
> > @@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
> >       return new_chan_count - old_chan_count;
> >  }
> >
> > -/*
> > - * called when multichannel is disabled by the server.
> > - * this always gets called from smb2_reconnect
> > - * and cannot get called in parallel threads.
> > +/**
> > + * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
> > + * @ses: pointer to the CIFS session structure
> > + * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
> > + *
> > + * This function disables and cleans up extra secondary channels for a CIFS session.
> > + * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
> > + * Otherwise, it disables all but the primary channel.
> >   */
> > -void
> > -cifs_disable_secondary_channels(struct cifs_ses *ses)
> > +void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
> >  {
>
> Maybe you could get rid of from_reconfigure parameter if you just set
> chan_max to 1 before calling cifs_decrease_secondary_channels when this
> function is not called from smb3_reconfigure. What do you think?

chan_max today is set based on "max_channels" mount option that the user sets.
I don't think we should be modifying that value. Unless we change it's
meaning today. :)

>
> >       int i, chan_count;
> >       struct TCP_Server_Info *server;
> > @@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
> >       if (chan_count == 1)
> >               goto done;
> >
> > -     ses->chan_count = 1;
> > -
> > -     /* for all secondary channels reset the need reconnect bit */
> > -     ses->chans_need_reconnect &= 1;
> > +     // Update the chan_count to the new maximum
> > +     if (from_reconfigure)
> > +             ses->chan_count = ses->chan_max;
> > +     else
> > +             ses->chan_count = 1;
> >
> > -     for (i = 1; i < chan_count; i++) {
> > +     for (i = ses->chan_max ; i < chan_count; i++) {
> >               iface = ses->chans[i].iface;
> >               server = ses->chans[i].server;
> >
> > @@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
> >               spin_lock(&ses->chan_lock);
> >       }
> >
> > +     /* For extra secondary channels, reset the need reconnect bit */
> > +     if (ses->chan_count == 1) {
> > +             cifs_server_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
> > +             ses->chans_need_reconnect &= 1;
> > +     } else {
> > +             cifs_server_dbg(VFS, "Disable extra secondary channels\n");
> > +             ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
> > +     }
> > +
> >  done:
> >       spin_unlock(&ses->chan_lock);
> >  }
> > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> > index c3b9d3f6210f..bf9a8dc0e8fc 100644
> > --- a/fs/smb/client/smb2pdu.c
> > +++ b/fs/smb/client/smb2pdu.c
> > @@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
> >  static int
> >  cifs_chan_skip_or_disable(struct cifs_ses *ses,
> >                         struct TCP_Server_Info *server,
> > -                       bool from_reconnect)
> > +                       bool from_reconnect, bool from_reconfigure)
> >  {
> >       struct TCP_Server_Info *pserver;
> >       unsigned int chan_index;
> > @@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
> >               return -EHOSTDOWN;
> >       }
> >
> > -     cifs_server_dbg(VFS,
> > -             "server does not support multichannel anymore. Disable all other channels\n");
> > -     cifs_disable_secondary_channels(ses);
> > +     cifs_decrease_secondary_channels(ses, from_reconfigure);
> >
> > +     return 0;
> > +}
> > +
> > +/**
> > + * smb3_sync_ses_channels - Synchronize session channels
> > + * with new configuration (cifs_sb_info version)
> > + * @cifs_sb: pointer to the CIFS superblock info structure
> > + * Returns 0 on success or -EINVAL if scaling is already in progress.
> > + */
> > +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
> > +{
> > +     struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
> > +     struct smb3_fs_context *ctx = cifs_sb->ctx;
> > +     bool from_reconnect = false;
> > +
> > +     /* Prevent concurrent scaling operations */
> > +     spin_lock(&ses->ses_lock);
> > +     if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
> > +             spin_unlock(&ses->ses_lock);
> > +             return -EINVAL;
> > +     }
> > +     ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
> > +     spin_unlock(&ses->ses_lock);
> > +
> > +     /*
> > +      * If the old max_channels is less than the new chan_max,
> > +      * try to add channels to reach the new maximum.
> > +      * Otherwise, disable or skip extra channels to match the new configuration.
> > +      */
> > +     if (ctx->max_channels < ses->chan_max) {
> > +             mutex_unlock(&ses->session_mutex);
> > +             cifs_try_adding_channels(ses);
> > +             mutex_lock(&ses->session_mutex);
> > +     } else {
>
> Maybe you can avoid entering any cifs_chan_skip_or_disable if
> ctx->max_channels == ses->chan_max. There is a cost of holding locks
> inside of it.
>
> > +             cifs_chan_skip_or_disable(ses, ses->server, from_reconnect, true);
> > +     }
> > +
> > +     /* Clear scaling flag after operation */
> > +     spin_lock(&ses->ses_lock);
> > +     ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
> > +     spin_unlock(&ses->ses_lock);
> >
> >       return 0;
> >  }
> > @@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
> >       if (ses->chan_count > 1 &&
> >           !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
> >               rc = cifs_chan_skip_or_disable(ses, server,
> > -                                            from_reconnect);
> > +                                            from_reconnect, false);
> >               if (rc) {
> >                       mutex_unlock(&ses->session_mutex);
> >                       goto out;
> > @@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
> >                        */
> >
> >                       rc = cifs_chan_skip_or_disable(ses, server,
> > -                                                    from_reconnect);
> > +                                                    from_reconnect, false);
> >                       goto skip_add_channels;
> >               } else if (rc)
> >                       cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
> > @@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
> >               req->SecurityMode = 0;
> >
> >       req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
> > -     if (ses->chan_max > 1)
> > -             req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> > +     req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> >
> >       /* ClientGUID must be zero for SMB2.02 dialect */
> >       if (server->vals->protocol_id == SMB20_PROT_ID)
> > @@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> >       if (!pneg_inbuf)
> >               return -ENOMEM;
> >
> > -     pneg_inbuf->Capabilities =
> > -                     cpu_to_le32(server->vals->req_capabilities);
> > -     if (tcon->ses->chan_max > 1)
> > -             pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> > +     pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities);
> > +     pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
> >
> >       memcpy(pneg_inbuf->Guid, server->client_guid,
> >                                       SMB2_CLIENT_GUID_SIZE);
> > diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
> > index 3c09a58dfd07..d3f63a4ef426 100644
> > --- a/fs/smb/client/smb2pdu.h
> > +++ b/fs/smb/client/smb2pdu.h
> > @@ -420,6 +420,8 @@ struct smb2_create_ea_ctx {
> >       struct smb2_file_full_ea_info ea;
> >  } __packed;
> >
> > +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb);
> > +
> >  #define SMB2_WSL_XATTR_UID           "$LXUID"
> >  #define SMB2_WSL_XATTR_GID           "$LXGID"
> >  #define SMB2_WSL_XATTR_MODE          "$LXMOD"
>
> I also agree with Enzo that we could have an update_channels that
> centralizes the logic of rescaling channels, but that could also come in
> another patch as Steve suggested.
Ack. We'll review that.

>
> --
> Henrique
> SUSE Labs
>

Thank you for reviewing.

-- 
Regards,
Shyam

Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount

[Enzo Matsumiya]

On 09/23, Shyam Prasad N wrote:
>On Tue, Sep 23, 2025 at 2:52 AM Henrique Carvalho
><henrique.carvalho@suse.com> wrote:
>>
>> Hi Rajasi,
>>
>> On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
>> > From: Rajasi Mandal <rajasimandal@microsoft.com>
>> >
>> > Previously, the client did not properly update the session's channel
>> > state when multichannel or max_channels mount options were changed
>> > during remount. This led to inconsistent behavior and prevented
>> > enabling or disabling multichannel support without a full
>> > unmount/remount.
>> >
>> > Enable dynamic reconfiguration of multichannel and max_channels
>> > options during remount by introducing smb3_sync_ses_chan_max() to
>> > safely update the session's chan_max field, and smb3_sync_ses_channels()
>> > to synchronize the session's channels with the new configuration.
>> > Replace cifs_disable_secondary_channels() with
>> > cifs_decrease_secondary_channels(), which now takes a from_reconfigure
>> > argument for more flexible channel cleanup. Update the remount logic
>> > to detect changes in multichannel or max_channels and trigger the
>> > appropriate session/channel updates.
>> >
>> > With this change, users can safely change multichannel and
>> > max_channels options on remount, and the client will correctly adjust
>> > the session's channel state to match the new configuration.
>> >
>> > Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>> > ---
>> >  fs/smb/client/cifsproto.h  |  2 +-
>> >  fs/smb/client/fs_context.c | 29 ++++++++++++++++++
>> >  fs/smb/client/fs_context.h |  2 +-
>> >  fs/smb/client/sess.c       | 35 +++++++++++++++-------
>> >  fs/smb/client/smb2pdu.c    | 60 ++++++++++++++++++++++++++++++--------
>> >  fs/smb/client/smb2pdu.h    |  2 ++
>> >  6 files changed, 105 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
>> > index e8fba98690ce..ec3118457b26 100644
>> > --- a/fs/smb/client/cifsproto.h
>> > +++ b/fs/smb/client/cifsproto.h
>> > @@ -667,7 +667,7 @@ bool
>> >  cifs_chan_is_iface_active(struct cifs_ses *ses,
>> >                         struct TCP_Server_Info *server);
>> >  void
>> > -cifs_disable_secondary_channels(struct cifs_ses *ses);
>> > +cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure);
>> >  void
>> >  cifs_chan_update_iface(struct cifs_ses *ses, struct TCP_Server_Info *server);
>> >  int
>> > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> > index 43552b44f613..96e80c70f25d 100644
>> > --- a/fs/smb/client/fs_context.c
>> > +++ b/fs/smb/client/fs_context.c
>> > @@ -1015,6 +1015,22 @@ int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_se
>> >       return 0;
>> >  }
>> >
>> > +/**
>> > + * smb3_sync_ses_chan_max - Synchronize the session's maximum channel count
>> > + * @ses: pointer to the old CIFS session structure
>> > + * @max_channels: new maximum number of channels to allow
>> > + *
>> > + * Updates the session's chan_max field to the new value, protecting the update
>> > + * with the session's channel lock. This should be called whenever the maximum
>> > + * allowed channels for a session changes (e.g., after a remount or reconfigure).
>> > + */
>> > +void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels)
>> > +{
>> > +     spin_lock(&ses->chan_lock);
>> > +     ses->chan_max = max_channels;
>> > +     spin_unlock(&ses->chan_lock);
>> > +}
>> > +
>>
>> The other writer of chan_max is when creating a session. Is this lock
>> really avoiding a race here?

That locked write on ses creation is also useless as it's done before ses
is even on list i.e. no other thread can even see this ses yet.

>It's not just the writers, but also readers that we protect with this
>lock. I don't have major objections to the locking here.

I couldn't find any _locked_ readers of ses->chan_max, did I miss
anything?

>> >  static int smb3_reconfigure(struct fs_context *fc)
>> >  {
>> >       struct smb3_fs_context *ctx = smb3_fc2context(fc);
>> > @@ -1097,6 +1113,18 @@ static int smb3_reconfigure(struct fs_context *fc)
>> >               ses->password2 = new_password2;
>> >       }
>> >
>> > +     /*
>> > +      * If multichannel or max_channels has changed, update the session's channels accordingly.
>> > +      * This may add or remove channels to match the new configuration.
>> > +      */
>> > +     if ((ctx->multichannel != cifs_sb->ctx->multichannel) ||
>> > +             (ctx->max_channels != cifs_sb->ctx->max_channels)) {
>> > +             //Synchronize ses->chan_max with the new mount context
>> > +             smb3_sync_ses_chan_max(ses, ctx->max_channels);
>> > +             //Now update the session's channels to match the new configuration
>> > +             rc = smb3_sync_ses_channels(cifs_sb);
>> > +     }
>> > +
>> >       mutex_unlock(&ses->session_mutex);
>> >
>> >       STEAL_STRING(cifs_sb, ctx, domainname);
>> > @@ -1110,6 +1138,7 @@ static int smb3_reconfigure(struct fs_context *fc)
>> >       smb3_cleanup_fs_context_contents(cifs_sb->ctx);
>> >       rc = smb3_fs_context_dup(cifs_sb->ctx, ctx);
>> >       smb3_update_mnt_flags(cifs_sb);
>> > +
>> >  #ifdef CONFIG_CIFS_DFS_UPCALL
>> >       if (!rc)
>> >               rc = dfs_cache_remount_fs(cifs_sb);
>> > diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
>> > index b0fec6b9a23b..a75185858285 100644
>> > --- a/fs/smb/client/fs_context.h
>> > +++ b/fs/smb/client/fs_context.h
>> > @@ -371,7 +371,7 @@ static inline struct smb3_fs_context *smb3_fc2context(const struct fs_context *f
>> >  extern int smb3_fs_context_dup(struct smb3_fs_context *new_ctx, struct smb3_fs_context *ctx);
>> >  extern int smb3_sync_session_ctx_passwords(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses);
>> >  extern void smb3_update_mnt_flags(struct cifs_sb_info *cifs_sb);
>> > -
>> > +extern void smb3_sync_ses_chan_max(struct cifs_ses *ses, unsigned int max_channels);
>> >  /*
>> >   * max deferred close timeout (jiffies) - 2^30
>> >   */
>> > diff --git a/fs/smb/client/sess.c b/fs/smb/client/sess.c
>> > index 0a8c2fcc9ded..42b5481c884a 100644
>> > --- a/fs/smb/client/sess.c
>> > +++ b/fs/smb/client/sess.c
>> > @@ -264,13 +264,16 @@ int cifs_try_adding_channels(struct cifs_ses *ses)
>> >       return new_chan_count - old_chan_count;
>> >  }
>> >
>> > -/*
>> > - * called when multichannel is disabled by the server.
>> > - * this always gets called from smb2_reconnect
>> > - * and cannot get called in parallel threads.
>> > +/**
>> > + * cifs_decrease_secondary_channels - Reduce the number of active secondary channels
>> > + * @ses: pointer to the CIFS session structure
>> > + * @from_reconfigure: if true, only reduce to chan_max; if false, reduce to a single channel
>> > + *
>> > + * This function disables and cleans up extra secondary channels for a CIFS session.
>> > + * If called during reconfiguration, it reduces the channel count to the new maximum (chan_max).
>> > + * Otherwise, it disables all but the primary channel.
>> >   */
>> > -void
>> > -cifs_disable_secondary_channels(struct cifs_ses *ses)
>> > +void cifs_decrease_secondary_channels(struct cifs_ses *ses, bool from_reconfigure)
>> >  {
>>
>> Maybe you could get rid of from_reconfigure parameter if you just set
>> chan_max to 1 before calling cifs_decrease_secondary_channels when this
>> function is not called from smb3_reconfigure. What do you think?
>
>chan_max today is set based on "max_channels" mount option that the user sets.
>I don't think we should be modifying that value. Unless we change it's
>meaning today. :)
>
>>
>> >       int i, chan_count;
>> >       struct TCP_Server_Info *server;
>> > @@ -281,12 +284,13 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>> >       if (chan_count == 1)
>> >               goto done;
>> >
>> > -     ses->chan_count = 1;
>> > -
>> > -     /* for all secondary channels reset the need reconnect bit */
>> > -     ses->chans_need_reconnect &= 1;
>> > +     // Update the chan_count to the new maximum
>> > +     if (from_reconfigure)
>> > +             ses->chan_count = ses->chan_max;
>> > +     else
>> > +             ses->chan_count = 1;
>> >
>> > -     for (i = 1; i < chan_count; i++) {
>> > +     for (i = ses->chan_max ; i < chan_count; i++) {
>> >               iface = ses->chans[i].iface;
>> >               server = ses->chans[i].server;
>> >
>> > @@ -318,6 +322,15 @@ cifs_disable_secondary_channels(struct cifs_ses *ses)
>> >               spin_lock(&ses->chan_lock);
>> >       }
>> >
>> > +     /* For extra secondary channels, reset the need reconnect bit */
>> > +     if (ses->chan_count == 1) {
>> > +             cifs_server_dbg(VFS, "server does not support multichannel anymore. Disable all other channels\n");
>> > +             ses->chans_need_reconnect &= 1;
>> > +     } else {
>> > +             cifs_server_dbg(VFS, "Disable extra secondary channels\n");
>> > +             ses->chans_need_reconnect &= ((1UL << ses->chan_max) - 1);
>> > +     }
>> > +
>> >  done:
>> >       spin_unlock(&ses->chan_lock);
>> >  }
>> > diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
>> > index c3b9d3f6210f..bf9a8dc0e8fc 100644
>> > --- a/fs/smb/client/smb2pdu.c
>> > +++ b/fs/smb/client/smb2pdu.c
>> > @@ -168,7 +168,7 @@ smb2_hdr_assemble(struct smb2_hdr *shdr, __le16 smb2_cmd,
>> >  static int
>> >  cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >                         struct TCP_Server_Info *server,
>> > -                       bool from_reconnect)
>> > +                       bool from_reconnect, bool from_reconfigure)
>> >  {
>> >       struct TCP_Server_Info *pserver;
>> >       unsigned int chan_index;
>> > @@ -206,10 +206,49 @@ cifs_chan_skip_or_disable(struct cifs_ses *ses,
>> >               return -EHOSTDOWN;
>> >       }
>> >
>> > -     cifs_server_dbg(VFS,
>> > -             "server does not support multichannel anymore. Disable all other channels\n");
>> > -     cifs_disable_secondary_channels(ses);
>> > +     cifs_decrease_secondary_channels(ses, from_reconfigure);
>> >
>> > +     return 0;
>> > +}
>> > +
>> > +/**
>> > + * smb3_sync_ses_channels - Synchronize session channels
>> > + * with new configuration (cifs_sb_info version)
>> > + * @cifs_sb: pointer to the CIFS superblock info structure
>> > + * Returns 0 on success or -EINVAL if scaling is already in progress.
>> > + */
>> > +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb)
>> > +{
>> > +     struct cifs_ses *ses = cifs_sb_master_tcon(cifs_sb)->ses;
>> > +     struct smb3_fs_context *ctx = cifs_sb->ctx;
>> > +     bool from_reconnect = false;
>> > +
>> > +     /* Prevent concurrent scaling operations */
>> > +     spin_lock(&ses->ses_lock);
>> > +     if (ses->flags & CIFS_SES_FLAG_SCALE_CHANNELS) {
>> > +             spin_unlock(&ses->ses_lock);
>> > +             return -EINVAL;
>> > +     }
>> > +     ses->flags |= CIFS_SES_FLAG_SCALE_CHANNELS;
>> > +     spin_unlock(&ses->ses_lock);
>> > +
>> > +     /*
>> > +      * If the old max_channels is less than the new chan_max,
>> > +      * try to add channels to reach the new maximum.
>> > +      * Otherwise, disable or skip extra channels to match the new configuration.
>> > +      */
>> > +     if (ctx->max_channels < ses->chan_max) {
>> > +             mutex_unlock(&ses->session_mutex);
>> > +             cifs_try_adding_channels(ses);
>> > +             mutex_lock(&ses->session_mutex);
>> > +     } else {
>>
>> Maybe you can avoid entering any cifs_chan_skip_or_disable if
>> ctx->max_channels == ses->chan_max. There is a cost of holding locks
>> inside of it.
>>
>> > +             cifs_chan_skip_or_disable(ses, ses->server, from_reconnect, true);
>> > +     }
>> > +
>> > +     /* Clear scaling flag after operation */
>> > +     spin_lock(&ses->ses_lock);
>> > +     ses->flags &= ~CIFS_SES_FLAG_SCALE_CHANNELS;
>> > +     spin_unlock(&ses->ses_lock);
>> >
>> >       return 0;
>> >  }
>> > @@ -356,7 +395,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>> >       if (ses->chan_count > 1 &&
>> >           !(server->capabilities & SMB2_GLOBAL_CAP_MULTI_CHANNEL)) {
>> >               rc = cifs_chan_skip_or_disable(ses, server,
>> > -                                            from_reconnect);
>> > +                                            from_reconnect, false);
>> >               if (rc) {
>> >                       mutex_unlock(&ses->session_mutex);
>> >                       goto out;
>> > @@ -439,7 +478,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
>> >                        */
>> >
>> >                       rc = cifs_chan_skip_or_disable(ses, server,
>> > -                                                    from_reconnect);
>> > +                                                    from_reconnect, false);
>> >                       goto skip_add_channels;
>> >               } else if (rc)
>> >                       cifs_dbg(FYI, "%s: failed to query server interfaces: %d\n",
>> > @@ -1105,8 +1144,7 @@ SMB2_negotiate(const unsigned int xid,
>> >               req->SecurityMode = 0;
>> >
>> >       req->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>> > -     if (ses->chan_max > 1)
>> > -             req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> > +     req->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >
>> >       /* ClientGUID must be zero for SMB2.02 dialect */
>> >       if (server->vals->protocol_id == SMB20_PROT_ID)
>> > @@ -1310,10 +1348,8 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>> >       if (!pneg_inbuf)
>> >               return -ENOMEM;
>> >
>> > -     pneg_inbuf->Capabilities =
>> > -                     cpu_to_le32(server->vals->req_capabilities);
>> > -     if (tcon->ses->chan_max > 1)
>> > -             pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> > +     pneg_inbuf->Capabilities = cpu_to_le32(server->vals->req_capabilities);
>> > +     pneg_inbuf->Capabilities |= cpu_to_le32(SMB2_GLOBAL_CAP_MULTI_CHANNEL);
>> >
>> >       memcpy(pneg_inbuf->Guid, server->client_guid,
>> >                                       SMB2_CLIENT_GUID_SIZE);
>> > diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
>> > index 3c09a58dfd07..d3f63a4ef426 100644
>> > --- a/fs/smb/client/smb2pdu.h
>> > +++ b/fs/smb/client/smb2pdu.h
>> > @@ -420,6 +420,8 @@ struct smb2_create_ea_ctx {
>> >       struct smb2_file_full_ea_info ea;
>> >  } __packed;
>> >
>> > +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb);
>> > +
>> >  #define SMB2_WSL_XATTR_UID           "$LXUID"
>> >  #define SMB2_WSL_XATTR_GID           "$LXGID"
>> >  #define SMB2_WSL_XATTR_MODE          "$LXMOD"
>>
>> I also agree with Enzo that we could have an update_channels that
>> centralizes the logic of rescaling channels, but that could also come in
>> another patch as Steve suggested.
>Ack. We'll review that.
>
>>
>> --
>> Henrique
>> SUSE Labs
>>
>
>Thank you for reviewing.
>
>-- 
>Regards,
>Shyam
>

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

[Henrique Carvalho]

Hi Rajasi,

On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
> From: Rajasi Mandal <rajasimandal@microsoft.com>
> 
> Previously, specifying both multichannel and max_channels=1 as mount
> options would leave multichannel enabled, even though it is not
> meaningful when only one channel is allowed. This led to confusion and
> inconsistent behavior, as the client would advertise multichannel
> capability but never establish secondary channels.
> 
> Fix this by forcing multichannel to false whenever max_channels=1,
> ensuring the mount configuration is consistent and matches user intent.
> This prevents the client from advertising or attempting multichannel
> support when it is not possible.
> 
> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> ---
>  fs/smb/client/fs_context.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 072383899e81..43552b44f613 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>  		goto cifs_parse_mount_err;
>  	}
>  
> +	/*
> +	 * Multichannel is not meaningful if max_channels is 1.
> +	 * Force multichannel to false to ensure consistent configuration.
> +	 */
> +	if (ctx->multichannel && ctx->max_channels == 1)
> +		ctx->multichannel = false;
> +
>  	return 0;
>  
>   cifs_parse_mount_err:

Do we even need ->multichannel flag at all? Maybe we could replace
->multichannel for a function that checks for max_channels > 1?

-- 
Henrique
SUSE Labs

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

[Enzo Matsumiya]

On 09/22, Henrique Carvalho wrote:
>Hi Rajasi,
>
>On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
>> From: Rajasi Mandal <rajasimandal@microsoft.com>
>>
>> Previously, specifying both multichannel and max_channels=1 as mount
>> options would leave multichannel enabled, even though it is not
>> meaningful when only one channel is allowed. This led to confusion and
>> inconsistent behavior, as the client would advertise multichannel
>> capability but never establish secondary channels.
>>
>> Fix this by forcing multichannel to false whenever max_channels=1,
>> ensuring the mount configuration is consistent and matches user intent.
>> This prevents the client from advertising or attempting multichannel
>> support when it is not possible.
>>
>> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>> ---
>>  fs/smb/client/fs_context.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> index 072383899e81..43552b44f613 100644
>> --- a/fs/smb/client/fs_context.c
>> +++ b/fs/smb/client/fs_context.c
>> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>>  		goto cifs_parse_mount_err;
>>  	}
>>
>> +	/*
>> +	 * Multichannel is not meaningful if max_channels is 1.
>> +	 * Force multichannel to false to ensure consistent configuration.
>> +	 */
>> +	if (ctx->multichannel && ctx->max_channels == 1)
>> +		ctx->multichannel = false;
>> +
>>  	return 0;
>>
>>   cifs_parse_mount_err:
>
>Do we even need ->multichannel flag at all? Maybe we could replace
>->multichannel for a function that checks for max_channels > 1?

I agree with Henrique.

I'd actually like to propose going even further and having the whole
module behaving as if multichannel was always on, even with
max_channels=1 -- the only difference being the need to run the
query_interfaces worker.

This is probably work for another patch/series though.


Cheers,

Enzo

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

[Steve French]

. >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)
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'd actually like to propose going even further and having the whole
module behaving as if multichannel was always on, even with
max_channels=1

Obviously the goal (would love patches for this) is to turn
multichannel on by default, have the client select the appropriate
number of channels by default etc. but we have to let the user
override it (as described above)

On Mon, Sep 22, 2025 at 9:59 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 09/22, Henrique Carvalho wrote:
> >Hi Rajasi,
> >
> >On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
> >> From: Rajasi Mandal <rajasimandal@microsoft.com>
> >>
> >> Previously, specifying both multichannel and max_channels=1 as mount
> >> options would leave multichannel enabled, even though it is not
> >> meaningful when only one channel is allowed. This led to confusion and
> >> inconsistent behavior, as the client would advertise multichannel
> >> capability but never establish secondary channels.
> >>
> >> Fix this by forcing multichannel to false whenever max_channels=1,
> >> ensuring the mount configuration is consistent and matches user intent.
> >> This prevents the client from advertising or attempting multichannel
> >> support when it is not possible.
> >>
> >> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> >> ---
> >>  fs/smb/client/fs_context.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> >> index 072383899e81..43552b44f613 100644
> >> --- a/fs/smb/client/fs_context.c
> >> +++ b/fs/smb/client/fs_context.c
> >> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
> >>              goto cifs_parse_mount_err;
> >>      }
> >>
> >> +    /*
> >> +     * Multichannel is not meaningful if max_channels is 1.
> >> +     * Force multichannel to false to ensure consistent configuration.
> >> +     */
> >> +    if (ctx->multichannel && ctx->max_channels == 1)
> >> +            ctx->multichannel = false;
> >> +
> >>      return 0;
> >>
> >>   cifs_parse_mount_err:
> >
> >Do we even need ->multichannel flag at all? Maybe we could replace
> >->multichannel for a function that checks for max_channels > 1?
>
> I agree with Henrique.
>
> I'd actually like to propose going even further and having the whole
> module behaving as if multichannel was always on, even with
> max_channels=1 -- the only difference being the need to run the
> query_interfaces worker.
>
> This is probably work for another patch/series though.
>
>
> Cheers,
>
> Enzo
>


-- 
Thanks,

Steve

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

[Henrique Carvalho]

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.

> 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
> 

Yes, I agree the patches are important, I am also testing them here.

> On Mon, Sep 22, 2025 at 9:59 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>> I'd actually like to propose going even further and having the whole
>> module behaving as if multichannel was always on, even with
>> max_channels=1
> 
> Obviously the goal (would love patches for this) is to turn
> multichannel on by default, have the client select the appropriate
> number of channels by default etc. but we have to let the user
> override it (as described above)
> 
>>
>> On 09/22, Henrique Carvalho wrote:
>>> Hi Rajasi,
>>>
>>> On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
>>>> From: Rajasi Mandal <rajasimandal@microsoft.com>
>>>>
>>>> Previously, specifying both multichannel and max_channels=1 as mount
>>>> options would leave multichannel enabled, even though it is not
>>>> meaningful when only one channel is allowed. This led to confusion and
>>>> inconsistent behavior, as the client would advertise multichannel
>>>> capability but never establish secondary channels.
>>>>
>>>> Fix this by forcing multichannel to false whenever max_channels=1,
>>>> ensuring the mount configuration is consistent and matches user intent.
>>>> This prevents the client from advertising or attempting multichannel
>>>> support when it is not possible.
>>>>
>>>> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>>>> ---
>>>>  fs/smb/client/fs_context.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>>>> index 072383899e81..43552b44f613 100644
>>>> --- a/fs/smb/client/fs_context.c
>>>> +++ b/fs/smb/client/fs_context.c
>>>> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>>>>              goto cifs_parse_mount_err;
>>>>      }
>>>>
>>>> +    /*
>>>> +     * Multichannel is not meaningful if max_channels is 1.
>>>> +     * Force multichannel to false to ensure consistent configuration.
>>>> +     */
>>>> +    if (ctx->multichannel && ctx->max_channels == 1)
>>>> +            ctx->multichannel = false;
>>>> +
>>>>      return 0;
>>>>
>>>>   cifs_parse_mount_err:
>>>
>>> Do we even need ->multichannel flag at all? Maybe we could replace
>>> ->multichannel for a function that checks for max_channels > 1?
>>
>> I agree with Henrique.
>>
>> I'd actually like to propose going even further and having the whole
>> module behaving as if multichannel was always on, even with
>> max_channels=1 -- the only difference being the need to run the
>> query_interfaces worker.
>>
>> This is probably work for another patch/series though.
>>
>>
>> Cheers,
>>
>> Enzo
>>
> 
> 

-- 
Henrique
SUSE Labs

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];

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

[Henrique Carvalho]

On 9/22/25 3:52 PM, Enzo Matsumiya wrote:
> 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
> 
> 

I agree. This is clearer and we can drop one flag.

> 
> ---------------------
> 
> 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];
> 

-- 
Henrique
SUSE Labs

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

[Shyam Prasad N]

On Mon, Sep 22, 2025 at 8:29 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 09/22, Henrique Carvalho wrote:
> >Hi Rajasi,
> >
> >On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
> >> From: Rajasi Mandal <rajasimandal@microsoft.com>
> >>
> >> Previously, specifying both multichannel and max_channels=1 as mount
> >> options would leave multichannel enabled, even though it is not
> >> meaningful when only one channel is allowed. This led to confusion and
> >> inconsistent behavior, as the client would advertise multichannel
> >> capability but never establish secondary channels.
> >>
> >> Fix this by forcing multichannel to false whenever max_channels=1,
> >> ensuring the mount configuration is consistent and matches user intent.
> >> This prevents the client from advertising or attempting multichannel
> >> support when it is not possible.
> >>
> >> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> >> ---
> >>  fs/smb/client/fs_context.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> >> index 072383899e81..43552b44f613 100644
> >> --- a/fs/smb/client/fs_context.c
> >> +++ b/fs/smb/client/fs_context.c
> >> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
> >>              goto cifs_parse_mount_err;
> >>      }
> >>
> >> +    /*
> >> +     * Multichannel is not meaningful if max_channels is 1.
> >> +     * Force multichannel to false to ensure consistent configuration.
> >> +     */
> >> +    if (ctx->multichannel && ctx->max_channels == 1)
> >> +            ctx->multichannel = false;
> >> +
> >>      return 0;
> >>
> >>   cifs_parse_mount_err:
> >
> >Do we even need ->multichannel flag at all? Maybe we could replace
> >->multichannel for a function that checks for max_channels > 1?
>
> I agree with Henrique.
>
> I'd actually like to propose going even further and having the whole
> module behaving as if multichannel was always on, even with
> max_channels=1 -- the only difference being the need to run the
> query_interfaces worker.
>
> This is probably work for another patch/series though.
>
>
> Cheers,
>
> Enzo
>

Although I agree with this line of thought, I'd want to do it slightly
differently.
max_channels should be a configurable option for users to tune the
number of max channels, even if the actual channel count is lower.
multichannel mount option should be kept to maintain backward
compatibility, but always be interpreted based on max_channels value.

In the future, we should make max_channels=16 the default, thereby
enabling multichannel by default. Users optionally can set
max_channels to a lower value.

-- 
Regards,
Shyam

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

[Enzo Matsumiya]

On 09/23, Shyam Prasad N wrote:
>On Mon, Sep 22, 2025 at 8:29 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>>
>> On 09/22, Henrique Carvalho wrote:
>> >Hi Rajasi,
>> >
>> >On 9/22/25 5:24 AM, rajasimandalos@gmail.com wrote:
>> >> From: Rajasi Mandal <rajasimandal@microsoft.com>
>> >>
>> >> Previously, specifying both multichannel and max_channels=1 as mount
>> >> options would leave multichannel enabled, even though it is not
>> >> meaningful when only one channel is allowed. This led to confusion and
>> >> inconsistent behavior, as the client would advertise multichannel
>> >> capability but never establish secondary channels.
>> >>
>> >> Fix this by forcing multichannel to false whenever max_channels=1,
>> >> ensuring the mount configuration is consistent and matches user intent.
>> >> This prevents the client from advertising or attempting multichannel
>> >> support when it is not possible.
>> >>
>> >> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
>> >> ---
>> >>  fs/smb/client/fs_context.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
>> >> index 072383899e81..43552b44f613 100644
>> >> --- a/fs/smb/client/fs_context.c
>> >> +++ b/fs/smb/client/fs_context.c
>> >> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>> >>              goto cifs_parse_mount_err;
>> >>      }
>> >>
>> >> +    /*
>> >> +     * Multichannel is not meaningful if max_channels is 1.
>> >> +     * Force multichannel to false to ensure consistent configuration.
>> >> +     */
>> >> +    if (ctx->multichannel && ctx->max_channels == 1)
>> >> +            ctx->multichannel = false;
>> >> +
>> >>      return 0;
>> >>
>> >>   cifs_parse_mount_err:
>> >
>> >Do we even need ->multichannel flag at all? Maybe we could replace
>> >->multichannel for a function that checks for max_channels > 1?
>>
>> I agree with Henrique.
>>
>> I'd actually like to propose going even further and having the whole
>> module behaving as if multichannel was always on, even with
>> max_channels=1 -- the only difference being the need to run the
>> query_interfaces worker.
>>
>> This is probably work for another patch/series though.
>>
>>
>> Cheers,
>>
>> Enzo
>>
>
>Although I agree with this line of thought, I'd want to do it slightly
>differently.
>max_channels should be a configurable option for users to tune the
>number of max channels, even if the actual channel count is lower.
>multichannel mount option should be kept to maintain backward
>compatibility, but always be interpreted based on max_channels value.
>
>In the future, we should make max_channels=16 the default, thereby
>enabling multichannel by default. Users optionally can set
>max_channels to a lower value.

Have you seen the PoC patch I put here?
https://lore.kernel.org/linux-cifs/byjdlepkzmhm6j4ap5eyzdcusl7dgq3iuhkduf3s5h4mrayj32@lzwe2rksc4ei/

It's an attempt to remove confusion for the user-facing mount options.

As for behaviour, I think we all agree on it in general, it's just that
the implementation (dev perspective, having too many ways to check for
the same thing) is confusing and error prone cf. my comment about
query_interfaces worker running even if the user never requested
multichannel -- regardless if it runs every Xmin, it's 100% unnecessary.


Cheers,

Enzo

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

[Steve French]

i just noticed a more serious problem with multichannel/max_channels

When we mount with multichannel (at least to Samba e.g.) with
multichannel disabled on the server it confusingly returns "resource
not available" we should at least log to dmesg something more
meaningful than what we do today:

[ 1195.349188] CIFS: VFS: failed to open extra channel on
iface:10.45.126.66 rc=-11
[ 1195.454361] CIFS: successfully opened new channel on
iface:2607:fb90:f2b6:0732:7504:183c:991e:6e53
[ 1195.454599] CIFS: VFS: reconnect tcon failed rc = -11
[ 1195.457025] CIFS: VFS: reconnect tcon failed rc = -11
[ 1195.457040] CIFS: VFS: cifs_read_super: get root inode failed


Samba behavior is also strange - it advertises multichannel support in
negprot response but doesn't advertise it in session setup flags.

On Mon, Sep 22, 2025 at 3:25 AM <rajasimandalos@gmail.com> wrote:
>
> From: Rajasi Mandal <rajasimandal@microsoft.com>
>
> Previously, specifying both multichannel and max_channels=1 as mount
> options would leave multichannel enabled, even though it is not
> meaningful when only one channel is allowed. This led to confusion and
> inconsistent behavior, as the client would advertise multichannel
> capability but never establish secondary channels.
>
> Fix this by forcing multichannel to false whenever max_channels=1,
> ensuring the mount configuration is consistent and matches user intent.
> This prevents the client from advertising or attempting multichannel
> support when it is not possible.
>
> Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> ---
>  fs/smb/client/fs_context.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 072383899e81..43552b44f613 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
>                 goto cifs_parse_mount_err;
>         }
>
> +       /*
> +        * Multichannel is not meaningful if max_channels is 1.
> +        * Force multichannel to false to ensure consistent configuration.
> +        */
> +       if (ctx->multichannel && ctx->max_channels == 1)
> +               ctx->multichannel = false;
> +
>         return 0;
>
>   cifs_parse_mount_err:
> --
> 2.43.0
>
>


-- 
Thanks,

Steve

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

[Shyam Prasad N]

On Mon, Sep 22, 2025 at 9:36 PM Steve French <smfrench@gmail.com> wrote:
>
> i just noticed a more serious problem with multichannel/max_channels
>
> When we mount with multichannel (at least to Samba e.g.) with
> multichannel disabled on the server it confusingly returns "resource
> not available" we should at least log to dmesg something more
> meaningful than what we do today:
>
> [ 1195.349188] CIFS: VFS: failed to open extra channel on
> iface:10.45.126.66 rc=-11
> [ 1195.454361] CIFS: successfully opened new channel on
> iface:2607:fb90:f2b6:0732:7504:183c:991e:6e53
> [ 1195.454599] CIFS: VFS: reconnect tcon failed rc = -11
> [ 1195.457025] CIFS: VFS: reconnect tcon failed rc = -11
> [ 1195.457040] CIFS: VFS: cifs_read_super: get root inode failed
>
>
> Samba behavior is also strange - it advertises multichannel support in
> negprot response but doesn't advertise it in session setup flags.

If the user mounts with multichannel enabled, then it is expected that
the client will try to setup more channels.
If the server cannot support it, these logs are expected.

Btw.. It looks like the client was able to setup one additional
channel above. But reconnect tcon failed. I wonder why that happened?

>
> On Mon, Sep 22, 2025 at 3:25 AM <rajasimandalos@gmail.com> wrote:
> >
> > From: Rajasi Mandal <rajasimandal@microsoft.com>
> >
> > Previously, specifying both multichannel and max_channels=1 as mount
> > options would leave multichannel enabled, even though it is not
> > meaningful when only one channel is allowed. This led to confusion and
> > inconsistent behavior, as the client would advertise multichannel
> > capability but never establish secondary channels.
> >
> > Fix this by forcing multichannel to false whenever max_channels=1,
> > ensuring the mount configuration is consistent and matches user intent.
> > This prevents the client from advertising or attempting multichannel
> > support when it is not possible.
> >
> > Signed-off-by: Rajasi Mandal <rajasimandal@microsoft.com>
> > ---
> >  fs/smb/client/fs_context.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> > index 072383899e81..43552b44f613 100644
> > --- a/fs/smb/client/fs_context.c
> > +++ b/fs/smb/client/fs_context.c
> > @@ -1820,6 +1820,13 @@ static int smb3_fs_context_parse_param(struct fs_context *fc,
> >                 goto cifs_parse_mount_err;
> >         }
> >
> > +       /*
> > +        * Multichannel is not meaningful if max_channels is 1.
> > +        * Force multichannel to false to ensure consistent configuration.
> > +        */
> > +       if (ctx->multichannel && ctx->max_channels == 1)
> > +               ctx->multichannel = false;
> > +
> >         return 0;
> >
> >   cifs_parse_mount_err:
> > --
> > 2.43.0
> >
> >
>
>
> --
> Thanks,
>
> Steve
>


-- 
Regards,
Shyam