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

On 27/07/2026 16:24, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] scsi-multipath fails to kick the multipath head's requeue list when underlying paths transition back to a usable state, causing pending I/Os to hang indefinitely.
> - [High] Core path selectors ignore the newly added `.is_optimized` callback, breaking ALUA priorities for round-robin and queue-depth I/O policies.
> - [High] LBA-dependent ALUA paths are incorrectly treated as disabled, leading to complete I/O failure for LBA-dependent arrays.
> --
> 
> --- Patch [19]: [PATCH v5 19/27] scsi-multipath: provide callbacks for path state ---
> commit c99fff5c406ddb30334df26d8cb6dcc7c86e7a13
> Author: John Garry <[email protected]>
> 
> scsi-multipath: provide callbacks for path state
> 
> Provide callbacks for .is_disabled, .is_optimized, and .available_path.
> These all use scsi_device.sdev_state and scsi_device.access_state.
> Member scsi_device.access_state will be driven by ALUA.
> 
>> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
>> index ec611f26d7960..d79e1b7b90051 100644
>> --- a/drivers/scsi/scsi_multipath.c
>> +++ b/drivers/scsi/scsi_multipath.c
>> @@ -314,7 +314,56 @@ static struct bio *scsi_mpath_clone_bio(struct bio *bio)
> 
> [ ... ]
> 
>> +static bool scsi_mpath_is_disabled(struct mpath_device *mpath_device)
>> +{
>> +	struct scsi_mpath_device *scsi_mpath_dev =
>> +				to_scsi_mpath_device(mpath_device);
>> +	struct scsi_device *sdev = scsi_mpath_dev->sdev;
>> +	unsigned char access_state = READ_ONCE(sdev->access_state) &
>> +					SCSI_ACCESS_STATE_MASK;
>> +
>> +	if (READ_ONCE(sdev->sdev_state) != SDEV_RUNNING)
>> +		return true;
>> +
>> +	if (access_state == SCSI_ACCESS_STATE_OPTIMAL ||
>> +	    access_state == SCSI_ACCESS_STATE_ACTIVE)
>> +		return false;
> 
> [Severity: High]
> Does this code inadvertently exclude SCSI_ACCESS_STATE_LBA?
> 
> If a path is in SCSI_ACCESS_STATE_LBA, it is considered a valid, usable active
> path by ALUA. By returning true for any state other than OPTIMAL or ACTIVE,
> won't this cause complete I/O failure if all available paths are
> LBA-dependent?

We don't fully support ALUA yet

> 
>> +
>> +	return true;
>> +}
> 
> [Severity: High]
> If a path's state changes to become usable again (for example, transitioning
> from SDEV_QUIESCE back to SDEV_RUNNING in scsi_device_resume(), or when the
> ALUA state changes to SCSI_ACCESS_STATE_OPTIMAL asynchronously), how is the
> multipath head notified?
> 
> Without a corresponding transition hook in the SCSI core or ALUA handlers
> to invoke mpath_schedule_requeue_work(), won't pending I/Os on the
> requeue_list hang indefinitely instead of being retried?
> 
>> +
>> +static bool scsi_mpath_is_optimized(struct mpath_device *mpath_device)
>> +{
>> +	struct scsi_mpath_device *scsi_mpath_dev =
>> +				to_scsi_mpath_device(mpath_device);
>> +	struct scsi_device *sdev = scsi_mpath_dev->sdev;
>> +	unsigned char access_state = READ_ONCE(sdev->access_state) &
>> +					SCSI_ACCESS_STATE_MASK;
>> +
>> +	if (READ_ONCE(sdev->sdev_state) != SDEV_RUNNING)
>> +		return false;
>> +
>> +	return access_state == SCSI_ACCESS_STATE_OPTIMAL;
>> +}
> 
> [Severity: High]
> Are core path selectors actually invoking this new callback?

It's ok as we will make it work properly when ALUA is supported

> 
> Looking at the core selection algorithms in lib/multipath.c, such as
> __mpath_find_path() or mpath_round_robin_path(), it appears they check the
> static field mpath_device->access_state instead of invoking the
> is_optimized() callback.
> 
> Because scsi_multipath_sdev_init() hardcodes mpath_device->access_state to
> MPATH_STATE_OPTIMIZED, won't all SCSI paths be incorrectly treated as optimized
> by the core logic, bypassing ALUA path weighting for round-robin and
> queue-depth policies?
>