Skip to contentSkip to conversation

blk-mq: cleanup terminal request when it could not be queued

PATCH In review John Garry 2 participants 3 messages Sep 11 – Sep 14

Subsystems (2)
SCSI Block layer
lore
Share & tools
lore

John Garry

author
#0 lore raw

PATCH 1/1 blk-mq: cleanup terminal request when it could not be queued

Sashiko points out that the SCSI midlayer error handling misses some cleanup for when a request could not be queued and won't be requeued [0]. Specifically, clearing the SCSI cmd flags member is missing. Normally that would be cleared in the non-error path in scsi_end_request() (which also calls __blk_mq_end_request()).

Callchain blk_mq_cleanup_rq() -> scsi_cleanup_rq() does all the necessary cleanup, so call that in blk_mq_dispatch_rq_list() for when there was an error in the dispatch and the rq will be ended.

Note that dm-rq code calls blk_mq_cleanup_rq() for a cloned rq (or SCSI rq) for when the clone could not be queued and will be retried, but that path does not use the sched code (and blk_mq_dispatch_rq_list()).

[0] https://lore.kernel.org/linux-scsi/[email protected]/

Suggesested-by Bart Van Assche <[email protected]>
Signed-off-by John Garry <[email protected]>

---
Maybe SCSI should be doing this cleanup directly, as we now create some confusion on who should do the cleanup - blk-mq or the driver.

block/blk-mq.c +1 -0
index a26a11c73ee3..eb3a481211d0 100644
@@ -2126,6 +2126,7 @@ 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:
+			blk_mq_cleanup_rq(rq);
 			blk_mq_end_request(rq, ret);
 		}
 	} while (!list_empty(list));
drivers/scsi/scsi_lib.c +1 -7
index af27fd3df8d4..1ea65b5eb25c 100644
@@ -1960,13 +1960,7 @@ 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);
+		/* blk-mq will cleanup our resources as the rq will end */
 		scsi_run_queue_async(sdev);
 		break;
 	}
Signature · 1 line
2.43.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 10 lines · John Garry

Sashiko points out that the SCSI midlayer error handling misses some cleanup for when a request could not be queued and won't be requeued [0]. Specifically, clearing the SCSI cmd flags member is missing. Normally that would be cleared in the non-error path in scsi_end_request() (which also calls __blk_mq_end_request()).

Callchain blk_mq_cleanup_rq() -> scsi_cleanup_rq() does all the necessary cleanup, so call that in blk_mq_dispatch_rq_list() for when there was an error in the dispatch and the rq will be ended.

Quoted 8 lines

Note that dm-rq code calls blk_mq_cleanup_rq() for a cloned rq (or SCSI rq) for when the clone could not be queued and will be retried, but that path does not use the sched code (and blk_mq_dispatch_rq_list()).

[0] https://lore.kernel.org/linux-scsi/[email protected]/

Suggesested-by Bart Van Assche <[email protected]>
Signed-off-by John Garry <[email protected]>

Does this patch need a Fixes: tag? Anyway:

Reviewed-by Bart Van Assche <[email protected]>
author
#2 lore raw
Quoted 15 lines · Bart Van Assche
Quoted 14 lines · John Garry

Sashiko points out that the SCSI midlayer error handling misses some cleanup for when a request could not be queued and won't be requeued [0]. Specifically, clearing the SCSI cmd flags member is missing. Normally that would be cleared in the non-error path in scsi_end_request() (which also calls __blk_mq_end_request()).

Callchain blk_mq_cleanup_rq() -> scsi_cleanup_rq() does all the necessary cleanup, so call that in blk_mq_dispatch_rq_list() for when there was an error in the dispatch and the rq will be ended.

Note that dm-rq code calls blk_mq_cleanup_rq() for a cloned rq (or SCSI rq) for when the clone could not be queued and will be retried, but that path does not use the sched code (and blk_mq_dispatch_rq_list()).

Quoted 5 lines
Quoted 4 lines

[0] https://lore.kernel.org/linux- scsi/[email protected]/

Suggesested-by Bart Van Assche <[email protected]>
Signed-off-by John Garry <[email protected]>

Does this patch need a Fixes: tag? Anyway:

I can imagine that whatever this fixes pre-dates blk_mq_cleanup_rq(), so backporting is a concern. However, the oldest LTS today is 5.10, and blk_mq_cleanup_rq() is present there.

Anyway, I would like to get more opinion on whether this is the best approach.

Quoted 2 lines
Reviewed-by Bart Van Assche <[email protected]>

thanks