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

[PATCH 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. This patch 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 19da5defd734..cf64d256787e 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
 	rq_list_for_each(&iob->req_list, req) {
 		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
 		virtblk_cleanup_cmd(req);
+		blk_mq_set_request_complete(req);
 	}
 	blk_mq_end_request_batch(iob);
 }
-- 
2.26.3

[PATCH 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 | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index cf64d256787e..48fcf745f007 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -272,6 +272,18 @@ 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_set_request_complete(req);
+	}
+	blk_mq_end_request_batch(iob);
+}
+
 static void virtblk_done(struct virtqueue *vq)
 {
 	struct virtio_blk *vblk = vq->vdev->priv;
@@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
 	struct virtblk_req *vbr;
 	unsigned long flags;
 	unsigned int len;
+	DEFINE_IO_COMP_BATCH(iob);
 
 	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
 	do {
@@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
 		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
 			struct request *req = blk_mq_rq_from_pdu(vbr);
 
-			if (likely(!blk_should_fake_timeout(req->q)))
+			if (likely(!blk_should_fake_timeout(req->q)) &&
+				!blk_mq_add_to_batch(req, &iob, vbr->status,
+							virtblk_complete_batch))
 				blk_mq_complete_request(req);
 			req_done = true;
 		}
@@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *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))
+			virtblk_complete_batch(&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);
 }
 
@@ -832,18 +852,6 @@ 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_set_request_complete(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;
-- 
2.26.3

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3594 bytes --]

On Tue, Dec 06, 2022 at 11:11:25PM +0900, Suwan Kim wrote:
> 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 | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)

Cool, thanks for doing this!

> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index cf64d256787e..48fcf745f007 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -272,6 +272,18 @@ 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_set_request_complete(req);
> +	}
> +	blk_mq_end_request_batch(iob);
> +}
> +
>  static void virtblk_done(struct virtqueue *vq)
>  {
>  	struct virtio_blk *vblk = vq->vdev->priv;
> @@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
>  	struct virtblk_req *vbr;
>  	unsigned long flags;
>  	unsigned int len;
> +	DEFINE_IO_COMP_BATCH(iob);
>  
>  	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
>  	do {
> @@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
>  		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
>  			struct request *req = blk_mq_rq_from_pdu(vbr);
>  
> -			if (likely(!blk_should_fake_timeout(req->q)))
> +			if (likely(!blk_should_fake_timeout(req->q)) &&
> +				!blk_mq_add_to_batch(req, &iob, vbr->status,
> +							virtblk_complete_batch))
>  				blk_mq_complete_request(req);
>  			req_done = true;
>  		}
> @@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *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))
> +			virtblk_complete_batch(&iob);

A little optimization to avoid the indirect call: iob.complete(&iob) :).
Not sure if it's good style to do that but it works in this case because
we know it can only be virtblk_complete_batch().

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Suwan Kim]

On Wed, Dec 07, 2022 at 05:05:06PM -0500, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:25PM +0900, Suwan Kim wrote:
> > 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 | 38 +++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> Cool, thanks for doing this!
> 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index cf64d256787e..48fcf745f007 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -272,6 +272,18 @@ 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_set_request_complete(req);
> > +	}
> > +	blk_mq_end_request_batch(iob);
> > +}
> > +
> >  static void virtblk_done(struct virtqueue *vq)
> >  {
> >  	struct virtio_blk *vblk = vq->vdev->priv;
> > @@ -280,6 +292,7 @@ static void virtblk_done(struct virtqueue *vq)
> >  	struct virtblk_req *vbr;
> >  	unsigned long flags;
> >  	unsigned int len;
> > +	DEFINE_IO_COMP_BATCH(iob);
> >  
> >  	spin_lock_irqsave(&vblk->vqs[qid].lock, flags);
> >  	do {
> > @@ -287,7 +300,9 @@ static void virtblk_done(struct virtqueue *vq)
> >  		while ((vbr = virtqueue_get_buf(vblk->vqs[qid].vq, &len)) != NULL) {
> >  			struct request *req = blk_mq_rq_from_pdu(vbr);
> >  
> > -			if (likely(!blk_should_fake_timeout(req->q)))
> > +			if (likely(!blk_should_fake_timeout(req->q)) &&
> > +				!blk_mq_add_to_batch(req, &iob, vbr->status,
> > +							virtblk_complete_batch))
> >  				blk_mq_complete_request(req);
> >  			req_done = true;
> >  		}
> > @@ -295,9 +310,14 @@ static void virtblk_done(struct virtqueue *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))
> > +			virtblk_complete_batch(&iob);
> 
> A little optimization to avoid the indirect call: iob.complete(&iob) :).
> Not sure if it's good style to do that but it works in this case because
> we know it can only be virtblk_complete_batch().
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Hi Stefan,

Thanks for the comment!
It also needs to use blk_mq_complete_request_remote() instead of
blk_mq_set_request_complete()
I will resend it with the modification of patch #1 and then please
review it again.

Regards,
Suwan Kim

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

On Tue, Dec 06, 2022 at 11:11:24PM +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. This patch 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 19da5defd734..cf64d256787e 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
>  	rq_list_for_each(&iob->req_list, req) {
>  		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
>  		virtblk_cleanup_cmd(req);
> +		blk_mq_set_request_complete(req);
>  	}
>  	blk_mq_end_request_batch(iob);
>  }

The doc comment for blk_mq_set_request_complete() mentions this being
used in ->queue_rq(), but that's not the case here. Does the doc comment
need to be updated if we're using the function in a different way?

I'm not familiar enough with the Linux block APIs, but this feels weird
to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
Why does it set the state to IDLE instead of COMPLETE?

I think Jens can confirm whether we really want all drivers that use
polling and io_comp_batch to manually call
blk_mq_set_request_complete().

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jens Axboe]

On 12/7/22 2:47 PM, Stefan Hajnoczi wrote:
> On Tue, Dec 06, 2022 at 11:11:24PM +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. This patch 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 | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 19da5defd734..cf64d256787e 100644
>> --- a/drivers/block/virtio_blk.c
>> +++ b/drivers/block/virtio_blk.c
>> @@ -839,6 +839,7 @@ static void virtblk_complete_batch(struct io_comp_batch *iob)
>>  	rq_list_for_each(&iob->req_list, req) {
>>  		virtblk_unmap_data(req, blk_mq_rq_to_pdu(req));
>>  		virtblk_cleanup_cmd(req);
>> +		blk_mq_set_request_complete(req);
>>  	}
>>  	blk_mq_end_request_batch(iob);
>>  }
> 
> The doc comment for blk_mq_set_request_complete() mentions this being
> used in ->queue_rq(), but that's not the case here. Does the doc comment
> need to be updated if we're using the function in a different way?

Looks like it's a bit outdated...

> I'm not familiar enough with the Linux block APIs, but this feels weird
> to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> Why does it set the state to IDLE instead of COMPLETE?
> 
> I think Jens can confirm whether we really want all drivers that use
> polling and io_comp_batch to manually call
> blk_mq_set_request_complete().

Should not be a need to call blk_mq_set_request_complete() directly in
the driver for this.

-- 
Jens Axboe

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

[Christoph Hellwig]

On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote:
> > The doc comment for blk_mq_set_request_complete() mentions this being
> > used in ->queue_rq(), but that's not the case here. Does the doc comment
> > need to be updated if we're using the function in a different way?
> 
> Looks like it's a bit outdated...

I think the comment is still entirely correct.

> 
> > I'm not familiar enough with the Linux block APIs, but this feels weird
> > to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> > Why does it set the state to IDLE instead of COMPLETE?
> > 
> > I think Jens can confirm whether we really want all drivers that use
> > polling and io_comp_batch to manually call
> > blk_mq_set_request_complete().
> 
> Should not be a need to call blk_mq_set_request_complete() directly in
> the driver for this.

Exactly.  Polling or not, drivers should go through the normal completion
interface, that is blk_mq_complete_request or the lower-level options
blk_mq_complete_request_remote and blk_mq_complete_request_direct.

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

[Suwan Kim]

On Sun, Dec 11, 2022 at 10:50:03PM -0800, Christoph Hellwig wrote:
> On Thu, Dec 08, 2022 at 09:48:23AM -0700, Jens Axboe wrote:
> > > The doc comment for blk_mq_set_request_complete() mentions this being
> > > used in ->queue_rq(), but that's not the case here. Does the doc comment
> > > need to be updated if we're using the function in a different way?
> > 
> > Looks like it's a bit outdated...
> 
> I think the comment is still entirely correct.
> 
> > 
> > > I'm not familiar enough with the Linux block APIs, but this feels weird
> > > to me. Shouldn't blk_mq_end_request_batch(iob) take care of this for us?
> > > Why does it set the state to IDLE instead of COMPLETE?
> > > 
> > > I think Jens can confirm whether we really want all drivers that use
> > > polling and io_comp_batch to manually call
> > > blk_mq_set_request_complete().
> > 
> > Should not be a need to call blk_mq_set_request_complete() directly in
> > the driver for this.
> 
> Exactly.  Polling or not, drivers should go through the normal completion
> interface, that is blk_mq_complete_request or the lower-level options
> blk_mq_complete_request_remote and blk_mq_complete_request_direct.

Hi all,

Thanks for the comment. It was the wrong use of the function...
I will use blk_mq_complete_request_remote() instead of
blk_mq_set_request_complete() and send next version soon.

Regards,
Suwan Kim