[PATCH v2 0/2] virtio-blk: set req->state to MQ_RQ_COMPLETE and support completion batching for the IRQ path

[PATCH v2 0/2] virtio-blk: set req->state to MQ_RQ_COMPLETE and support completion batching for the IRQ path

[Suwan Kim]

V2 changes
 - patch#1
  - use blk_mq_complete_request_remote() instead of blk_mq_set_request_complete()

 - patch#2
  - Modify patch based on the patch#1 v2 changes.
  - Use iob.complete(&iob) instead of virtblk_complete_batch(&iob)
    at virtblk_done() (Stefan's comment)
  - It shows almost same performance improvement as v1 patch.

Suwan Kim (2):
  virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is
    finished
  virtio-blk: support completion batching for the IRQ path

 drivers/block/virtio_blk.c | 81 +++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 36 deletions(-)

-- 
2.26.3

[PATCH v2 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished

[Suwan Kim]

Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
is no issue about it because blk_mq_end_request_batch() sets req->state to
MQ_RQ_IDLE.

In this patch, virblk_poll() calls blk_mq_complete_request_remote() to set
req->state to MQ_RQ_COMPLETE before it adds req to a batch completion list.
So it properly sets req->state after polling I/O is finished.

Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
 drivers/block/virtio_blk.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 19da5defd734..75ee51aba964 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -858,9 +858,10 @@ static int virtblk_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
 		struct request *req = blk_mq_rq_from_pdu(vbr);
 
 		found++;
-		if (!blk_mq_add_to_batch(req, iob, vbr->status,
+		if (!blk_mq_complete_request_remote(req) &&
+		    !blk_mq_add_to_batch(req, iob, vbr->status,
 						virtblk_complete_batch))
-			blk_mq_complete_request(req);
+			virtblk_request_done(req);
 	}
 
 	if (found)
-- 
2.26.3

Re: [PATCH v2 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished

[Christoph Hellwig]

On Wed, Dec 21, 2022 at 12:36:12AM +0900, Suwan Kim wrote:
> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
> is no issue about it because blk_mq_end_request_batch() sets req->state to
> MQ_RQ_IDLE.
> 
> In this patch, virblk_poll() calls blk_mq_complete_request_remote() to set
> req->state to MQ_RQ_COMPLETE before it adds req to a batch completion list.
> So it properly sets req->state after polling I/O is finished.
> 
> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 1/2] virtio-blk: set req->state to MQ_RQ_COMPLETE after polling I/O is finished

[Christoph Hellwig]

On Wed, Dec 21, 2022 at 12:36:12AM +0900, Suwan Kim wrote:
> Driver should set req->state to MQ_RQ_COMPLETE after it finishes to process
> req. But virtio-blk doesn't set MQ_RQ_COMPLETE after virtblk_poll() handles
> req and req->state still remains MQ_RQ_IN_FLIGHT. Fortunately so far there
> is no issue about it because blk_mq_end_request_batch() sets req->state to
> MQ_RQ_IDLE.
> 
> In this patch, virblk_poll() calls blk_mq_complete_request_remote() to set
> req->state to MQ_RQ_COMPLETE before it adds req to a batch completion list.
> So it properly sets req->state after polling I/O is finished.
> 
> Fixes: 4e0400525691 ("virtio-blk: support polling I/O")
> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v2 2/2] virtio-blk: support completion batching for the IRQ path

[Suwan Kim]

This patch adds completion batching to the IRQ path. It reuses batch
completion code of virtblk_poll(). It collects requests to io_comp_batch
and processes them all at once. It can boost up the performance by 2%.

To validate the performance improvement and stabilty, I did fio test with
4 vCPU VM and 12 vCPU VM respectively. Both VMs have 8GB ram and the same
number of HW queues as vCPU.
The fio cammad is as follows and I ran the fio 5 times and got IOPS average.
(io_uring, randread, direct=1, bs=512, iodepth=64 numjobs=2,4)

Test result shows about 2% improvement.

           4 vcpu VM       |   numjobs=2   |   numjobs=4
      -----------------------------------------------------------
        fio without patch  |  367.2K IOPS  |   397.6K IOPS
      -----------------------------------------------------------
        fio with patch     |  372.8K IOPS  |   407.7K IOPS

           12 vcpu VM      |   numjobs=2   |   numjobs=4
      -----------------------------------------------------------
        fio without patch  |  363.6K IOPS  |   374.8K IOPS
      -----------------------------------------------------------
        fio with patch     |  373.8K IOPS  |   385.3K IOPS

Signed-off-by: Suwan Kim <suwan.kim027@gmail.com>
---
 drivers/block/virtio_blk.c | 82 +++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 37 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 75ee51aba964..0ac7b570300c 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -272,33 +272,63 @@ static inline void virtblk_request_done(struct request *req)
 	blk_mq_end_request(req, virtblk_result(vbr));
 }
 
+static void virtblk_complete_batch(struct io_comp_batch *iob)
+{
+	struct request *req;
+
+	rq_list_for_each(&iob->req_list, req) {
+		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
+		virtblk_cleanup_cmd(req);
+	}
+	blk_mq_end_request_batch(iob);
+}
+
+static int virtblk_handle_req(struct virtio_blk_vq *vq,
+				struct io_comp_batch *iob)
+{
+	struct virtblk_req *vbr;
+	int req_done = 0;
+	unsigned int len;
+
+	while ((vbr = virtqueue_get_buf(vq->vq, &len)) != NULL) {
+		struct request *req = blk_mq_rq_from_pdu(vbr);
+
+		if (likely(!blk_should_fake_timeout(req->q)) &&
+			!blk_mq_complete_request_remote(req) &&
+			!blk_mq_add_to_batch(req, iob, vbr->status,
+						virtblk_complete_batch))
+			virtblk_request_done(req);
+		req_done++;
+	}
+
+	return req_done;
+}
+
 static void virtblk_done(struct virtqueue *vq)
 {
 	struct virtio_blk *vblk = vq->vdev->priv;
-	bool req_done = false;
-	int qid = vq->index;
-	struct virtblk_req *vbr;
+	struct virtio_blk_vq *vblk_vq = &vblk->vqs[vq->index];
+	int req_done = 0;
 	unsigned long flags;
-	unsigned int len;
+	DEFINE_IO_COMP_BATCH(iob);
 
-	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
+	spin_lock_irqsave(&vblk_vq->lock, flags);
 	do {
 		virtqueue_disable_cb(vq);
-		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
-			struct request *req = blk_mq_rq_from_pdu(vbr);
+		req_done += virtblk_handle_req(vblk_vq, &iob);
 
-			if (likely(!blk_should_fake_timeout(req->q)))
-				blk_mq_complete_request(req);
-			req_done = true;
-		}
 		if (unlikely(virtqueue_is_broken(vq)))
 			break;
 	} while (!virtqueue_enable_cb(vq));
 
-	/* In case queue is stopped waiting for more buffers. */
-	if (req_done)
+	if (req_done) {
+		if (!rq_list_empty(iob.req_list))
+			iob.complete(&iob);
+
+		/* In case queue is stopped waiting for more buffers. */
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
-	spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
+	}
+	spin_unlock_irqrestore(&vblk_vq->lock, flags);
 }
 
 static void virtio_commit_rqs(struct blk_mq_hw_ctx *hctx)
@@ -832,37 +862,15 @@ static void virtblk_map_queues(struct blk_mq_tag_set *set)
 	}
 }
 
-static void virtblk_complete_batch(struct io_comp_batch *iob)
-{
-	struct request *req;
-
-	rq_list_for_each(&iob->req_list, req) {
-		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
-		virtblk_cleanup_cmd(req);
-	}
-	blk_mq_end_request_batch(iob);
-}
-
 static int virtblk_poll(struct blk_mq_hw_ctx *hctx, struct io_comp_batch *iob)
 {
 	struct virtio_blk *vblk = hctx->queue->queuedata;
 	struct virtio_blk_vq *vq = get_virtio_blk_vq(hctx);
-	struct virtblk_req *vbr;
 	unsigned long flags;
-	unsigned int len;
 	int found = 0;
 
 	spin_lock_irqsave(&vq->lock, flags);
-
-	while ((vbr = virtqueue_get_buf(vq->vq, &len)) != NULL) {
-		struct request *req = blk_mq_rq_from_pdu(vbr);
-
-		found++;
-		if (!blk_mq_complete_request_remote(req) &&
-		    !blk_mq_add_to_batch(req, iob, vbr->status,
-						virtblk_complete_batch))
-			virtblk_request_done(req);
-	}
+	found = virtblk_handle_req(vq, iob);
 
 	if (found)
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
-- 
2.26.3

Re: [PATCH v2 2/2] virtio-blk: support completion batching for the IRQ path

[Christoph Hellwig]

> +		if (likely(!blk_should_fake_timeout(req->q)) &&
> +			!blk_mq_complete_request_remote(req) &&
> +			!blk_mq_add_to_batch(req, iob, vbr->status,
> +						virtblk_complete_batch))

One tab indents for line continuations are really confusing.  Please
make this:

		if (likely(!blk_should_fake_timeout(req->q)) &&
		    !blk_mq_complete_request_remote(req) &&
		    !blk_mq_add_to_batch(req, iob, vbr->status,
					 virtblk_complete_batch))

> +	found = virtblk_handle_req(vq, iob);
>  
>  	if (found)

You can drop the found variable here now:

	if (virtblk_handle_req(vq, iob))
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/2] virtio-blk: support completion batching for the IRQ path

[Christoph Hellwig]

> +		if (likely(!blk_should_fake_timeout(req->q)) &&
> +			!blk_mq_complete_request_remote(req) &&
> +			!blk_mq_add_to_batch(req, iob, vbr->status,
> +						virtblk_complete_batch))

One tab indents for line continuations are really confusing.  Please
make this:

		if (likely(!blk_should_fake_timeout(req->q)) &&
		    !blk_mq_complete_request_remote(req) &&
		    !blk_mq_add_to_batch(req, iob, vbr->status,
					 virtblk_complete_batch))

> +	found = virtblk_handle_req(vq, iob);
>  
>  	if (found)

You can drop the found variable here now:

	if (virtblk_handle_req(vq, iob))
 		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/2] virtio-blk: support completion batching for the IRQ path

[Suwan Kim]

On Wed, Dec 21, 2022 at 02:48:30AM -0800, Christoph Hellwig wrote:
> > +		if (likely(!blk_should_fake_timeout(req->q)) &&
> > +			!blk_mq_complete_request_remote(req) &&
> > +			!blk_mq_add_to_batch(req, iob, vbr->status,
> > +						virtblk_complete_batch))
> 
> One tab indents for line continuations are really confusing.  Please
> make this:
> 
> 		if (likely(!blk_should_fake_timeout(req->q)) &&
> 		    !blk_mq_complete_request_remote(req) &&
> 		    !blk_mq_add_to_batch(req, iob, vbr->status,
> 					 virtblk_complete_batch))
> 
> > +	found = virtblk_handle_req(vq, iob);
> >  
> >  	if (found)
> 
> You can drop the found variable here now:
> 
> 	if (virtblk_handle_req(vq, iob))
>  		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
> 

Hi Christoph,

Thanks for the comment!
I will send v3.

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Regards,
Suwan Kim

Re: [PATCH v2 2/2] virtio-blk: support completion batching for the IRQ path

[Suwan Kim]

On Wed, Dec 21, 2022 at 10:46 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
>
> On Wed, Dec 21, 2022 at 02:48:30AM -0800, Christoph Hellwig wrote:
> > > +           if (likely(!blk_should_fake_timeout(req->q)) &&
> > > +                   !blk_mq_complete_request_remote(req) &&
> > > +                   !blk_mq_add_to_batch(req, iob, vbr->status,
> > > +                                           virtblk_complete_batch))
> >
> > One tab indents for line continuations are really confusing.  Please
> > make this:
> >
> >               if (likely(!blk_should_fake_timeout(req->q)) &&
> >                   !blk_mq_complete_request_remote(req) &&
> >                   !blk_mq_add_to_batch(req, iob, vbr->status,
> >                                        virtblk_complete_batch))
> >
> > > +   found = virtblk_handle_req(vq, iob);
> > >
> > >     if (found)
> >
> > You can drop the found variable here now:
> >
> >       if (virtblk_handle_req(vq, iob))
> >               blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
> >
>

Ah, virtblk_poll() should return found to indicate whether a request
was processed. I'll leave this as is.

Regards,
Suwan Kim