[PATCH v9 01/29] scsi: core: Clear cmd->flags for terminal queuing error

[Bart Van Assche]

On 11/8/26 12:47 PM, John Garry wrote:
 > If a request can't be queued in scsi_queue_rq(), then we error out
 > with a blk_status_t code. Such a scenario may be the scsi_device has
 > its transport offline. For BLK_STS_RESOURCE or BLK_STS_AGAIN, the
 > request will be requeued in the block layer.

This is not correct for BLK_STS_AGAIN. Only BLK_STS_RESOURCE and
BLK_STS_DEV_RESOURCE cause the block layer to requeue a request. For
all other status codes, including BLK_STS_AGAIN,
blk_mq_dispatch_rq_list() and blk_mq_try_issue_directly() call
blk_mq_end_request().

 > For other errors, the block layer ends the request and so we won't
 > see it again (until recycled). Normally when a request completes
 > scmd->flags is cleared in scsi_end_request(). However for the error
 > scenario mentioned above, scsi_end_request() is not called and so
 > scmd->flags is not cleared. As such, when the request is recycled we
 > see the old value in scmd->flags.

Please describe the consequences of this bug in the patch description.
As far as I can tell there are at least two:
- SCMD_FAIL_IF_RECOVERING is part of SCMD_PRESERVED_FLAGS and
   scsi_execute_cmd() ORs new flags into scmd->flags. If a command
   submitted with SCMD_FAIL_IF_RECOVERING is failed with BLK_STS_OFFLINE
   because the host is in recovery, the flag leaks into the next request
   that is allocated with the same tag. That request will then be failed
   instead of requeued if it is submitted while the SCSI error handler
   is active.
- If SCMD_INITIALIZED leaks, scsi_init_command() skips
   scsi_initialize_rq() for the next filesystem request. Hence,
   jiffies_at_alloc and retries are not reinitialized and that request
   may be failed prematurely by scsi_cmd_runtime_exceeded().

 > Fix this issue by clearing scmd->flags in the error scenario mentioned
 > by calling scsi_cleanup_rq().

Please add Fixes: and Cc: stable tags. For the SCMD_FAIL_IF_RECOVERING
leak, I think this should be:

Fixes: 310bcaef6d7e ("scsi: core: Support failing requests while 
recovering")

 > @@ -1994,8 +1994,8 @@ static blk_status_t scsi_queue_rq(struct 
blk_mq_hw_ctx *hctx,
 >  		 * we hit an error, as we will never see this command
 >  		 * again.
 >  		 */
 > -		if (req->rq_flags & RQF_DONTPREP)
 > -			scsi_mq_uninit_cmd(cmd);
 > +		scsi_cleanup_rq(req);
 > +		scsi_run_queue_async(sdev);

Since the block layer also ends requests for which scsi_queue_rq()
returns BLK_STS_AGAIN, the BLK_STS_AGAIN case has the same issue.
Please convert that case too:

	case BLK_STS_AGAIN:
		cmd->result = DID_BUS_BUSY << 16;
		scsi_cleanup_rq(req);
		break;

Although no SCSI driver returns BLK_STS_AGAIN today, I think both
error paths should handle this in the same way.

Thanks,

Bart.