Re: [PATCH v6 19/27] scsi-multipath: provide callbacks for path state

On 29/07/2026 16:43, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> 
> New issues:
> - [High] `scsi_mpath_is_disabled()` returns true for STANDBY paths, preventing I/O and deadlocking implicit ALUA failovers.
> - [High] Bios placed on the `requeue_list` during an ALUA state change are permanently stalled due to missing notification hooks.
> - [High] ALUA path prioritization is broken because routing policies check a static struct field instead of the new `.is_optimized` callback.
> - [High] `scsi_mpath_available_path` ignores ALUA offline state, causing indefinite I/O hangs instead of failing.
> - [High] LBA-dependent paths are incorrectly flagged as disabled, preventing I/O.
> - [High] SDEV_CREATED state is omitted from available paths, causing premature I/O failures.
> - [High] I/O submitted during device quiesce (e.g., system suspend) will hang indefinitely because the requeue list is never flushed upon resume.
> 
> Pre-existing issues:
> - [High] Missing cancellation of background `requeue_work` task before tearing down and freeing the `scsi_mpath_head` object.
> --
> 
> --- Patch [19]: [PATCH v6 19/27] scsi-multipath: provide callbacks for path state ---
> commit a79ca9463c83fa5a5480f800bdc6d5f3fef27829
> Author: John Garry <[email protected]>
> 
> scsi-multipath: provide callbacks for path state
> 
> This commit provides callbacks for .is_disabled, .is_optimized, and
> .available_path. These callbacks use the scsi_device's sdev_state
> and access_state, which will be driven by ALUA.
> 
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
> 
> [ ... ]
> 
>> +static bool scsi_mpath_is_disabled(struct mpath_device *mpath_device)
>> +{
> 
> [ ... ]
> 
>> +	if (access_state == SCSI_ACCESS_STATE_OPTIMAL ||
>> +	    access_state == SCSI_ACCESS_STATE_ACTIVE)
>> +		return false;
>> +
>> +	return true;
>> +}
> 
> [Severity: High]
> Will this return true for STANDBY paths?
> 
> If implicit ALUA relies on sending I/O to a STANDBY path to transition it
> to ACTIVE via a Unit Attention, marking it disabled might prevent the core
> from routing any I/O to it, potentially deadlocking the failover.

ALUA support to be improved

> 
> [Severity: High]
> Does this also need to allow SCSI_ACCESS_STATE_LBA?

As above

> 
> SPC-4 defines LBA-dependent states as active and capable of processing I/O
> commands. Should this state also be permitted here to avoid stalling I/O
> on storage arrays that rely on it?
> 
>> +static bool scsi_mpath_available_path(struct mpath_device *mpath_device)
>> +{
> 
> [ ... ]
> 
>> +	if (sdev_state == SDEV_RUNNING || sdev_state == SDEV_QUIESCE ||
>> +	    sdev_state == SDEV_BLOCK || sdev_state == SDEV_CREATED_BLOCK)
>> +		return true;
>> +
>> +	return false;
>> +}
> 
> [Severity: High]
> Should SDEV_CREATED be included here?
> 
> During SCSI device probing, the device state is SDEV_CREATED and the core
> SCSI layer permits I/O operations like INQUIRY. If SDEV_CREATED is omitted
> here, could valid I/O be incorrectly failed as unavailable during device
> scanning?

Not relevant to multipath

> 
> [Severity: High]
> By allowing SDEV_QUIESCE here, bios will be placed on the requeue list.
> 
> When the device is later unquiesced during a system resume,
> scsi_device_resume() sets the state back to SDEV_RUNNING, but it does not
> seem to trigger scsi_mpath_revalidate_paths() or schedule requeue_work.

No bios should be queued when quiesed

> 
> Will bios queued during suspend remain stuck on the list indefinitely?
> 
> [Severity: High]
> This function only checks sdev_state.
> 
> If an ALUA path transitions to SCSI_ACCESS_STATE_OFFLINE, sdev_state remains
> SDEV_RUNNING, which causes this callback to return true.

ALUA support incomplete

> 
> Could this cause the block layer to endlessly requeue the I/O, believing
> the path is temporarily unavailable, rather than failing it immediately
> for being permanently offline?
> 
>>   static struct mpath_head_template smpdt = {
>> +	.is_disabled = scsi_mpath_is_disabled,
>> +	.is_optimized = scsi_mpath_is_optimized,
>> +	.available_path = scsi_mpath_available_path,
>>   	.clone_bio = scsi_mpath_clone_bio,
>>   };
> 
> [Severity: High]
> How does this integration handle ALUA state changes for requeued I/O?
> 
> When no active paths are found, bios are placed on the requeue list:
> 
> lib/multipath.c:mpath_bdev_submit_bio()
>      spin_lock_irq(&mpath_head->requeue_lock);
>      bio_list_add(&mpath_head->requeue_list, bio);
>      spin_unlock_irq(&mpath_head->requeue_lock);
> 
> When scsi_dh_alua.c handles an ALUA state change, does it notify
> scsi-multipath to schedule the requeue work? If not, could these bios
> remain permanently stalled?

ALUA support incomplete

> 
> [Severity: High]
> Are the routing algorithms in lib/multipath.c actually using this new
> .is_optimized callback?
> 
> lib/multipath.c:__mpath_find_path()
>      switch(mpath_device->access_state) {
>      case MPATH_STATE_OPTIMIZED:
> 
> It appears the path finder checks the statically initialized access_state
> field instead of invoking the new callback. Will this cause all active
> paths to be incorrectly treated as optimized, bypassing path prioritization?

used elsewhere