Message-ID: <vs3r33bqfug44hyax2o4zxtuop7m5w3npfbvm7belb6veto3dx@dx5pihhf6ovn>
Date: Tue, 23 Sep 2025 15:12:08 -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:
>The interface info in debug data that we get at mount time I have found
>useful for debugging non multichannel cases. I don't have strong opinions
>about whether requerying it occasionally in non multichannel case is of
>value but clearly has been useful to get it at mount time to populate Debug
>data

Steve, sorry, are you reading what I'm proposing/saying here?

mount.cifs -o multichannel,...
-> fills debug data, starts query_interface, try/add chans, yadda yadda


mount.cifs -o nomultichannel,...
-> do nothing related to multichannel


What's so wrong/problematic with having it that way?

>Thanks,
>
>Steve
>
>On Tue, Sep 23, 2025, 1:01 PM Enzo Matsumiya <ematsumiya@...e.de> wrote:
>
>> 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