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

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.

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

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?

[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.

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.

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?

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

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
during device removal in scsi_mpath_head_release():

drivers/scsi/scsi_multipath.c:scsi_mpath_head_release()
static void scsi_mpath_head_release(struct device *dev)
{
    ...
    mpath_head_uninit(mpath_head);
    kfree(scsi_mpath_head);
}

Since scsi_mpath_revalidate_paths() asynchronously schedules the
requeue_work task, if the device is removed and the refcount drops to zero,
the structure is freed without calling cancel_work_sync().

Could this allow pending work to execute on the freed scsi_mpath_head
pointer?

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