Message-ID: <4htyleuklcdod3togdqcjl5w7g3j5cmwcc4bww5knlwgwhtsbd@a7s2fa2nvcup>
Date: Mon, 22 Sep 2025 12:12:56 -0300
From: Enzo Matsumiya <ematsumiya@...e.de>
To: rajasimandalos@...il.com
Cc: linux-cifs@...r.kernel.org, sfrench@...ba.org, pc@...guebit.org, 
	ronniesahlberg@...il.com, sprasad@...rosoft.com, tom@...pey.com, bharathsm@...rosoft.com, 
	linux-kernel@...r.kernel.org, Rajasi Mandal <rajasimandal@...rosoft.com>
Subject: Re: [PATCH 2/2] cifs: client: allow changing multichannel mount
 options on remount

On 09/22, rajasimandalos@...il.com wrote:
>From: Rajasi Mandal <rajasimandal@...rosoft.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@...rosoft.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

Powered by blists - more mailing lists