[RFC PATCH 0/3] blk-mq: Allow to complete requests directly

[RFC PATCH 0/3] blk-mq: Allow to complete requests directly

[Sebastian Andrzej Siewior]

This is a follow up to
  https://lkml.kernel.org/r/20201102181238.GA17806@infradead.org

where I *think* we agreed to audit driver which complete their blk
request from process context and then let them complete the request
directly instead going through ksoftirqd.

This series converts a part from the MMC layer which completes the
requests from kworker/ preemptible context. It was verified with
sdhci-pci device under normal usage. It also converts the usb-storage
driver which is slightly complicated since it goes through the SCSI
layer.

Sebastian

[RFC PATCH 1/3] blk-mq: Add blk_mq_complete_request_direct()

[Sebastian Andrzej Siewior]

Add blk_mq_complete_request_direct() which completes the block request
directly instead deferring it to softirq for single queue devices.

This is useful for devices which complete the requests in preemptible
context and raising softirq from means scheduling ksoftirqd.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 block/blk-mq.c         | 6 ++++++
 include/linux/blk-mq.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 108a352051be5..44582aef3c32c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -667,6 +667,12 @@ bool blk_mq_complete_request_remote(struct request *rq)
 }
 EXPORT_SYMBOL_GPL(blk_mq_complete_request_remote);
 
+void blk_mq_complete_request_direct(struct request *rq)
+{
+	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
+	rq->q->mq_ops->complete(rq);
+}
+
 /**
  * blk_mq_complete_request - end I/O on a request
  * @rq:		the request being processed
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 13ba1861e688f..df9ea4c5d91c9 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -528,6 +528,7 @@ void __blk_mq_end_request(struct request *rq, blk_status_t error);
 void blk_mq_requeue_request(struct request *rq, bool kick_requeue_list);
 void blk_mq_kick_requeue_list(struct request_queue *q);
 void blk_mq_delay_kick_requeue_list(struct request_queue *q, unsigned long msecs);
+void blk_mq_complete_request_direct(struct request *rq);
 void blk_mq_complete_request(struct request *rq);
 bool blk_mq_complete_request_remote(struct request *rq);
 bool blk_mq_queue_stopped(struct request_queue *q);
-- 
2.33.0

Re: [RFC PATCH 1/3] blk-mq: Add blk_mq_complete_request_direct()

[Christoph Hellwig]

On Fri, Oct 15, 2021 at 05:14:10PM +0200, Sebastian Andrzej Siewior wrote:
> +void blk_mq_complete_request_direct(struct request *rq)
> +{
> +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
> +	rq->q->mq_ops->complete(rq);
> +}

As this is called by the driver we known what ->complete this helper
would call.  So instead of doing this we could just call
blk_mq_set_request_complete and the actual completion helper.
The comment above it will need some updates of course.

Re: [RFC PATCH 1/3] blk-mq: Add blk_mq_complete_request_direct()

[Sebastian Andrzej Siewior]

On 2021-10-15 09:03:28 [-0700], Christoph Hellwig wrote:
> On Fri, Oct 15, 2021 at 05:14:10PM +0200, Sebastian Andrzej Siewior wrote:
> > +void blk_mq_complete_request_direct(struct request *rq)
> > +{
> > +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
> > +	rq->q->mq_ops->complete(rq);
> > +}
> 
> As this is called by the driver we known what ->complete this helper
> would call.  So instead of doing this we could just call
> blk_mq_set_request_complete and the actual completion helper.
> The comment above it will need some updates of course.

So
	blk_mq_set_request_complete();
	mmc_blk_mq_complete();

for the mmc driver and no fiddling in the blk-mq then.

Sebastian

Re: [RFC PATCH 1/3] blk-mq: Add blk_mq_complete_request_direct()

[Jens Axboe]

On 10/15/21 10:15 AM, Sebastian Andrzej Siewior wrote:
> On 2021-10-15 09:03:28 [-0700], Christoph Hellwig wrote:
>> On Fri, Oct 15, 2021 at 05:14:10PM +0200, Sebastian Andrzej Siewior wrote:
>>> +void blk_mq_complete_request_direct(struct request *rq)
>>> +{
>>> +	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
>>> +	rq->q->mq_ops->complete(rq);
>>> +}
>>
>> As this is called by the driver we known what ->complete this helper
>> would call.  So instead of doing this we could just call
>> blk_mq_set_request_complete and the actual completion helper.
>> The comment above it will need some updates of course.
> 
> So
> 	blk_mq_set_request_complete();
> 	mmc_blk_mq_complete();
> 
> for the mmc driver and no fiddling in the blk-mq then.

Just pass in the handler:

void blk_mq_complete_request_direct(struct request *rq,
				    void (*complete)(struct request *rq))
{
	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
	complete(rq);
}

And we should probably put it in blk-mq.h so it inlines nicely, and
that'll be faster than the indirect call.

-- 
Jens Axboe

[RFC PATCH 2/3] mmc: core: Use blk_mq_complete_request_direct().

[Sebastian Andrzej Siewior]

The completion callback for the sdhci-pci device is invoked from a
kworker.
I couldn't identify in which context is mmc_blk_mq_req_done() invoke but
the remaining caller are from invoked from preemptible context. Here it
would make sense to complete the request directly instead scheduling
ksoftirqd for its completion.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mmc/core/block.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 431af5e8be2f8..16b4ea0e92c4b 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2051,7 +2051,8 @@ static void mmc_blk_mq_dec_in_flight(struct mmc_queue *mq, struct request *req)
 		mmc_put_card(mq->card, &mq->ctx);
 }
 
-static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
+static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req,
+				bool can_sleep)
 {
 	struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
 	struct mmc_request *mrq = &mqrq->brq.mrq;
@@ -2065,8 +2066,12 @@ static void mmc_blk_mq_post_req(struct mmc_queue *mq, struct request *req)
 	 */
 	if (mq->in_recovery)
 		mmc_blk_mq_complete_rq(mq, req);
-	else if (likely(!blk_should_fake_timeout(req->q)))
-		blk_mq_complete_request(req);
+	else if (likely(!blk_should_fake_timeout(req->q))) {
+		if (can_sleep)
+			blk_mq_complete_request_direct(req);
+		else
+			blk_mq_complete_request(req);
+	}
 
 	mmc_blk_mq_dec_in_flight(mq, req);
 }
@@ -2087,7 +2092,7 @@ void mmc_blk_mq_recovery(struct mmc_queue *mq)
 
 	mmc_blk_urgent_bkops(mq, mqrq);
 
-	mmc_blk_mq_post_req(mq, req);
+	mmc_blk_mq_post_req(mq, req, true);
 }
 
 static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
@@ -2106,7 +2111,7 @@ static void mmc_blk_mq_complete_prev_req(struct mmc_queue *mq,
 	if (prev_req)
 		*prev_req = mq->complete_req;
 	else
-		mmc_blk_mq_post_req(mq, mq->complete_req);
+		mmc_blk_mq_post_req(mq, mq->complete_req, true);
 
 	mq->complete_req = NULL;
 
@@ -2178,7 +2183,8 @@ static void mmc_blk_mq_req_done(struct mmc_request *mrq)
 	mq->rw_wait = false;
 	wake_up(&mq->wait);
 
-	mmc_blk_mq_post_req(mq, req);
+	/* context unknown */
+	mmc_blk_mq_post_req(mq, req, false);
 }
 
 static bool mmc_blk_rw_wait_cond(struct mmc_queue *mq, int *err)
@@ -2238,7 +2244,7 @@ static int mmc_blk_mq_issue_rw_rq(struct mmc_queue *mq,
 	err = mmc_start_request(host, &mqrq->brq.mrq);
 
 	if (prev_req)
-		mmc_blk_mq_post_req(mq, prev_req);
+		mmc_blk_mq_post_req(mq, prev_req, true);
 
 	if (err)
 		mq->rw_wait = false;
-- 
2.33.0

[RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Sebastian Andrzej Siewior]

The usb-storage driver runs in a thread and completes its request from
that thread. Since it is a single queued device it always schedules
ksoftirqd for its completion.

The completion is performed in the SCSI stack. Add
scsi_done_preemptible() which inlines most of scsi_mq_done() and
completes the request directly via blk_mq_complete_request_direct().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/scsi/scsi_lib.c   | 17 ++++++++++++++++-
 drivers/usb/storage/usb.c |  2 +-
 include/scsi/scsi_cmnd.h  |  7 +++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 572673873ddf8..f0eeedce6b081 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1575,16 +1575,31 @@ static blk_status_t scsi_prepare_cmd(struct request *req)
 	return scsi_cmd_to_driver(cmd)->init_command(cmd);
 }
 
-static void scsi_mq_done(struct scsi_cmnd *cmd)
+static void _scsi_mq_done(struct scsi_cmnd *cmd)
 {
 	if (unlikely(blk_should_fake_timeout(scsi_cmd_to_rq(cmd)->q)))
 		return;
 	if (unlikely(test_and_set_bit(SCMD_STATE_COMPLETE, &cmd->state)))
 		return;
 	trace_scsi_dispatch_cmd_done(cmd);
+}
+
+static void scsi_mq_done(struct scsi_cmnd *cmd)
+{
+	_scsi_mq_done(cmd);
 	blk_mq_complete_request(scsi_cmd_to_rq(cmd));
 }
 
+void scsi_done_preemptible(struct scsi_cmnd *scmd)
+{
+	if (scmd->scsi_done != scsi_mq_done) {
+		scmd->scsi_done(scmd);
+		return;
+	}
+	_scsi_mq_done(scmd);
+	blk_mq_complete_request_direct(scsi_cmd_to_rq(scmd));
+}
+
 static void scsi_mq_put_budget(struct request_queue *q, int budget_token)
 {
 	struct scsi_device *sdev = q->queuedata;
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 90aa9c12ffac5..6ceedd1e14ce7 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);
-			srb->scsi_done(srb);
+			scsi_done_preemptible(srb);
 		}
 	} /* for (;;) */
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index eaf04c9a1dfcb..e992f2f74dd69 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -396,4 +396,11 @@ static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd)
 extern void scsi_build_sense(struct scsi_cmnd *scmd, int desc,
 			     u8 key, u8 asc, u8 ascq);
 
+static inline void scsi_done(struct scsi_cmnd *scmd)
+{
+	scmd->scsi_done(scmd);
+}
+
+extern void scsi_done_preemptible(struct scsi_cmnd *scmd);
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
2.33.0

Re: [RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Christoph Hellwig]

Bart has been working on removing the ->scsi_done indirection, so this
will need to find a way to interact with that

Re: [RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Sebastian Andrzej Siewior]

On 2021-10-15 09:04:47 [-0700], Christoph Hellwig wrote:
> Bart has been working on removing the ->scsi_done indirection, so this
> will need to find a way to interact with that

Okay. So I just wait until it is there. Is this v5.15/16 material?

Sebastian

Re: [RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Bart Van Assche]

On 10/15/21 09:16, Sebastian Andrzej Siewior wrote:
> On 2021-10-15 09:04:47 [-0700], Christoph Hellwig wrote:
>> Bart has been working on removing the ->scsi_done indirection, so this
>> will need to find a way to interact with that
> 
> Okay. So I just wait until it is there. Is this v5.15/16 material?

Isn't it too late to submit patches for v5.15 other than bugfixes for 
patches merged during the v5.15 merge window?

Martin Petersen, the SCSI maintainer, has been so kind to queue the 
patch series that removes the scsi_done member for the v5.16 merge 
window. So that patch series should become available soon in the 
following git repository:
git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git

Thanks,

Bart.

Re: [RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Sebastian Andrzej Siewior]

On 2021-10-16 19:17:05 [-0700], Bart Van Assche wrote:
> On 10/15/21 09:16, Sebastian Andrzej Siewior wrote:
> > On 2021-10-15 09:04:47 [-0700], Christoph Hellwig wrote:
> > > Bart has been working on removing the ->scsi_done indirection, so this
> > > will need to find a way to interact with that
> > 
> > Okay. So I just wait until it is there. Is this v5.15/16 material?
> 
> Isn't it too late to submit patches for v5.15 other than bugfixes for
> patches merged during the v5.15 merge window?

yeah, off by one, meant 16/17 ;)

> Martin Petersen, the SCSI maintainer, has been so kind to queue the patch
> series that removes the scsi_done member for the v5.16 merge window. So that
> patch series should become available soon in the following git repository:
> git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git

Thanks.

> Thanks,
> 
> Bart.

Sebastian

Re: [RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Bart Van Assche]

On 10/15/21 08:14, Sebastian Andrzej Siewior wrote:
> +static inline void scsi_done(struct scsi_cmnd *scmd)
> +{
> +	scmd->scsi_done(scmd);
> +}

How about leaving out this function definition and open-coding it into 
its callers?

Additionally, please rebase this patch series on top of "[PATCH v3 
00/88] Call scsi_done() directly" 
(https://lore.kernel.org/linux-scsi/20211007202923.2174984-1-bvanassche@acm.org/ 
or https://github.com/bvanassche/linux/tree/scsi-remove-done-callback). 
Otherwise Linus will have to resolve a very complicated merge conflict.

Thank you,

Bart.

Re: [RFC PATCH 3/3] scsi, usb: storage: Complete the blk-request directly.

[Sebastian Andrzej Siewior]

On 2021-10-16 14:21:37 [-0700], Bart Van Assche wrote:
> On 10/15/21 08:14, Sebastian Andrzej Siewior wrote:
> > +static inline void scsi_done(struct scsi_cmnd *scmd)
> > +{
> > +	scmd->scsi_done(scmd);
> > +}
> 
> How about leaving out this function definition and open-coding it into its
> callers?

Let me reevaluate the situation with your series. I saw that you
provided a scsi_done() function.

> Thank you,
> 
> Bart.

Sebastian