Re: [PATCH v9 20/29] scsi-multipath: provide callbacks for path state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: scsi_multipath: scsi_mpath_available_path() ignores
  sdev->access_state
- [Medium] scsi: scsi_multipath: mpath_device access_state never updated
--

commit 6f83cc4eccb483172140b55837b87755b4421b91
Author: John Garry <john.garry@linux.dev>

scsi-multipath: provide callbacks for path state

This commit implements the .is_disabled, .is_optimized, and .available_path
multipath callbacks for SCSI multipath devices using sdev_state and
access_state.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index 4c965650c2bd3..2f47c39d82936 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -298,7 +298,56 @@ static struct bio *scsi_mpath_clone_bio(struct bio *bio)
>  	return clone;
>  }
>  
> +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;
> +
> +	return true;
> +}
> +
> +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: Medium]
Does scsi_mpath_is_optimized() disagree with mpath_device->access_state for
active/non-optimized paths?

scsi_multipath_sdev_init() initializes mpath_device->access_state to
MPATH_STATE_OPTIMIZED, and nothing updates it when sdev->access_state
changes:

lib/multipath.c:__mpath_find_path() {
	...
		switch(mpath_device->access_state) {
		case MPATH_STATE_OPTIMIZED:
		    if (distance < found_distance) {
			    found_distance = distance;
			    found = mpath_device;
		    }
		    break;
		case MPATH_STATE_NONOPTIMIZED:
	...
}

Because __mpath_find_path(), mpath_round_robin_path(), and
mpath_queue_depth_path() select paths using mpath_device->access_state
rather than mpdt->is_optimized(), an active/non-optimized path is still
treated as optimized during path selection.  Under the NUMA policy,
mpath_numa_path() then checks scsi_mpath_is_optimized() on every bio, sees
false, and calls __mpath_find_path() to rescan dev_list and pick the same
non-optimized path again on every I/O.

> +
> +static bool scsi_mpath_available_path(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;
> +	enum scsi_device_state sdev_state = READ_ONCE(sdev->sdev_state);
> +
> +	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 scsi_mpath_available_path() also check sdev->access_state?

When all paths are in SDEV_RUNNING with an unusable ALUA state such as
standby, unavailable, or offline, scsi_mpath_is_disabled() returns true for
every path and mpath_find_path() returns NULL.  Because
scsi_mpath_available_path() still returns true for any SDEV_RUNNING device,
mpath_bdev_submit_bio() queues bios onto mpath_head->requeue_list instead
of failing them with bio_io_error(), leaving I/O stuck on requeue_list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=20