Re: [PATCH v6 20/27] scsi-multipath: add scsi_mpath_{start,end}_request()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Permanent leak of mpath_nr_active counter and master block device in-flight IO stats when a requeued multipath request fails with a hard error.
--

--- Patch [20]: [PATCH v6 20/27] scsi-multipath: add scsi_mpath_{start,end}_request() ---
commit 1a27bdf4453d197f11533d04e7997e7873652eb4
Author: John Garry <[email protected]>

scsi-multipath: add scsi_mpath_{start,end}_request()

Add scsi_mpath_{start,end}_request() to handle updating private multipath
request data, like nvme_mpath_{start,end}_request().

New member Scsi_Host.mpath_nr_active is added. This is required for queue
depth multipath iopolicy.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 6a2d7dbfbe8c3..ce9a2c7fbe707 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
[ ... ]
> @@ -1917,6 +1920,9 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
>  	cmd->submitter = SUBMITTED_BY_BLOCK_LAYER;
>  
> +	if (sdev->scsi_mpath_dev && is_mpath_request(req))
> +		scsi_mpath_start_request(req);
> +
>  	blk_mq_start_request(req);
>  	if (blk_mq_is_reserved_rq(req)) {
>  		reason = shost->hostt->queue_reserved_command(shost, cmd);

[Severity: High]
Can this permanently leak the mpath_nr_active counter and master block device
in-flight IO stats when a requeued multipath request fails with a hard error?

When a multipath request is dispatched, scsi_mpath_start_request() increments
mpath_nr_active and sets SCMD_MPATH_CNT_ACTIVE. If the dispatch then fails
with BLK_STS_RESOURCE (e.g. due to a full queue), the request is requeued and
the multipath flags are preserved by SCMD_PRESERVED_FLAGS.

On a subsequent retry, if the device has transitioned to an offline state
(which is common during path failure), the early state check in
scsi_queue_rq() returns an error before scsi_mpath_start_request() is
reached:

	if (unlikely(sdev->sdev_state != SDEV_RUNNING)) {
		ret = scsi_device_state_check(sdev, req);
		if (ret != BLK_STS_OK)
			goto out_put_budget;
	}

Returning an error here causes the block layer to directly complete the
request, bypassing the driver's normal scsi_end_request() completion path.
The block layer will eventually call the scsi_cleanup_rq() hook, which merely
zeroes cmd->flags:

	cmd->flags = 0;

but fails to decrement mpath_nr_active or call bdev_end_io_acct().

Would this leave the queue-depth multipath I/O policy perceiving an
artificially inflated load on the controller, permanently degrading load
balancing?

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