Message-ID: <d09c8506-6326-45e6-8687-8e0b002eff62@suse.com>
Date: Mon, 22 Sep 2025 18:11:49 -0300
From: Henrique Carvalho <henrique.carvalho@...e.com>
To: rajasimandalos@...il.com, Rajasi Mandal <rajasimandal@...rosoft.com>
Cc: 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

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?

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

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

-- 
Henrique
SUSE Labs

Powered by blists - more mailing lists