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

[John Garry <john.garry@linux.dev>]

On 9/7/26 13:09, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Leaked multipath active counter (`mpath_nr_active`) and corrupted IO stats when `scsi_queue_rq` returns a hard error or `blk_mq` cancels a requeued request.
> --
> 
> commit 2b78aa9717f460c8d3814043976386ee7f6864c3
> Author: John Garry <john.garry@linux.dev>
> 
> scsi-multipath: add scsi_mpath_{start,end}_request()
> 
> This patch adds scsi_mpath_start_request() and scsi_mpath_end_request() to
> manage multipath request data. It also introduces mpath_nr_active to track
> the number of active requests per SCSI host.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 1a6f5e1fd80cb..193e7e9a2d30b 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
> [ ... ]
>> @@ -1918,6 +1921,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)) {
> 
> [Severity: High]

Same response as v7 - I think that the SCSI core needs to be fixed here

> Can this leak the mpath_nr_active counter and corrupt IO stats if a request
> encounters a hard error after being requeued?
> 
> For example, if a request is dispatched and scsi_mpath_start_request()
> increments the counter and sets SCMD_MPATH_CNT_ACTIVE:
> 
> If the dispatch returns a non-fatal error (like BLK_STS_RESOURCE), the
> block layer requeues the request. The SCMD_MPATH_CNT_ACTIVE flag is
> preserved across the requeue via SCMD_PRESERVED_FLAGS.
> 
> On the next dispatch attempt, if the device goes offline,
> scsi_device_state_check() fails early, jumping to the error path in
> scsi_queue_rq():
> 
> 	out_put_budget:
> 	...
> 	default:
> 		if (unlikely(!scsi_device_online(sdev)))
> 			cmd->result = DID_NO_CONNECT << 16;
> 		...
> 		if (req->rq_flags & RQF_DONTPREP)
> 			scsi_mq_uninit_cmd(cmd);
> 
> scsi_queue_rq() then returns the hard error to the block layer, which
> completes the request directly, bypassing scsi_end_request() and
> scsi_mpath_end_request().
> 
> Since the counter and stats are never decremented, will this permanently
> skew the in-flight IO stats and eventually stall I/O when MPATH_IOPOLICY_QD
> is used?
> 
>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>> index 8ecfb94049db5..52227e2cd9749 100644
>> --- a/include/scsi/scsi_cmnd.h
>> +++ b/include/scsi/scsi_cmnd.h
>> @@ -58,8 +58,11 @@ struct scsi_pointer {
>>    */
>>   #define SCMD_FORCE_EH_SUCCESS	(1 << 3)
>>   #define SCMD_FAIL_IF_RECOVERING	(1 << 4)
>> +#define SCMD_MPATH_IO_STATS	(1 << 5)
>> +#define SCMD_MPATH_CNT_ACTIVE	(1 << 6)
>>   /* flags preserved across unprep / reprep */
>> -#define SCMD_PRESERVED_FLAGS	(SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING)
>> +#define SCMD_PRESERVED_FLAGS	(SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING | \
>> +				SCMD_MPATH_IO_STATS | SCMD_MPATH_CNT_ACTIVE)
> 
> [Severity: High]
> Does adding these flags to SCMD_PRESERVED_FLAGS cause the issue mentioned
> above by masking the fact that the request already holds the mpath active
> counter when it comes back through scsi_queue_rq()?
>