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

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?

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

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19