Add MMC packed request support

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

block/blk-mq-sched.c +25 -11

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);
                        else
-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
                }
        } else if (has_sched_dispatch) {
-               blk_mq_do_dispatch_sched(hctx);
+               dispatch_ret = blk_mq_do_dispatch_sched(hctx);
        } else if (hctx->dispatch_busy) {
                /* dequeue request one by one from sw queue if queue is busy */
-               blk_mq_do_dispatch_ctx(hctx);
+               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
        } else {
                blk_mq_flush_busy_ctxs(hctx, &rq_list);
-               blk_mq_dispatch_rq_list(q, &rq_list, false);
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+       }
+
+       if (dispatch_ret) {
+               if (hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)
+                       hctx->queue->mq_ops->commit_rqs(hctx);
        }
 }
 

block/blk-mq.c +9 -5

index 87c6699f35ae..9b46f5d6c7fd 100644
@@ -1238,11 +1238,15 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
                 * Flag last if we have no more requests, or if we have more
                 * but can't assign a driver tag to it.
                 */
-               if (list_empty(list))
-                       bd.last = true;
-               else {
-                       nxt = list_first_entry(list, struct request, queuelist);
-                       bd.last = !blk_mq_get_driver_tag(nxt);
+               if (!(hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)) {
+                       if (list_empty(list))
+                               bd.last = true;
+                       else {
+                               nxt = list_first_entry(list, struct request, queuelist);
+                               bd.last = !blk_mq_get_driver_tag(nxt);
+                       }
+               } else {
+                       bd.last = false;
                }
 
                ret = q->mq_ops->queue_rq(hctx, &bd);

include/linux/blk-mq.h +1 -0

index 07fa767bff86..c0ef6990b698 100644
@@ -394,6 +394,7 @@ enum {
        BLK_MQ_F_SHOULD_MERGE   = 1 << 0,
        BLK_MQ_F_TAG_SHARED     = 1 << 1,
        BLK_MQ_F_NO_MANAGED_IRQ = 1 << 2,
+       BLK_MQ_F_FORCE_COMMIT_RQS = 1 << 3,
        BLK_MQ_F_BLOCKING       = 1 << 5,
        BLK_MQ_F_NO_SCHED       = 1 << 6,
        BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,

Thanks,
Ming

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

Quoted 66 lines

block/blk-mq-sched.c +13 -5

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 12 lines

block/blk-mq-sched.c +3 -2

        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Quoted 37 lines

block/blk-mq-sched.c +9 -4

                        else
-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
                }
        } else if (has_sched_dispatch) {
-               blk_mq_do_dispatch_sched(hctx);
+               dispatch_ret = blk_mq_do_dispatch_sched(hctx);
        } else if (hctx->dispatch_busy) {
                /* dequeue request one by one from sw queue if queue is busy */
-               blk_mq_do_dispatch_ctx(hctx);
+               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
        } else {
                blk_mq_flush_busy_ctxs(hctx, &rq_list);
-               blk_mq_dispatch_rq_list(q, &rq_list, false);
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+       }
+
+       if (dispatch_ret) {
+               if (hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)
+                       hctx->queue->mq_ops->commit_rqs(hctx);
        }
 }
 

block/blk-mq.c +1 -5

index 87c6699f35ae..9b46f5d6c7fd 100644
@@ -1238,11 +1238,15 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
                 * Flag last if we have no more requests, or if we have more
                 * but can't assign a driver tag to it.
                 */
-               if (list_empty(list))
-                       bd.last = true;
-               else {
-                       nxt = list_first_entry(list, struct request, queuelist);
-                       bd.last = !blk_mq_get_driver_tag(nxt);
+               if (!(hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)) {

block/blk-mq.c bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list, · line 1249

Quoted 8 lines

block/blk-mq.c +8 -0

+                       if (list_empty(list))
+                               bd.last = true;
+                       else {
+                               nxt = list_first_entry(list, struct request, queuelist);
+                               bd.last = !blk_mq_get_driver_tag(nxt);
+                       }
+               } else {
+                       bd.last = false;

Quoted 16 lines

block/blk-mq.c +0 -0

                }
 
                ret = q->mq_ops->queue_rq(hctx, &bd);

include/linux/blk-mq.h +1 -0

index 07fa767bff86..c0ef6990b698 100644
@@ -394,6 +394,7 @@ enum {
        BLK_MQ_F_SHOULD_MERGE   = 1 << 0,
        BLK_MQ_F_TAG_SHARED     = 1 << 1,
        BLK_MQ_F_NO_MANAGED_IRQ = 1 << 2,
+       BLK_MQ_F_FORCE_COMMIT_RQS = 1 << 3,
        BLK_MQ_F_BLOCKING       = 1 << 5,
        BLK_MQ_F_NO_SCHED       = 1 << 6,
        BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,

Quoted 3 lines

Thanks,
Ming

--
Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

this case, we will not issue commit_rqs() to tell the hardware to handle previous request dispatched from &rq_list.

So I think we should not overlap the 'dispatch_ret'? Or do you have any other thoughts to fix?

Quoted 42 lines

Quoted 42 lines

block/blk-mq-sched.c +9 -4

                        else
-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
                }
        } else if (has_sched_dispatch) {
-               blk_mq_do_dispatch_sched(hctx);
+               dispatch_ret = blk_mq_do_dispatch_sched(hctx);
        } else if (hctx->dispatch_busy) {
                /* dequeue request one by one from sw queue if queue is busy */
-               blk_mq_do_dispatch_ctx(hctx);
+               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
        } else {
                blk_mq_flush_busy_ctxs(hctx, &rq_list);
-               blk_mq_dispatch_rq_list(q, &rq_list, false);
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+       }
+
+       if (dispatch_ret) {
+               if (hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)
+                       hctx->queue->mq_ops->commit_rqs(hctx);
        }
 }
 

block/blk-mq.c +6 -5

index 87c6699f35ae..9b46f5d6c7fd 100644
@@ -1238,11 +1238,15 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
                 * Flag last if we have no more requests, or if we have more
                 * but can't assign a driver tag to it.
                 */
-               if (list_empty(list))
-                       bd.last = true;
-               else {
-                       nxt = list_first_entry(list, struct request, queuelist);
-                       bd.last = !blk_mq_get_driver_tag(nxt);
+               if (!(hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)) {
+                       if (list_empty(list))
+                               bd.last = true;
+                       else {
+                               nxt = list_first_entry(list, struct request, queuelist);
+                               bd.last = !blk_mq_get_driver_tag(nxt);

Quoted 3 lines

block/blk-mq.c bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list, · line 1249

Quoted 3 lines

block/blk-mq.c +3 -0

+                       }
+               } else {
+                       bd.last = false;

If we enabled BLK_MQ_F_FORCE_COMMIT_RQS flag, we will always get bd.last = false even for the real last request in the IO scheduler. I know you already use commit_irqs() to help to kick driver. But I worried if it is reasonable that drivers always get bd.last = false.

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 49 lines

this case, we will not issue commit_rqs() to tell the hardware to handle previous request dispatched from &rq_list.

So I think we should not overlap the 'dispatch_ret'? Or do you have any other thoughts to fix?

Quoted 42 lines

Quoted 42 lines

block/blk-mq-sched.c +9 -4

                        else
-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
                }
        } else if (has_sched_dispatch) {
-               blk_mq_do_dispatch_sched(hctx);
+               dispatch_ret = blk_mq_do_dispatch_sched(hctx);
        } else if (hctx->dispatch_busy) {
                /* dequeue request one by one from sw queue if queue is busy */
-               blk_mq_do_dispatch_ctx(hctx);
+               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);
        } else {
                blk_mq_flush_busy_ctxs(hctx, &rq_list);
-               blk_mq_dispatch_rq_list(q, &rq_list, false);
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+       }
+
+       if (dispatch_ret) {
+               if (hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)
+                       hctx->queue->mq_ops->commit_rqs(hctx);
        }
 }
 

block/blk-mq.c +6 -5

index 87c6699f35ae..9b46f5d6c7fd 100644
@@ -1238,11 +1238,15 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
                 * Flag last if we have no more requests, or if we have more
                 * but can't assign a driver tag to it.
                 */
-               if (list_empty(list))
-                       bd.last = true;
-               else {
-                       nxt = list_first_entry(list, struct request, queuelist);
-                       bd.last = !blk_mq_get_driver_tag(nxt);
+               if (!(hctx->flags & BLK_MQ_F_FORCE_COMMIT_RQS)) {
+                       if (list_empty(list))
+                               bd.last = true;
+                       else {
+                               nxt = list_first_entry(list, struct request, queuelist);
+                               bd.last = !blk_mq_get_driver_tag(nxt);

Quoted 9 lines

Quoted 3 lines

block/blk-mq.c bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list, · line 1249

Quoted 3 lines

block/blk-mq.c +3 -0

+                       }
+               } else {
+                       bd.last = false;

If we enabled BLK_MQ_F_FORCE_COMMIT_RQS flag, we will always get bd.last = false even for the real last request in the IO scheduler. I know you already use commit_irqs() to help to kick driver. But I worried if it is reasonable that drivers always get bd.last = false.

BLK_MQ_F_FORCE_COMMIT_RQS means the .last flag is ignored, and we can document this usage.

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

       } while (ret);

       return ret;

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Quoted 223 lines · Ming Lei

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

Quoted 6 lines

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Yes, this can work.

But I found your patch will drop some performance comparing with my method in patch 1/2. My method can fetch several requests from IO scheduler and dispatch them to block driver at one time, but in your patch we still need dispatch request one by one, which will drop some performance I think.
What do you think? Thanks.

Quoted 233 lines · Baolin Wang

Quoted 223 lines · Ming Lei

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

Quoted 6 lines

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Yes, this can work.

Quoted 6 lines

But I found your patch will drop some performance comparing with my method in patch 1/2. My method can fetch several requests from IO scheduler and dispatch them to block driver at one time, but in your patch we still need dispatch request one by one, which will drop some performance I think.
What do you think? Thanks.

Please run your test and see if performance drop can be observed.

Hi Ming,

Quoted 235 lines · Ming Lei

Quoted 233 lines · Baolin Wang

Quoted 223 lines · Ming Lei

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 7 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request, that means we can package several requests to host controller at one time to improve performence. So the hardware driver expects the blk-mq can dispatch a batch of requests at one time, and driver can use bd.last to indicate if it is the last request in the batch to help to combine requests as much as possible.

Quoted 5 lines

Thus we should add batch requests setting from the block driver to tell the scheduler how many requests can be dispatched in a batch, as well as changing the scheduler to dispatch more than one request if setting the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

Quoted 6 lines

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Yes, this can work.

Quoted 8 lines

Quoted 6 lines

But I found your patch will drop some performance comparing with my method in patch 1/2. My method can fetch several requests from IO scheduler and dispatch them to block driver at one time, but in your patch we still need dispatch request one by one, which will drop some performance I think.
What do you think? Thanks.

Please run your test and see if performance drop can be observed.

From my testing (using the same fio configuration in cover letter), I found your method will drop some performance from below data.

My original patches:
Sequential read: 229.6MiB/s
Random read:180.8MiB/s
Sequential write: 172MiB/s
Random write:169.2MiB/s

Your patches:
Sequential read: 209MiB/s
Random read:177MiB/s
Sequential write: 148MiB/s
Random write:147MiB/s

Hi Ming,

Quoted 257 lines · Baolin Wang

Hi Ming,

Quoted 235 lines · Ming Lei

Quoted 233 lines · Baolin Wang

Quoted 223 lines · Ming Lei

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

Quoted 12 lines · Baolin Wang

As we know, some SD/MMC host controllers can support packed request,
that means we can package several requests to host controller at one
time to improve performence. So the hardware driver expects the blk-mq
can dispatch a batch of requests at one time, and driver can use bd.last
to indicate if it is the last request in the batch to help to combine
requests as much as possible.

Thus we should add batch requests setting from the block driver to tell
the scheduler how many requests can be dispatched in a batch, as well
as changing the scheduler to dispatch more than one request if setting
the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in .queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage? And you can do whatever batch processing in .commit_rqs() which will be guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

block/blk-mq-sched.c +15 -6

index 856356b1619e..cd2bbe56f83f 100644
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
                 * in blk_mq_dispatch_rq_list().
                 */
                list_add(&rq->queuelist, &rq_list);
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
+
+       return ret;
 }
 
 static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,

@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
  * its queue by itself in its completion handler, so we don't need to
  * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
  */
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 {
        struct request_queue *q = hctx->queue;
        LIST_HEAD(rq_list);
        struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+       bool ret = false;
 
        do {
                struct request *rq;

@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
 
                /* round robin for fair dispatch */
                ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
-       } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+               ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+       } while (ret);
 
        WRITE_ONCE(hctx->dispatch_from, ctx);
+
+       return ret;
 }
 
 void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */
        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+               if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

block/blk-mq-sched.c void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) · line 220

Quoted 3 lines

block/blk-mq-sched.c +1 -1

                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

Quoted 6 lines

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Yes, this can work.

Quoted 8 lines

Quoted 6 lines

But I found your patch will drop some performance comparing with my method in patch 1/2. My method can fetch several requests from IO scheduler and dispatch them to block driver at one time, but in your patch we still need dispatch request one by one, which will drop some performance I think.
What do you think? Thanks.

Please run your test and see if performance drop can be observed.

From my testing (using the same fio configuration in cover letter), I found your method will drop some performance from below data.

My original patches:
Sequential read: 229.6MiB/s
Random read:180.8MiB/s
Sequential write: 172MiB/s
Random write:169.2MiB/s

Quoted 5 lines

Your patches:
Sequential read: 209MiB/s
Random read:177MiB/s
Sequential write: 148MiB/s
Random write:147MiB/s

After some optimiziton and I did more testing, I did not found any performance issue with your patch comparing with my old method. Sorry for noise in my last email.

So will you send out a formal patch? If yes, please add my test-by tag. Thanks for your help.

Tested-by Baolin Wang <[email protected]>

Hi Ming,

Quoted 271 lines · Baolin Wang

Hi Ming,

Quoted 257 lines · Baolin Wang

Hi Ming,

Quoted 235 lines · Ming Lei

Quoted 233 lines · Baolin Wang

Quoted 223 lines · Ming Lei

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 23 lines · Baolin Wang

Hi Ming,

Quoted 19 lines · Ming Lei

On Mon, Mar 16, 2020 at 06:01:19PM +0800, Baolin Wang wrote:
As we know, some SD/MMC host controllers can support packed request,
that means we can package several requests to host controller at one
time to improve performence. So the hardware driver expects the blk-mq
can dispatch a batch of requests at one time, and driver can use bd.last
to indicate if it is the last request in the batch to help to combine
requests as much as possible.

Thus we should add batch requests setting from the block driver to tell
the scheduler how many requests can be dispatched in a batch, as well
as changing the scheduler to dispatch more than one request if setting
the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other
drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in
.queue_rq().

Quoted 7 lines

Quoted 3 lines

So what about the following way by extending .commit_rqs() to this usage?
And you can do whatever batch processing in .commit_rqs() which will be
guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine. It seems to solve my problem, and I will try it on my platform to see if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

Quoted 75 lines

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 856356b1619e..cd2bbe56f83f 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
* its queue by itself in its completion handler, so we don't need to
* restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
*/
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
{
struct request_queue *q = hctx->queue;
struct elevator_queue *e = q->elevator;
LIST_HEAD(rq_list);
+ bool ret = false;

do {
struct request *rq;
@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
* in blk_mq_dispatch_rq_list().
*/
list_add(&rq->queuelist, &rq_list);
- } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+ ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+ } while (ret);
+
+ return ret;
}

static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
* its queue by itself in its completion handler, so we don't need to
* restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
*/
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
{
struct request_queue *q = hctx->queue;
LIST_HEAD(rq_list);
struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+ bool ret = false;

do {
struct request *rq;
@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)

/* round robin for fair dispatch */
ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
- } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+ ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+ } while (ret);

WRITE_ONCE(hctx->dispatch_from, ctx);
+
+ return ret;
}

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
struct elevator_queue *e = q->elevator;
const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
LIST_HEAD(rq_list);
+ bool dispatch_ret;

/* RCU or SRCU read lock is needed before checking quiesced flag */
if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))
@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
*/
if (!list_empty(&rq_list)) {
blk_mq_sched_mark_restart_hctx(hctx);
- if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+ dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+ if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

if (has_sched_dispatch)
- blk_mq_do_dispatch_sched(hctx);
+ dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

Quoted 6 lines

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Yes, this can work.

Quoted 8 lines

Quoted 6 lines

But I found your patch will drop some performance comparing with my method in patch 1/2. My method can fetch several requests from IO scheduler and dispatch them to block driver at one time, but in your patch we still need dispatch request one by one, which will drop some performance I think.
What do you think? Thanks.

Please run your test and see if performance drop can be observed.

From my testing (using the same fio configuration in cover letter), I found your method will drop some performance from below data.

My original patches:
Sequential read: 229.6MiB/s
Random read:180.8MiB/s
Sequential write: 172MiB/s
Random write:169.2MiB/s

Quoted 5 lines

Your patches:
Sequential read: 209MiB/s
Random read:177MiB/s
Sequential write: 148MiB/s
Random write:147MiB/s

After some optimiziton and I did more testing, I did not found any performance issue with your patch comparing with my old method. Sorry for noise in my last email.

Quoted 3 lines

So will you send out a formal patch? If yes, please add my test-by tag. Thanks for your help.

Tested-by Baolin Wang <[email protected]>

Can I take this patch into my patch set with your authority? Or you want to send it out by yourself? Thanks.

Quoted 278 lines · Baolin Wang

Hi Ming,

Quoted 274 lines · Baolin Wang

Hi Ming,

Quoted 257 lines · Baolin Wang

Hi Ming,

Quoted 235 lines · Ming Lei

Quoted 233 lines · Baolin Wang

Quoted 223 lines · Ming Lei

Quoted 214 lines · Baolin Wang

Quoted 170 lines · Ming Lei

Quoted 129 lines · Baolin Wang

Quoted 122 lines · Ming Lei

Quoted 111 lines · Baolin Wang

Hi Ming,

Quoted 30 lines · Baolin Wang

Hi Ming,

On Wed, Mar 18, 2020 at 6:01 PM Ming Lei <[email protected]> wrote:

On Mon, Mar 16, 2020 at 06:01:19PM +0800, Baolin Wang wrote:
As we know, some SD/MMC host controllers can support packed request,
that means we can package several requests to host controller at one
time to improve performence. So the hardware driver expects the blk-mq
can dispatch a batch of requests at one time, and driver can use bd.last
to indicate if it is the last request in the batch to help to combine
requests as much as possible.

Thus we should add batch requests setting from the block driver to tell
the scheduler how many requests can be dispatched in a batch, as well
as changing the scheduler to dispatch more than one request if setting
the maximum batch requests number.

I feel this batch dispatch style is more complicated, and some other
drivers(virtio blk/scsi) still may get benefit if we can pass real 'last' flag in
.queue_rq().

So what about the following way by extending .commit_rqs() to this usage?
And you can do whatever batch processing in .commit_rqs() which will be
guaranteed to be called if BLK_MQ_F_FORCE_COMMIT_RQS is set by driver.

I'm very appreciated for your good suggestion, which is much simpler than mine.
It seems to solve my problem, and I will try it on my platform to see
if it can work and give you the feadback. Thanks again.

I tried your approach on my platform, but met some problems, see below.

Quoted 75 lines

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 856356b1619e..cd2bbe56f83f 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -85,11 +85,12 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx)
* its queue by itself in its completion handler, so we don't need to
* restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
*/
-static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
{
struct request_queue *q = hctx->queue;
struct elevator_queue *e = q->elevator;
LIST_HEAD(rq_list);
+ bool ret = false;

do {
struct request *rq;
@@ -112,7 +113,10 @@ static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)
* in blk_mq_dispatch_rq_list().
*/
list_add(&rq->queuelist, &rq_list);
- } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+ ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+ } while (ret);
+
+ return ret;
}

static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
@@ -131,11 +135,12 @@ static struct blk_mq_ctx *blk_mq_next_ctx(struct blk_mq_hw_ctx *hctx,
* its queue by itself in its completion handler, so we don't need to
* restart queue if .get_budget() returns BLK_STS_NO_RESOURCE.
*/
-static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
+static bool blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)
{
struct request_queue *q = hctx->queue;
LIST_HEAD(rq_list);
struct blk_mq_ctx *ctx = READ_ONCE(hctx->dispatch_from);
+ bool ret = false;

do {
struct request *rq;
@@ -161,10 +166,12 @@ static void blk_mq_do_dispatch_ctx(struct blk_mq_hw_ctx *hctx)

/* round robin for fair dispatch */
ctx = blk_mq_next_ctx(hctx, rq->mq_ctx);
-
- } while (blk_mq_dispatch_rq_list(q, &rq_list, true));
+ ret = blk_mq_dispatch_rq_list(q, &rq_list, true);
+ } while (ret);

WRITE_ONCE(hctx->dispatch_from, ctx);
+
+ return ret;
}

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
struct elevator_queue *e = q->elevator;
const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
LIST_HEAD(rq_list);
+ bool dispatch_ret;

/* RCU or SRCU read lock is needed before checking quiesced flag */
if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))
@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
*/
if (!list_empty(&rq_list)) {
blk_mq_sched_mark_restart_hctx(hctx);
- if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+ dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
+ if (dispatch_ret) {

Quoted 8 lines

Quoted 3 lines

if (has_sched_dispatch)
- blk_mq_do_dispatch_sched(hctx);
+ dispatch_ret = blk_mq_do_dispatch_sched(hctx);

If we dispatched a request successfully by blk_mq_dispatch_rq_list(), and got dispatch_ret = true now. Then we will try to dispatch more reuqests from scheduler by blk_mq_do_dispatch_sched(), but if now no more requests in scheduler, then we will got dispatch_ret = false. In

Quoted 5 lines

'dispatch_ret' always holds result of the last blk_mq_do_dispatch_sched(). When any one request has been dispatched successfully, 'dispatch_ret' is true. New request is always added to list before calling blk_mq_do_dispatch_sched(), so once blk_mq_do_dispatch_sched() returns false, it means that .commit_rqs() has been called.

Quoted 3 lines

Not really, if no requests int the IO cheduler, we will break the loop in blk_mq_do_dispatch_sched() and return false without calling .commit_rqs().

If there isn't any request to dispatch, false is returned. Otherwise, always return the return value of last 'blk_mq_dispatch_rq_list'.

Quoted 4 lines

So in this case, blk_mq_do_dispatch_sched() will return 'false', which overlapped the return value of 'true' from blk_mq_dispatch_rq_list(), and did not call .commit_rqs(). Then the IO processing will be stuck.

See below.

Quoted 7 lines

static void blk_mq_do_dispatch_sched(struct blk_mq_hw_ctx *hctx)

{
        struct request_queue *q = hctx->queue;
        struct elevator_queue *e = q->elevator;
        LIST_HEAD(rq_list);
        bool ret = false;

The above initialization is just done once.

Quoted 8 lines

       do {
              struct request *rq;

              if (e->type->ops.has_work && !e->type->ops.has_work(hctx))
                     break;

              .......

                            ret = blk_mq_dispatch_rq_list(q, list, ...);

list includes one request, so blk_mq_dispatch_rq_list() won't return false in case of no request in list.

Quoted 2 lines

       } while (ret);

Quoted 7 lines

Quoted 1 line

       return ret;

'ret' is always updated by return value of last blk_mq_dispatch_rq_list() if at least one request is dispatched. So if it becomes false, the loop breaks, that means .commit_rqs() has been called cause 'list' does include one request for blk_mq_dispatch_rq_list(). Otherwise, true is still returned.

Sorry for my confusing description, let me try again to describe the problem. When I try to mount the block device, I got the IO stuck with your patch, and I did some debugging. I found we missed calling commit_rqs() for one case:

void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)

(quoted diff) +1 -0

@@ -173,6 +180,7 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
        struct elevator_queue *e = q->elevator;
        const bool has_sched_dispatch = e && e->type->ops.dispatch_request;
        LIST_HEAD(rq_list);
+       bool dispatch_ret;
 
        /* RCU or SRCU read lock is needed before checking quiesced flag */

        if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)))

(quoted diff) +1 -1

@@ -206,20 +214,26 @@ void blk_mq_sched_dispatch_requests(struct
 blk_mq_hw_ctx *hctx)
         */
        if (!list_empty(&rq_list)) {
                blk_mq_sched_mark_restart_hctx(hctx);
-               if (blk_mq_dispatch_rq_list(q, &rq_list, false)) {
+               dispatch_ret = blk_mq_dispatch_rq_list(q, &rq_list, false);
 

Suppose we dispatch one request to block driver, and return 'true' here.

(quoted diff) +2 -1

+               if (dispatch_ret) {
                        if (has_sched_dispatch)
-                               blk_mq_do_dispatch_sched(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_sched(hctx);

Then we will continue to try to dispatch more requests from IO scheduler, but if there are no requests in IO scheduler now, it will return 'false' here, and set dispatch_ret as false.

else

Review

Quoted 2 lines

(quoted diff) +1 -1

-                               blk_mq_do_dispatch_ctx(hctx);
+                               dispatch_ret = blk_mq_do_dispatch_ctx(hctx);

OK, this one is an issue, but it can be fixed simply by not updating 'dispatch_ret' for the following dispatch, something like the below way:

Quoted 6 lines

        if (dispatch_ret) {
                if (has_sched_dispatch)
                        blk_mq_do_dispatch_sched(hctx);
                else
                        blk_mq_do_dispatch_ctx(hctx);
        }

Yes, this can work.

Quoted 8 lines

Quoted 6 lines

But I found your patch will drop some performance comparing with my method in patch 1/2. My method can fetch several requests from IO scheduler and dispatch them to block driver at one time, but in your patch we still need dispatch request one by one, which will drop some performance I think.
What do you think? Thanks.

Please run your test and see if performance drop can be observed.

From my testing (using the same fio configuration in cover letter), I found your method will drop some performance from below data.

My original patches:
Sequential read: 229.6MiB/s
Random read:180.8MiB/s
Sequential write: 172MiB/s
Random write:169.2MiB/s

Quoted 5 lines

Your patches:
Sequential read: 209MiB/s
Random read:177MiB/s
Sequential write: 148MiB/s
Random write:147MiB/s

After some optimiziton and I did more testing, I did not found any performance issue with your patch comparing with my old method. Sorry for noise in my last email.

So will you send out a formal patch? If yes, please add my test-by tag. Thanks for your help.

Tested-by Baolin Wang <[email protected]>

Quoted 2 lines

Can I take this patch into my patch set with your authority? Or you want to send it out by yourself? Thanks.

Hi Baolin,

You can fold the patch into your series.