[RFC] smb: client: define async channel update serialization

[RFC] smb: client: define async channel update serialization

[Pengpeng Hou]

Commit 556bb341f9f2 ("smb: client: introduce multichannel async work
during mount") moved multichannel setup out of the mount path and into
independent work items. Mounts can share an existing session, so more than
one worker can update the same channel array concurrently.

cifs_try_adding_channels() snapshots chan_count and the number of missing
channels under chan_lock, but drops that lock while each channel is opened.
Two workers can therefore act on the same initial count. More importantly,
cifs_ses_add_channel() publishes a new slot before taking session_mutex, and
its failure path later decrements the current chan_count. If another worker
has appended a slot in between, that rollback can remove the wrong tail.
At the array limit, a stale worker can also index chans[chan_count] after
another worker has filled the last slot.

I considered making the mount worker use
CIFS_SES_FLAG_SCALE_CHANNELS, but that flag is not a blocking lock. A worker
that fails to acquire it has no retry or completion to wait on. In addition,
smb2_reconnect() currently clears the flag on a shared exit path even when it
only observed another updater owning it, and an early channel-disable path
runs before the existing flag acquisition.

Extending session_mutex around slot publication and rollback would serialize
concurrent additions, but it does not by itself serialize the reconfigure
decrease path. Conversely, a new mutex around all of
smb3_update_ses_channels() needs an explicit lock order because reconnect can
call channel removal while already holding session_mutex, whereas channel
addition later acquires session_mutex for session setup.

Would you prefer this to be fixed by coalescing mount requests onto one
per-session work item, or by introducing a dedicated channel-update
serialization primitive with an agreed lock order? Once that lifetime and
locking contract is clear, I can prepare a narrow patch and keep the capacity
check at the actual chans[] append as a final invariant.

Re: [RFC] smb: client: define async channel update serialization

[Henrique Carvalho]

On Wed Jul 22, 2026 at 1:10 AM -03, Pengpeng Hou wrote:
> Commit 556bb341f9f2 ("smb: client: introduce multichannel async work
> during mount") moved multichannel setup out of the mount path and into
> independent work items. Mounts can share an existing session, so more than
> one worker can update the same channel array concurrently.
>
> cifs_try_adding_channels() snapshots chan_count and the number of missing
> channels under chan_lock, but drops that lock while each channel is opened.
> Two workers can therefore act on the same initial count. More importantly,
> cifs_ses_add_channel() publishes a new slot before taking session_mutex, and
> its failure path later decrements the current chan_count. If another worker
> has appended a slot in between, that rollback can remove the wrong tail.
> At the array limit, a stale worker can also index chans[chan_count] after
> another worker has filled the last slot.
>
> I considered making the mount worker use
> CIFS_SES_FLAG_SCALE_CHANNELS, but that flag is not a blocking lock. A worker
> that fails to acquire it has no retry or completion to wait on.

I have a patch that uses the delaying idea, please see: https://lore.kernel.org/linux-cifs/20260429205236.456099-2-henrique.carvalho@suse.com/

It should be rebased and tweaked, though, I have been delaying this for
a while now.

Do you have any opinion?

> In addition,
> smb2_reconnect() currently clears the flag on a shared exit path even when it
> only observed another updater owning it,

Actually this looks like a bug to me, no? Why is smb2_reconnect()
clearing a flag it did not acquire?

> and an early channel-disable path
> runs before the existing flag acquisition.
>
> Extending session_mutex around slot publication and rollback would serialize
> concurrent additions, but it does not by itself serialize the reconfigure
> decrease path. Conversely, a new mutex around all of
> smb3_update_ses_channels() needs an explicit lock order because reconnect can
> call channel removal while already holding session_mutex, whereas channel
> addition later acquires session_mutex for session setup.
>
> Would you prefer this to be fixed by coalescing mount requests onto one
> per-session work item, or by introducing a dedicated channel-update
> serialization primitive with an agreed lock order? Once that lifetime and
> locking contract is clear, I can prepare a narrow patch and keep the capacity
> check at the actual chans[] append as a final invariant.

I like the idea to put everything in the same worker so instead of
having a mchan_mount work have just an mchan_work and reschedule it from
mount / remount / reconnect.

I have started this but abandoned because I couldn't find a good enough
reason to change things. It just seemed I was moving the complexity
elsewhere without any real benefit.

-- 
Henrique