Message-ID: <vwwwvo2ehhlvq64vzp3j62asaryc6kkw34rjnkf7ozssv3hzyd@vwmxraotzwo6>
Date: Tue, 23 Sep 2025 14:32:24 -0300
From: Enzo Matsumiya <ematsumiya@...e.de>
To: RAJASI MANDAL <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/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@...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.
>>
>> >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