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 <[email protected]> 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/[email protected]?part=20
