Message-ID: <CANT5p=qsh+jJ2PD2vy1x7ZjE646Hh28n1epS7Fy=RSqmuht_=g@mail.gmail.com>
Date: Tue, 23 Sep 2025 11:42:36 +0530
From: Shyam Prasad N <nspmangalore@...il.com>
To: Henrique Carvalho <henrique.carvalho@...e.com>
Cc: rajasimandalos@...il.com, Rajasi Mandal <rajasimandal@...rosoft.com>, sfrench@...ba.org, 
	pc@...guebit.org, ronniesahlberg@...il.com, sprasad@...rosoft.com, 
	tom@...pey.com, bharathsm@...rosoft.com, linux-kernel@...r.kernel.org, 
	linux-cifs@...r.kernel.org
Subject: Re: [PATCH 2/2] cifs: client: allow changing multichannel mount
 options on remount

On Tue, Sep 23, 2025 at 2:52 AM Henrique Carvalho
<henrique.carvalho@...e.com> wrote:
>
> Hi Rajasi,
>
> On 9/22/25 5:24 AM, 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(-)
> >
> > 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);
> > +}
> > +
>
> The other writer of chan_max is when creating a session. Is this lock
> really avoiding a race here?

It's not just the writers, but also readers that we protect with this
lock. I don't have major objections to the locking here.

>
> >  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)
> >  {
>
> Maybe you could get rid of from_reconfigure parameter if you just set
> chan_max to 1 before calling cifs_decrease_secondary_channels when this
> function is not called from smb3_reconfigure. What do you think?

chan_max today is set based on "max_channels" mount option that the user sets.
I don't think we should be modifying that value. Unless we change it's
meaning today. :)

>
> >       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 {
>
> Maybe you can avoid entering any cifs_chan_skip_or_disable if
> ctx->max_channels == ses->chan_max. There is a cost of holding locks
> inside of it.
>
> > +             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",
> > @@ -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);
> >
> >       memcpy(pneg_inbuf->Guid, server->client_guid,
> >                                       SMB2_CLIENT_GUID_SIZE);
> > diff --git a/fs/smb/client/smb2pdu.h b/fs/smb/client/smb2pdu.h
> > index 3c09a58dfd07..d3f63a4ef426 100644
> > --- a/fs/smb/client/smb2pdu.h
> > +++ b/fs/smb/client/smb2pdu.h
> > @@ -420,6 +420,8 @@ struct smb2_create_ea_ctx {
> >       struct smb2_file_full_ea_info ea;
> >  } __packed;
> >
> > +int smb3_sync_ses_channels(struct cifs_sb_info *cifs_sb);
> > +
> >  #define SMB2_WSL_XATTR_UID           "$LXUID"
> >  #define SMB2_WSL_XATTR_GID           "$LXGID"
> >  #define SMB2_WSL_XATTR_MODE          "$LXMOD"
>
> I also agree with Enzo that we could have an update_channels that
> centralizes the logic of rescaling channels, but that could also come in
> another patch as Steve suggested.
Ack. We'll review that.

>
> --
> Henrique
> SUSE Labs
>

Thank you for reviewing.

-- 
Regards,
Shyam

Powered by blists - more mailing lists