Message-ID: <mrcj7b6wyh5hyqwgufasxpwxytllzuboro4fjunrqro6eq3siz@6ncku7sdjpkn>
Date: Tue, 23 Sep 2025 11:21:18 -0300
From: Enzo Matsumiya <ematsumiya@...e.de>
To: Shyam Prasad N <nspmangalore@...il.com>
Cc: Henrique Carvalho <henrique.carvalho@...e.com>, 
	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 09/23, Shyam Prasad N wrote:
>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?

That locked write on ses creation is also useless as it's done before ses
is even on list i.e. no other thread can even see this ses yet.

>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.

I couldn't find any _locked_ readers of ses->chan_max, did I miss
anything?

>> >  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