[PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error

[PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error

[John Garry]

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. 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.

Fix this issue by clearing scmd->flags in the error scenario mentioned.

Signed-off-by: John Garry <john.garry@linux.dev>
---
Setting as an RFC as I am not 100% sure that this is correct as there are
lots of paths involved. Sashiko pointed out a related issue in
https://lore.kernel.org/linux-scsi/20260729161243.C918D1F00A3A@smtp.kernel.org/,
but blk_mq_cleanup_rq() -> scsi_cleanup_rq() -> cmd->flags = 0 is only
called for by DM code.

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index af27fd3df8d4..295f8b4afaa7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1967,6 +1967,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		 */
 		if (req->rq_flags & RQF_DONTPREP)
 			scsi_mq_uninit_cmd(cmd);
+		cmd->flags = 0;
 		scsi_run_queue_async(sdev);
 		break;
 	}
-- 
2.43.0

Re: [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error

[Bart Van Assche]

On 9/8/26 5:08 AM, 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. 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.
> 
> Fix this issue by clearing scmd->flags in the error scenario mentioned.
> 
> Signed-off-by: John Garry <john.garry@linux.dev>
> ---
> Setting as an RFC as I am not 100% sure that this is correct as there are
> lots of paths involved. Sashiko pointed out a related issue in
> https://lore.kernel.org/linux-scsi/20260729161243.C918D1F00A3A@smtp.kernel.org/,
> but blk_mq_cleanup_rq() -> scsi_cleanup_rq() -> cmd->flags = 0 is only
> called for by DM code.
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index af27fd3df8d4..295f8b4afaa7 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1967,6 +1967,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>   		 */
>   		if (req->rq_flags & RQF_DONTPREP)
>   			scsi_mq_uninit_cmd(cmd);
> +		cmd->flags = 0;
>   		scsi_run_queue_async(sdev);
>   		break;
>   	}

Is there agreement that all block drivers are affected that define a
.cleanup_rq() callback? Would this be a valid alternative (untested)?

diff --git a/block/blk-mq.c b/block/blk-mq.c
index a26a11c73ee3..0be933e26d67 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2126,6 +2126,12 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx 
*hctx, struct list_head *list,
  			blk_mq_handle_dev_resource(rq, list);
  			goto out;
  		default:
+			/*
+			 * Make sure to release all allocated resources when
+			 * we hit an error, as we will never see this command
+			 * again.
+			 */
+			blk_mq_cleanup_rq(rq);
  			blk_mq_end_request(rq, ret);
  		}
  	} while (!list_empty(list));
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 48aab0df30b7..636854bffd1e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1960,13 +1960,6 @@ static blk_status_t scsi_queue_rq(struct 
blk_mq_hw_ctx *hctx,
  			cmd->result = DID_NO_CONNECT << 16;
  		else
  			cmd->result = DID_ERROR << 16;
-		/*
-		 * Make sure to release all allocated resources when
-		 * we hit an error, as we will never see this command
-		 * again.
-		 */
-		if (req->rq_flags & RQF_DONTPREP)
-			scsi_mq_uninit_cmd(cmd);
  		scsi_run_queue_async(sdev);
  		break;
  	}

Thanks,

Bart.

Re: [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error

[John Garry]

On 9/9/26 17:32, Bart Van Assche wrote:
> On 9/8/26 5:08 AM, 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. 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.
>>
>> Fix this issue by clearing scmd->flags in the error scenario mentioned.
>>
>> Signed-off-by: John Garry <john.garry@linux.dev>
>> ---
>> Setting as an RFC as I am not 100% sure that this is correct as there are
>> lots of paths involved. Sashiko pointed out a related issue in
>> https://lore.kernel.org/linux- 
>> scsi/20260729161243.C918D1F00A3A@smtp.kernel.org/,
>> but blk_mq_cleanup_rq() -> scsi_cleanup_rq() -> cmd->flags = 0 is only
>> called for by DM code.
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index af27fd3df8d4..295f8b4afaa7 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1967,6 +1967,7 @@ static blk_status_t scsi_queue_rq(struct 
>> blk_mq_hw_ctx *hctx,
>>            */
>>           if (req->rq_flags & RQF_DONTPREP)
>>               scsi_mq_uninit_cmd(cmd);
>> +        cmd->flags = 0;
>>           scsi_run_queue_async(sdev);
>>           break;
>>       }
> 
> Is there agreement that all block drivers are affected that define a
> .cleanup_rq() callback? 

That is only SCSI (which is affected) and SCSI does define that (callback).

> Would this be a valid alternative (untested)?

Yes, logically this looks the same. However does scsi_cleanup_rq() need 
the unsetting of RQF_DONTPREP at ***, below. I suppose that it is not 
doing any harm if the req is going to end.

static void scsi_cleanup_rq(struct request *rq)
{
	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);

	cmd->flags = 0;

	if (rq->rq_flags & RQF_DONTPREP) {
		scsi_mq_uninit_cmd(cmd);
		rq->rq_flags &= ~RQF_DONTPREP; ***
	}
}

> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index a26a11c73ee3..0be933e26d67 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2126,6 +2126,12 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx 
> *hctx, struct list_head *list,
>               blk_mq_handle_dev_resource(rq, list);
>               goto out;
>           default:
> +            /*
> +             * Make sure to release all allocated resources when
> +             * we hit an error, as we will never see this command
> +             * again.
> +             */
> +            blk_mq_cleanup_rq(rq);
>               blk_mq_end_request(rq, ret);
>           }
>       } while (!list_empty(list));
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 48aab0df30b7..636854bffd1e 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1960,13 +1960,6 @@ static blk_status_t scsi_queue_rq(struct 
> blk_mq_hw_ctx *hctx,
>               cmd->result = DID_NO_CONNECT << 16;
>           else
>               cmd->result = DID_ERROR << 16;
> -        /*
> -         * Make sure to release all allocated resources when
> -         * we hit an error, as we will never see this command
> -         * again.
> -         */
> -        if (req->rq_flags & RQF_DONTPREP)
> -            scsi_mq_uninit_cmd(cmd);
>           scsi_run_queue_async(sdev);
>           break;
>       }
> 

cheers,
John

Re: [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error

[Bart Van Assche]

On 9/9/26 9:49 AM, John Garry wrote:
> Yes, logically this looks the same. However does scsi_cleanup_rq() need 
> the unsetting of RQF_DONTPREP at ***, below. I suppose that it is not 
> doing any harm if the req is going to end.

It's not necessary in the scsi_queue_rq() code path that we are
discussing but it improves consistency because clearing RQF_DONTPREP
reflects that the scsi_mq_uninit_cmd() call undid the changes made
by the code that sets RQF_DONTPREP.

Thanks,

Bart.