Re: [PATCH v3 08/17] scsi-multipath: provide callbacks for path state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `scsi_mpath_is_disabled` and `scsi_mpath_is_optimized` fail to mask out the `SCSI_ACCESS_STATE_PREFERRED` flag from `sdev->access_state`.
- [High] `scsi_mpath_available_path()` incorrectly treats `SDEV_CREATED_BLOCK` as an available path.
--

commit 71967b8cc1b7e6dc50c814782c317913564b9ee9
Author: John Garry <john.g.garry@oracle.com>

scsi-multipath: provide callbacks for path state

This patch provides callbacks for path states in SCSI multipath, including
scsi_mpath_is_disabled, scsi_mpath_is_optimized, and
scsi_mpath_available_path. These rely on the SCSI device's sdev_state and
access_state to determine path availability.

> diff --git a/drivers/scsi/scsi_multipath.c b/drivers/scsi/scsi_multipath.c
> index f22e3677cf2ad..d8ea9ffe8942c 100644
> --- a/drivers/scsi/scsi_multipath.c
> +++ b/drivers/scsi/scsi_multipath.c
> @@ -301,7 +301,53 @@ 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);
> +
> +	if (sdev->sdev_state != SDEV_RUNNING)
> +		return true;
> +
> +	if (access_state == SCSI_ACCESS_STATE_OPTIMAL ||
> +	    access_state == SCSI_ACCESS_STATE_ACTIVE)

[Severity: High]
Does this comparison correctly handle ALUA preferred paths?

If a device handler sets sdev->access_state to include the preferred bit
(SCSI_ACCESS_STATE_OPTIMAL | SCSI_ACCESS_STATE_PREFERRED), this raw byte
comparison will fail. This would cause the multipath layer to incorrectly
classify valid, optimal preferred paths as disabled, potentially discarding
them from I/O consideration.

Should this mask the access_state with SCSI_ACCESS_STATE_MASK before
comparing?

> +		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;
> +
> +	if (sdev->sdev_state != SDEV_RUNNING)
> +		return false;
> +
> +	return READ_ONCE(sdev->access_state) == SCSI_ACCESS_STATE_OPTIMAL;

[Severity: High]
Similar to the check in scsi_mpath_is_disabled(), will this strict equality
check misclassify optimal paths that also have the
SCSI_ACCESS_STATE_PREFERRED flag set?

> +}
> +
> +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 = sdev->sdev_state;
> +
> +	if (sdev_state == SDEV_RUNNING || sdev_state == SDEV_QUIESCE ||
> +	    sdev_state == SDEV_BLOCK || sdev_state == SDEV_CREATED_BLOCK)

[Severity: High]
Is it intended to route user I/O to a device in the SDEV_CREATED_BLOCK state?

Devices in SDEV_CREATED and SDEV_CREATED_BLOCK are newly created, not fully
initialized, and generally only accept internal commands. If a newly created
device is temporarily blocked, this function returns true and routes user I/O
to it, which could lead to I/O rejection or indefinite blocking.

Furthermore, when the device unblocks and returns to SDEV_CREATED, this
function would suddenly return false. Should SDEV_CREATED_BLOCK be excluded
from the available paths list?

> +		return true;
> +
> +	return false;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703103402.3725011-1-john.g.garry@oracle.com?part=8