Re: [PATCH 1/2] cifs: client: force multichannel=off when max_channels=1

[Henrique Carvalho <henrique.carvalho@suse.com>]

On 9/22/25 3:52 PM, Enzo Matsumiya wrote:
> On 09/22, Henrique Carvalho wrote:
>>
>>
>> On 9/22/25 1:14 PM, Steve French wrote:
>>> . >Do we even need ->multichannel flag at all?
>>>
>>> Yes - especially in the future.   The goal is for the user to have
>>> three options:
>>> 1) (preferred) "multichannel" (max channels will be dynamically
>>> selected and can change) the client gets to choose how many channels
>>> to connect to based on what it sees in the output of the most recent
>>> query interfaces call (it can change on the fly as server dynamically
>>> adds and removes channels or networks become temporarily unreachable)
>>
>> I'm guessing this would be required while we are transitioning from
>> setting channels dynamically to having multichannel on by default, as
>> you commented below. Because once we have it on by default, I don't
>> think there is a point in having the flag.
> 
> Exactly.
> 
>>> 2) "max_channels="   This is for the case where user wants to choose
>>> the number of channels rather than have the client automatically
>>> (hopefully soon in the future) choose it for you
>>> 3) if server has mchan bugs, allow client to mount with no
>>> multichannel (or equivalent max_channels=1)
>>>
>>> But ... "remount" also has to work for the three above (and currently
>>> doesn't) and this is what I am hoping the recent patches can fix (?)
>>> but haven't tested them enough yet
> 
> I was talking more in the context of code, that it could use some
> refactoring/improvements -- I also think such functionality (hence the
> patches) are necessary.
> 
> There's no sense for me, as a user, to specify e.g.:
>   # mount.cifs -o ...,multichannel,max_channels=2 ...
> 
> Was there only a single option for this, it would be less confusing and
> wouldn't require this patch here.
> 
> 
> The below patch (PoC) is an idea I had that would make things much
> clearer for users -- have 'multipath' mount option be either a flag or a
> value option, e.g.:
> 
>   # mount.cifs -o ...,multichannel //srv/share /mnt/test
>   # findmnt -t cifs -u | grep -Eo 'max_channels=[0-9]+'
>   max_channels=2
>   # umount /mnt/test
>   # mount.cifs -o ...,multichannel=4 //srv/share /mnt/test
>   # findmnt -t cifs -u | grep -Eo 'max_channels=[0-9]+'
>   max_channels=4
> 
> 

I agree. This is clearer and we can drop one flag.

> 
> ---------------------
> 
> diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> index dd12f3eb61dc..ad9a588c7103 100644
> --- a/fs/smb/client/connect.c
> +++ b/fs/smb/client/connect.c
> @@ -2487,7 +2487,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server,
> struct smb3_fs_context *ctx)
>      spin_lock(&ses->chan_lock);
>      ses->chans[0].server = server;
>      ses->chan_count = 1;
> -    ses->chan_max = ctx->multichannel ? ctx->max_channels:1;
> +    ses->chan_max = ctx->max_channels;
>      ses->chans_need_reconnect = 1;
>      spin_unlock(&ses->chan_lock);
>  
> diff --git a/fs/smb/client/fs_context.c b/fs/smb/client/fs_context.c
> index 072383899e81..ceb19a58b743 100644
> --- a/fs/smb/client/fs_context.c
> +++ b/fs/smb/client/fs_context.c
> @@ -165,7 +165,7 @@ const struct fs_parameter_spec smb3_fs_parameters[] = {
>      fsparam_u32("max_cached_dirs", Opt_max_cached_dirs),
>      fsparam_u32("handletimeout", Opt_handletimeout),
>      fsparam_u64("snapshot", Opt_snapshot),
> -    fsparam_u32("max_channels", Opt_max_channels),
> +    fsparam_u32("multichannel", Opt_multichannel),
>  
>      /* Mount options which take string value */
>      fsparam_string("source", Opt_source),
> @@ -1252,14 +1252,16 @@ static int smb3_fs_context_parse_param(struct
> fs_context *fc,
>          ctx->nodelete = 1;
>          break;
>      case Opt_multichannel:
> -        if (result.negated) {
> -            ctx->multichannel = false;
> -            ctx->max_channels = 1;
> -        } else {
> -            ctx->multichannel = true;
> -            /* if number of channels not specified, default to 2 */
> -            if (ctx->max_channels < 2)
> +        if (param->type == fs_value_is_flag) {
> +            if (!result.negated && ctx->max_channels < 2)
>                  ctx->max_channels = 2;
> +        } else {
> +            if (result.uint_32 < 1 || result.uint_32 >
> CIFS_MAX_CHANNELS) {
> +                cifs_errorf(fc, "%s: Invalid max_channels value, needs
> to be 1-%d\n",
> +                     __func__, CIFS_MAX_CHANNELS);
> +                goto cifs_parse_mount_err;
> +            }
> +            ctx->max_channels = result.uint_32;
>          }
>          break;
>      case Opt_uid:
> @@ -1395,17 +1397,6 @@ static int smb3_fs_context_parse_param(struct
> fs_context *fc,
>          }
>          ctx->max_credits = result.uint_32;
>          break;
> -    case Opt_max_channels:
> -        if (result.uint_32 < 1 || result.uint_32 > CIFS_MAX_CHANNELS) {
> -            cifs_errorf(fc, "%s: Invalid max_channels value, needs to
> be 1-%d\n",
> -                 __func__, CIFS_MAX_CHANNELS);
> -            goto cifs_parse_mount_err;
> -        }
> -        ctx->max_channels = result.uint_32;
> -        /* If more than one channel requested ... they want multichan */
> -        if (result.uint_32 > 1)
> -            ctx->multichannel = true;
> -        break;
>      case Opt_max_cached_dirs:
>          if (result.uint_32 < 1) {
>              cifs_errorf(fc, "%s: Invalid max_cached_dirs, needs to be 1
> or more\n",
> @@ -1901,7 +1892,6 @@ int smb3_init_fs_context(struct fs_context *fc)
>      ctx->echo_interval = SMB_ECHO_INTERVAL_DEFAULT;
>  
>      /* default to no multichannel (single server connection) */
> -    ctx->multichannel = false;
>      ctx->max_channels = 1;
>  
>      ctx->backupuid_specified = false; /* no backup intent for a user */
> diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
> index b0fec6b9a23b..ff75a7cc11dd 100644
> --- a/fs/smb/client/fs_context.h
> +++ b/fs/smb/client/fs_context.h
> @@ -175,7 +175,6 @@ enum cifs_param {
>      Opt_max_credits,
>      Opt_max_cached_dirs,
>      Opt_snapshot,
> -    Opt_max_channels,
>      Opt_handletimeout,
>  
>      /* Mount options which take string value */
> @@ -293,7 +292,6 @@ struct smb3_fs_context {
>      bool resilient:1; /* noresilient not required since not fored for
> CA */
>      bool domainauto:1;
>      bool rdma:1;
> -    bool multichannel:1;
>      bool use_client_guid:1;
>      /* reuse existing guid for multichannel */
>      u8 client_guid[SMB2_CLIENT_GUID_SIZE];
> 

-- 
Henrique
SUSE Labs