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

[John Garry <john.g.garry@oracle.com>]

On 03/07/2026 12:49, sashiko-bot@kernel.org wrote:
> 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?

will fix

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

will fix

> 
>> +}
>> +
>> +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.

It does not specifically route IO to it, but allows IO to be requeued 
for the head to be issued later

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

probably

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