Message-ID: <CANT5p=oQaYZEjrkroP8sHw73GgT6tTX=Do6q_Mu8zmKettufHA@mail.gmail.com>
Date: Tue, 23 Sep 2025 11:34:46 +0530
From: Shyam Prasad N <nspmangalore@...il.com>
To: Enzo Matsumiya <ematsumiya@...e.de>
Cc: rajasimandalos@...il.com, 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 Mon, Sep 22, 2025 at 8:43 PM Enzo Matsumiya <ematsumiya@...e.de> wrote:
>
> 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.

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

Powered by blists - more mailing lists