Re: [PATCH 3/6] block/blkio: convert to blk_io_plug_call() API

From: Stefano Garzarella <sgarzare@redhat.com>
To: Stefan Hajnoczi <stefanha@redhat.com>
Cc: qemu-devel@nongnu.org, xen-devel@lists.xenproject.org,
	"Julia Suvorova" <jusual@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Kevin Wolf" <kwolf@redhat.com>, "Fam Zheng" <fam@euphon.net>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	qemu-block@nongnu.org, "Paul Durrant" <paul@xen.org>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Aarushi Mehta" <mehta.aaru20@gmail.com>,
	"Hanna Reitz" <hreitz@redhat.com>
Subject: Re: [PATCH 3/6] block/blkio: convert to blk_io_plug_call() API
Date: Fri, 19 May 2023 10:47:00 +0200	[thread overview]
Message-ID: <wtyut5kd4v5vapon7fzpvi3kghvpplokcas5ovcwnjhiwyuccb@rm6eb6jjhhp5> (raw)
In-Reply-To: <20230517221022.325091-4-stefanha@redhat.com>

On Wed, May 17, 2023 at 06:10:19PM -0400, Stefan Hajnoczi wrote:
>Stop using the .bdrv_co_io_plug() API because it is not multi-queue
>block layer friendly. Use the new blk_io_plug_call() API to batch I/O
>submission instead.
>
>Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>---
> block/blkio.c | 40 +++++++++++++++++++++-------------------
> 1 file changed, 21 insertions(+), 19 deletions(-)

With this patch, the build fails in several places, maybe it's an old
version:

../block/blkio.c:347:5: error: implicit declaration of function 
‘blk_io_plug_call’ [-Werror=implicit-function-declaration]
   347 |     blk_io_plug_call(blkio_unplug_fn, bs);

../block/blkio.c:348:22: error: passing argument 1 of ‘blk_io_plug_call’ 
from incompatible pointer type [-Werror=incompatible-pointer-types]
   348 |     blk_io_plug_call(blkio_unplug_fn, bs);

Thanks,
Stefano

>
>diff --git a/block/blkio.c b/block/blkio.c
>index 0cdc99a729..f2a1dc1fb2 100644
>--- a/block/blkio.c
>+++ b/block/blkio.c
>@@ -325,16 +325,28 @@ static void blkio_detach_aio_context(BlockDriverState *bs)
>                        false, NULL, NULL, NULL, NULL, NULL);
> }
>
>-/* Call with s->blkio_lock held to submit I/O after enqueuing a new request */
>-static void blkio_submit_io(BlockDriverState *bs)
>+/*
>+ * Called by blk_io_unplug() or immediately if not plugged. Called without
>+ * blkio_lock.
>+ */
>+static void blkio_unplug_fn(BlockDriverState *bs)
> {
>-    if (qatomic_read(&bs->io_plugged) == 0) {
>-        BDRVBlkioState *s = bs->opaque;
>+    BDRVBlkioState *s = bs->opaque;
>
>+    WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>         blkioq_do_io(s->blkioq, NULL, 0, 0, NULL);
>     }
> }
>
>+/*
>+ * Schedule I/O submission after enqueuing a new request. Called without
>+ * blkio_lock.
>+ */
>+static void blkio_submit_io(BlockDriverState *bs)
>+{
>+    blk_io_plug_call(blkio_unplug_fn, bs);
>+}
>+
> static int coroutine_fn
> blkio_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
> {
>@@ -345,9 +357,9 @@ blkio_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
>
>     WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>         blkioq_discard(s->blkioq, offset, bytes, &cod, 0);
>-        blkio_submit_io(bs);
>     }
>
>+    blkio_submit_io(bs);
>     qemu_coroutine_yield();
>     return cod.ret;
> }
>@@ -378,9 +390,9 @@ blkio_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
>
>     WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>         blkioq_readv(s->blkioq, offset, iov, iovcnt, &cod, 0);
>-        blkio_submit_io(bs);
>     }
>
>+    blkio_submit_io(bs);
>     qemu_coroutine_yield();
>
>     if (use_bounce_buffer) {
>@@ -423,9 +435,9 @@ static int coroutine_fn blkio_co_pwritev(BlockDriverState *bs, int64_t offset,
>
>     WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>         blkioq_writev(s->blkioq, offset, iov, iovcnt, &cod, blkio_flags);
>-        blkio_submit_io(bs);
>     }
>
>+    blkio_submit_io(bs);
>     qemu_coroutine_yield();
>
>     if (use_bounce_buffer) {
>@@ -444,9 +456,9 @@ static int coroutine_fn blkio_co_flush(BlockDriverState *bs)
>
>     WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>         blkioq_flush(s->blkioq, &cod, 0);
>-        blkio_submit_io(bs);
>     }
>
>+    blkio_submit_io(bs);
>     qemu_coroutine_yield();
>     return cod.ret;
> }
>@@ -472,22 +484,13 @@ static int coroutine_fn blkio_co_pwrite_zeroes(BlockDriverState *bs,
>
>     WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>         blkioq_write_zeroes(s->blkioq, offset, bytes, &cod, blkio_flags);
>-        blkio_submit_io(bs);
>     }
>
>+    blkio_submit_io(bs);
>     qemu_coroutine_yield();
>     return cod.ret;
> }
>
>-static void coroutine_fn blkio_co_io_unplug(BlockDriverState *bs)
>-{
>-    BDRVBlkioState *s = bs->opaque;
>-
>-    WITH_QEMU_LOCK_GUARD(&s->blkio_lock) {
>-        blkio_submit_io(bs);
>-    }
>-}
>-
> typedef enum {
>     BMRR_OK,
>     BMRR_SKIP,
>@@ -1009,7 +1012,6 @@ static void blkio_refresh_limits(BlockDriverState *bs, Error **errp)
>         .bdrv_co_pwritev         = blkio_co_pwritev, \
>         .bdrv_co_flush_to_disk   = blkio_co_flush, \
>         .bdrv_co_pwrite_zeroes   = blkio_co_pwrite_zeroes, \
>-        .bdrv_co_io_unplug       = blkio_co_io_unplug, \
>         .bdrv_refresh_limits     = blkio_refresh_limits, \
>         .bdrv_register_buf       = blkio_register_buf, \
>         .bdrv_unregister_buf     = blkio_unregister_buf, \
>-- 
>2.40.1
>