* [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error @ 2026-09-08 12:08 John Garry 2026-09-09 16:32 ` Bart Van Assche 0 siblings, 1 reply; 4+ messages in thread From: John Garry @ 2026-09-08 12:08 UTC (permalink / raw) To: James.Bottomley, mkp; +Cc: linux-scsi, bvanassche, hare, 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error 2026-09-08 12:08 [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error John Garry @ 2026-09-09 16:32 ` Bart Van Assche 2026-09-09 16:49 ` John Garry 0 siblings, 1 reply; 4+ messages in thread From: Bart Van Assche @ 2026-09-09 16:32 UTC (permalink / raw) To: John Garry, James.Bottomley, mkp; +Cc: linux-scsi, hare 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. ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error 2026-09-09 16:32 ` Bart Van Assche @ 2026-09-09 16:49 ` John Garry 2026-09-09 17:55 ` Bart Van Assche 0 siblings, 1 reply; 4+ messages in thread From: John Garry @ 2026-09-09 16:49 UTC (permalink / raw) To: Bart Van Assche, James.Bottomley, mkp; +Cc: linux-scsi, hare 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error 2026-09-09 16:49 ` John Garry @ 2026-09-09 17:55 ` Bart Van Assche 0 siblings, 0 replies; 4+ messages in thread From: Bart Van Assche @ 2026-09-09 17:55 UTC (permalink / raw) To: John Garry, James.Bottomley, mkp; +Cc: linux-scsi, hare 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-09 17:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-08 12:08 [PATCH RFC] scsi: core: Clear cmd->flags for terminal queuing error John Garry 2026-09-09 16:32 ` Bart Van Assche 2026-09-09 16:49 ` John Garry 2026-09-09 17:55 ` Bart Van Assche
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox