[PATCH 0/2] Add scsi_done_direct() to complete request directly.

[PATCH 0/2] Add scsi_done_direct() to complete request directly.

[Sebastian Andrzej Siewior]

This mini series adds scsi_done_direct() in order to complete scsi
requests directly via blk_mq_complete_request_direct(). This used by the
usb-storage driver.

Sebastian

[PATCH 1/2] scsi: Add scsi_done_direct() for immediate completion.

[Sebastian Andrzej Siewior]

Add scsi_done_direct() which behaves like scsi_done() except that it
invokes blk_mq_complete_request_direct() in order to complete the
request.
Callers from process context can complete the request directly instead
waking ksoftirqd.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 drivers/scsi/scsi_lib.c  | 27 +++++++++++++++++++++++----
 include/scsi/scsi_cmnd.h |  1 +
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0a70aa763a961..778ef6d09616f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1625,26 +1625,45 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
 
-void scsi_done(struct scsi_cmnd *cmd)
+static bool scsi_done_need_blk_compl(struct scsi_cmnd *cmd)
 {
 	switch (cmd->submitter) {
 	case SUBMITTED_BY_BLOCK_LAYER:
 		break;
 	case SUBMITTED_BY_SCSI_ERROR_HANDLER:
-		return scsi_eh_done(cmd);
+		scsi_eh_done(cmd);
+		return false;
 	case SUBMITTED_BY_SCSI_RESET_IOCTL:
-		return;
+		return false;
 	}
 
 	if (unlikely(blk_should_fake_timeout(scsi_cmd_to_rq(cmd)->q)))
-		return;
+		return false;
 	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))
+		return false;
+	return true;
+}
+
+void scsi_done(struct scsi_cmnd *cmd)
+{
+	if (!scsi_done_need_blk_compl(cmd))
 		return;
+
 	trace_scsi_dispatch_cmd_done(cmd);
 	blk_mq_complete_request(scsi_cmd_to_rq(cmd));
 }
 EXPORT_SYMBOL(scsi_done);
 
+void scsi_done_direct(struct scsi_cmnd *cmd)
+{
+	if (!scsi_done_need_blk_compl(cmd))
+		return;
+
+	trace_scsi_dispatch_cmd_done(cmd);
+	blk_mq_complete_request_direct(scsi_cmd_to_rq(cmd), scsi_complete);
+}
+EXPORT_SYMBOL(scsi_done_direct);
+
 static void scsi_mq_put_budget(struct request_queue *q, int budget_token)
 {
 	struct scsi_device *sdev = q->queuedata;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 6794d7322cbde..ff1c4b51f7aef 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -168,6 +168,7 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 }
 
 void scsi_done(struct scsi_cmnd *cmd);
+void scsi_done_direct(struct scsi_cmnd *cmd);
 
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
-- 
2.34.1

Re: [PATCH 1/2] scsi: Add scsi_done_direct() for immediate completion.

[Bart Van Assche]

On 2/1/22 13:09, Sebastian Andrzej Siewior wrote:
> -void scsi_done(struct scsi_cmnd *cmd)
> +static bool scsi_done_need_blk_compl(struct scsi_cmnd *cmd)

I'm not happy about the name of this function. The word "need" in the 
function name suggests that this function does not modify any state. 
However, the body of the function shows that it may complete a SCSI 
command. How about renaming the existing scsi_done() function into 
scsi_done_internal() or so and adding a "bool complete_directly" 
argument to that function?

BTW, I only received patch 1/2 but not patch 2/2. Please Cc the 
linux-scsi mailing list for the entire patch series when reposting the 
patch series.

Thanks,

Bart.

Re: [PATCH 1/2] scsi: Add scsi_done_direct() for immediate completion.

[Sebastian Andrzej Siewior]

On 2022-02-02 12:49:16 [-0800], Bart Van Assche wrote:
> On 2/1/22 13:09, Sebastian Andrzej Siewior wrote:
> > -void scsi_done(struct scsi_cmnd *cmd)
> > +static bool scsi_done_need_blk_compl(struct scsi_cmnd *cmd)
> 
> I'm not happy about the name of this function. The word "need" in the
> function name suggests that this function does not modify any state.
> However, the body of the function shows that it may complete a SCSI command.
> How about renaming the existing scsi_done() function into
> scsi_done_internal() or so and adding a "bool complete_directly" argument to
> that function?

Let me see what I can do.

> BTW, I only received patch 1/2 but not patch 2/2. Please Cc the linux-scsi
> mailing list for the entire patch series when reposting the patch series.

I did and based on lore's archive it made it to the list:
	https://lore.kernel.org/linux-scsi/20220201210954.570896-1-sebastian@breakpoint.cc/

> Thanks,
> 
> Bart.

Sebastian

[PATCH v2 1/2] scsi: Add scsi_done_direct() for immediate completion.

[Sebastian Andrzej Siewior]

Add scsi_done_direct() which behaves like scsi_done() except that it
invokes blk_mq_complete_request_direct() in order to complete the
request.
Callers from process context can complete the request directly instead
waking ksoftirqd.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
On 2022-02-03 20:46:44 [+0100], To Bart Van Assche wrote:
> 
> Let me see what I can do.

Something like this perhaps? The compiler not inline
scsi_done_internal() so we maybe provide scsi_done() / _direct() as
static inlines?

 drivers/scsi/scsi_lib.c  | 21 +++++++++++++++++++--
 include/scsi/scsi_cmnd.h |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0a70aa763a961..a1c18ba5e8d38 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1625,8 +1625,10 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
 
-void scsi_done(struct scsi_cmnd *cmd)
+static void scsi_done_internal(struct scsi_cmnd *cmd, bool complete_directly)
 {
+	struct request *req = scsi_cmd_to_rq(cmd);
+
 	switch (cmd->submitter) {
 	case SUBMITTED_BY_BLOCK_LAYER:
 		break;
@@ -1641,10 +1643,25 @@ void scsi_done(struct scsi_cmnd *cmd)
 	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))
 		return;
 	trace_scsi_dispatch_cmd_done(cmd);
-	blk_mq_complete_request(scsi_cmd_to_rq(cmd));
+
+	if (complete_directly)
+		blk_mq_complete_request_direct(req, scsi_complete);
+	else
+		blk_mq_complete_request(req);
+}
+
+void scsi_done(struct scsi_cmnd *cmd)
+{
+	scsi_done_internal(cmd, false);
 }
 EXPORT_SYMBOL(scsi_done);
 
+void scsi_done_direct(struct scsi_cmnd *cmd)
+{
+	scsi_done_internal(cmd, true);
+}
+EXPORT_SYMBOL(scsi_done_direct);
+
 static void scsi_mq_put_budget(struct request_queue *q, int budget_token)
 {
 	struct scsi_device *sdev = q->queuedata;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 6794d7322cbde..ff1c4b51f7aef 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -168,6 +168,7 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 }
 
 void scsi_done(struct scsi_cmnd *cmd);
+void scsi_done_direct(struct scsi_cmnd *cmd);
 
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
-- 
2.34.1

Re: [PATCH v2 1/2] scsi: Add scsi_done_direct() for immediate completion.

[Bart Van Assche]

On 2/3/22 12:29, Sebastian Andrzej Siewior wrote:
> Let me see what I can do.
> 
> Something like this perhaps? The compiler not inline
> scsi_done_internal() so we maybe provide scsi_done() / _direct() as
> static inlines?

In general declaring a static function inline in a .c file is frowned 
upon but I think in this case we should do that. With that change 
applied, feel free to add:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/2] scsi: Add scsi_done_direct() for immediate completion.

[Bart Van Assche]

On 2/3/22 11:46, Sebastian Andrzej Siewior wrote:
> On 2022-02-02 12:49:16 [-0800], Bart Van Assche wrote:
>> On 2/1/22 13:09, Sebastian Andrzej Siewior wrote:
>>> -void scsi_done(struct scsi_cmnd *cmd)
>>> +static bool scsi_done_need_blk_compl(struct scsi_cmnd *cmd)
>>
>> I'm not happy about the name of this function. The word "need" in the
>> function name suggests that this function does not modify any state.
>> However, the body of the function shows that it may complete a SCSI command.
>> How about renaming the existing scsi_done() function into
>> scsi_done_internal() or so and adding a "bool complete_directly" argument to
>> that function?
> 
> Let me see what I can do.
> 
>> BTW, I only received patch 1/2 but not patch 2/2. Please Cc the linux-scsi
>> mailing list for the entire patch series when reposting the patch series.
> 
> I did and based on lore's archive it made it to the list:
> 	https://lore.kernel.org/linux-scsi/20220201210954.570896-1-sebastian@breakpoint.cc/

I agree that patch 2/2 seems to have made it to the linux-scsi list. 
However, I can't find that patch in my mailbox nor in my spam folder. I 
think this is the first time that I did not receive a patch sent to the 
linux-scsi mailing list. Weird ...

Bart.

[PATCH 2/2] usb: storage: Complete the scsi request directly.

[Sebastian Andrzej Siewior]

The storage driver completes its requests directly from a kernel thread.

Use scsi_done_direct() to avoid waking ksoftirqd.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
 drivers/usb/storage/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 8b543f2c98575..ed7c6ad96a743 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -417,7 +417,7 @@ static int usb_stor_control_thread(void * __us)
 		if (srb) {
 			usb_stor_dbg(us, "scsi cmd done, result=0x%x\n",
 					srb->result);
-			scsi_done(srb);
+			scsi_done_direct(srb);
 		}
 	} /* for (;;) */
 
-- 
2.34.1

Re: [PATCH 0/2] Add scsi_done_direct() to complete request directly.

[Martin K. Petersen]

Sebastian,

> This mini series adds scsi_done_direct() in order to complete scsi
> requests directly via blk_mq_complete_request_direct(). This used by
> the usb-storage driver.

Applied to 5.18/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] Add scsi_done_direct() to complete request directly.

[Martin K. Petersen]

On Tue, 1 Feb 2022 22:09:52 +0100, Sebastian Andrzej Siewior wrote:

> This mini series adds scsi_done_direct() in order to complete scsi
> requests directly via blk_mq_complete_request_direct(). This used by the
> usb-storage driver.
> 
> Sebastian
> 

Applied to 5.18/scsi-queue, thanks!

[1/2] scsi: Add scsi_done_direct() for immediate completion.
      https://git.kernel.org/mkp/scsi/c/b84b6ec0f976
[2/2] usb: storage: Complete the scsi request directly.
      https://git.kernel.org/mkp/scsi/c/23fe075519c6

-- 
Martin K. Petersen	Oracle Linux Engineering