Re: [PATCH v3 09/17] scsi-multipath: add scsi_mpath_{start,end}_request()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Requeueing a command causes permanent leaks of `mpath_nr_active` and block device in-flight I/O statistics because `SCMD_MPATH_*` flags are not preserved.
- [High] Premature termination of I/O accounting and double-accounting on partial request completions because `scsi_mpath_end_request()` is called before `blk_update_request()`.
- [Low] Implicit function declarations of `is_mpath_request` and `mpath_qd_iopolicy` cause build failures.
--

commit 6d78b641877657fb26302d1c7965b47fd751383b
Author: John Garry <john.g.garry@oracle.com>

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

This patch adds scsi_mpath_start_request() and scsi_mpath_end_request() to
handle updating private multipath request data. It also introduces
mpath_nr_active to Scsi_Host to support queue depth multipath iopolicy.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 3c493068adb98..72ba066235911 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -669,6 +669,9 @@ static bool scsi_end_request(struct request *req, blk_status_t error,
>  	struct scsi_device *sdev = cmd->device;
>  	struct request_queue *q = sdev->request_queue;
>  
> +	if (is_mpath_request(req))
> +		scsi_mpath_end_request(req);
> +

[Severity: Low]
Are the functions is_mpath_request() and mpath_qd_iopolicy() defined anywhere?
It looks like they are used in this patch but not declared, which would cause
a build failure.

[Severity: High]
Does calling scsi_mpath_end_request() before blk_update_request() cause double
accounting on partial completions?

If blk_update_request() returns true for a partial completion, the request is
retried. Since scsi_mpath_end_request() has already decremented
mpath_nr_active and called bdev_end_io_acct() without clearing the state
flags, a later completion will decrement these again. Should this be called
after blk_update_request() finishes the entire request?

>  	if (blk_update_request(req, error, bytes))
>  		return true;
>  
[ ... ]
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index 8ecfb94049db5..c6571a36e577b 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -60,6 +60,8 @@ struct scsi_pointer {
>  #define SCMD_FAIL_IF_RECOVERING	(1 << 4)
>  /* flags preserved across unprep / reprep */
>  #define SCMD_PRESERVED_FLAGS	(SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING)
> +#define SCMD_MPATH_IO_STATS	(1 << 5)
> +#define SCMD_MPATH_CNT_ACTIVE	(1 << 6)

[Severity: High]
Can we leak mpath_nr_active and in-flight I/O statistics when a command is
requeued?

In scsi_queue_rq(), cmd->flags is masked with SCMD_PRESERVED_FLAGS. Because
SCMD_MPATH_IO_STATS and SCMD_MPATH_CNT_ACTIVE are not added to
SCMD_PRESERVED_FLAGS, they are cleared on every retry.

When scsi_mpath_start_request() runs again on a retry, it will see the flags
as unset, increment mpath_nr_active, and call bdev_start_io_acct() a second
time. Upon completion, scsi_mpath_end_request() only cleans up once, leaving a
permanent leak. Should these new flags be added to SCMD_PRESERVED_FLAGS?

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