Message-ID: <23xlv6coiyaxwdfitprfvwfmipi5avk4of27hqypukibm2mkop@44sd5ici4sis>
Date: Tue, 23 Sep 2025 15:01:52 -0300
From: Enzo Matsumiya <ematsumiya@...e.de>
To: Steve French <smfrench@...il.com>
Cc: RAJASI MANDAL <rajasimandalos@...il.com>, 
	CIFS <linux-cifs@...r.kernel.org>, Steve French <sfrench@...ba.org>, 
	Paulo Alcantara <pc@...guebit.org>, ronnie sahlberg <ronniesahlberg@...il.com>, 
	Shyam Prasad N <sprasad@...rosoft.com>, Tom Talpey <tom@...pey.com>, 
	Bharath S M <bharathsm@...rosoft.com>, LKML <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, 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@...e.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@...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