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

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

On 9/1/26 12:25, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Bypassing `scsi_end_request()` on hard errors in `scsi_queue_rq()` permanently leaks multipath active counters and corrupts state for recycled blk-mq tags.
> --
> 
> commit d6985f792a0819a7b56a04e827a71ff5a8dd2172
> Author: John Garry <john.garry@linux.dev>
> 
> scsi-multipath: add scsi_mpath_{start,end}_request()
> 
> This commit adds scsi_mpath_start_request() and scsi_mpath_end_request() to
> handle private multipath request data. It also introduces mpath_nr_active
> per SCSI host to track active requests for queue depth io-policies.
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 189263ce56624..5916dead5422e 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)) {
>>   		reason = shost->hostt->queue_reserved_command(shost, cmd);
> 
> [ ... ]
> 
>> 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 preserving these multipath flags across requeue operations cause a leak
> of the mpath_nr_active counter and stale state for recycled tags?
> 
> If a multipath request is dispatched, it calls scsi_mpath_start_request()
> which increments the host's mpath_nr_active counter and sets these flags.
> 
> If this request is requeued (for example, if the host is busy) and then
> re-dispatched while the device is offline or entering recovery,
> scsi_device_state_check() in scsi_queue_rq() will fail early. This causes
> scsi_queue_rq() to return a hard error directly to the block layer.
> 
> Because the block layer completes the request natively on hard dispatch
> errors, it bypasses scsi_end_request(), leaving scsi_mpath_end_request()
> uncalled. The mpath_nr_active counter is leaked.

This seems like something which needs to be fixed in scsi midlayer

> 
> Furthermore, since SCMD_MPATH_IO_STATS and SCMD_MPATH_CNT_ACTIVE are
> preserved in SCMD_PRESERVED_FLAGS, any new request reusing this tag might
> inherit the stale flags, bypassing initialization in
> scsi_mpath_start_request().
> 
> Could there be a mechanism to clear these flags and decrement the counter
> when a requeued request encounters a hard failure during re-dispatch?
>