Re: [PATCH 2/2] cifs: client: allow changing multichannel mount options on remount
From: Enzo Matsumiya
Date: Tue Sep 23 2025 - 14:01:57 EST
On 09/23, Steve French wrote:
IMO, like I replied to Shyam, is that the worker should only run if the userrequested 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@xxxxxxx> 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@xxxxxxx>
wrote:
>
>> On 09/22, rajasimandalos@xxxxxxxxx wrote:
>> >From: Rajasi Mandal <rajasimandal@xxxxxxxxxxxxx>
>> >
>> >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@xxxxxxxxxxxxx>
>> >---
>> > 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
>>