From: Stefano Garzarella <sgarzare@redhat.com>

Until libblkio 1.3.0, virtio-blk drivers had completion eventfd

notifications enabled from the start, but from the next releases

this is no longer the case, so we have to explicitly enable them.

In fact, the libblkio documentation says they could be disabled,

so we should always enable them at the start if we want to be

sure to get completion eventfd notifications:

By default, the driver might not generate completion events for

requests so it is necessary to explicitly enable the completion

file descriptor before use:

void blkioq_set_completion_fd_enabled(struct blkioq *q, bool enable);

I discovered this while trying a development version of libblkio:

the guest kernel hangs during boot, while probing the device.

Fixes: fd66dbd424f5 ("blkio: add libblkio block driver")

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-id: 20230725103744.77343-1-sgarzare@redhat.com

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

---

block/blkio.c | 1 +

1 file changed, 1 insertion(+)

diff --git a/block/blkio.c b/block/blkio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/blkio.c

+++ b/block/blkio.c

@@ -XXX,XX +XXX,XX @@ static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,

QLIST_INIT(&s->bounce_bufs);

s->blkioq = blkio_get_queue(s->blkio, 0);

s->completion_fd = blkioq_get_completion_fd(s->blkioq);

+ blkioq_set_completion_fd_enabled(s->blkioq, true);

blkio_attach_aio_context(bs, bdrv_get_aio_context(bs));

return 0;

--

2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

qemu_open() in blkio_virtio_blk_common_open() is used to open the

character device (e.g. /dev/vhost-vdpa-0 or /dev/vfio/vfio) or in

the future eventually the unix socket.

In all these cases we cannot open the path in read-only mode,

when the `read-only` option of blockdev is on, because the exchange

of IOCTL commands for example will fail.

In order to open the device read-only, we have to use the `read-only`

property of the libblkio driver as we already do in blkio_file_open().

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2225439

Reported-by: Qing Wang <qinwang@redhat.com>

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Message-id: 20230726074807.14041-1-sgarzare@redhat.com

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

---

block/blkio.c | 21 ++++++++++++---------

1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/blkio.c

+++ b/block/blkio.c

@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,

* layer through the "/dev/fdset/N" special path.

*/

if (fd_supported) {

- int open_flags;

-

- if (flags & BDRV_O_RDWR) {

- open_flags = O_RDWR;

- } else {

- open_flags = O_RDONLY;

- }

-

- fd = qemu_open(path, open_flags, errp);

+ /*

+ * `path` can contain the path of a character device

+ * (e.g. /dev/vhost-vdpa-0 or /dev/vfio/vfio) or a unix socket.

+ *

+ * So, we should always open it with O_RDWR flag, also if BDRV_O_RDWR

+ * is not set in the open flags, because the exchange of IOCTL commands

+ * for example will fail.

+ *

+ * In order to open the device read-only, we are using the `read-only`

+ * property of the libblkio driver in blkio_file_open().

+ */

+ fd = qemu_open(path, O_RDWR, errp);

if (fd < 0) {

return -EINVAL;

}

--

2.41.0

From: Hanna Czenczek <hreitz@redhat.com>

bdrv_pad_request() relies on requests' lengths not to exceed SIZE_MAX,

which bdrv_check_qiov_request() does not guarantee.

bdrv_check_request32() however will guarantee this, and both of

bdrv_pad_request()'s callers (bdrv_co_preadv_part() and

bdrv_co_pwritev_part()) already run it before calling

bdrv_pad_request(). Therefore, bdrv_pad_request() can safely call

bdrv_check_request32() without expecting error, too.

In effect, this patch will not change guest-visible behavior. It is a

clean-up to tighten a condition to match what is guaranteed by our

callers, and which exists purely to show clearly why the subsequent

assertion (`assert(*bytes <= SIZE_MAX)`) is always true.

Note there is a difference between the interfaces of

bdrv_check_qiov_request() and bdrv_check_request32(): The former takes

an errp, the latter does not, so we can no longer just pass

&error_abort. Instead, we need to check the returned value. While we

do expect success (because the callers have already run this function),

an assert(ret == 0) is not much simpler than just to return an error if

it occurs, so let us handle errors by returning them up the stack now.

Reported-by: Peter Maydell <peter.maydell@linaro.org>

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

Message-id: 20230714085938.202730-1-hreitz@redhat.com

Fixes: 18743311b829cafc1737a5f20bc3248d5f91ee2a

("block: Collapse padded I/O vecs exceeding IOV_MAX")

Signed-off-by: Hanna Czenczek <hreitz@redhat.com>

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

---

block/io.c | 8 ++++++--

1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c

index XXXXXXX..XXXXXXX 100644

--- a/block/io.c

+++ b/block/io.c

@@ -XXX,XX +XXX,XX @@ static int bdrv_pad_request(BlockDriverState *bs,

int sliced_niov;

size_t sliced_head, sliced_tail;

- bdrv_check_qiov_request(*offset, *bytes, *qiov, *qiov_offset, &error_abort);

+ /* Should have been checked by the caller already */

+ ret = bdrv_check_request32(*offset, *bytes, *qiov, *qiov_offset);

+ if (ret < 0) {

+ return ret;

+ }

if (!bdrv_init_padding(bs, *offset, *bytes, write, pad)) {

if (padded) {

@@ -XXX,XX +XXX,XX @@ static int bdrv_pad_request(BlockDriverState *bs,

&sliced_head, &sliced_tail,

&sliced_niov);

- /* Guaranteed by bdrv_check_qiov_request() */

+ /* Guaranteed by bdrv_check_request32() */

assert(*bytes <= SIZE_MAX);

ret = bdrv_create_padded_qiov(bs, pad, sliced_iov, sliced_niov,

sliced_head, *bytes);

--

2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

This is in preparation for the next patch, where for virtio-blk

drivers we need to handle the failure of blkio_connect().

Let's also rename the *_open() functions to *_connect() to make

the code reflect the changes applied.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-id: 20230727161020.84213-2-sgarzare@redhat.com

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

---

block/blkio.c | 71 ++++++++++++++++++++++++++++++---------------------

1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/blkio.c

+++ b/block/blkio.c

@@ -XXX,XX +XXX,XX @@ static void blkio_unregister_buf(BlockDriverState *bs, void *host, size_t size)

}

}

-static int blkio_io_uring_open(BlockDriverState *bs, QDict *options, int flags,

- Error **errp)

+static int blkio_io_uring_connect(BlockDriverState *bs, QDict *options,

+ int flags, Error **errp)

{

const char *filename = qdict_get_str(options, "filename");

BDRVBlkioState *s = bs->opaque;

@@ -XXX,XX +XXX,XX @@ static int blkio_io_uring_open(BlockDriverState *bs, QDict *options, int flags,

}

}

+ ret = blkio_connect(s->blkio);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "blkio_connect failed: %s",

+ blkio_get_error_msg());

+ return ret;

+ }

+

return 0;

}

-static int blkio_nvme_io_uring(BlockDriverState *bs, QDict *options, int flags,

- Error **errp)

+static int blkio_nvme_io_uring_connect(BlockDriverState *bs, QDict *options,

+ int flags, Error **errp)

{

const char *path = qdict_get_try_str(options, "path");

BDRVBlkioState *s = bs->opaque;

@@ -XXX,XX +XXX,XX @@ static int blkio_nvme_io_uring(BlockDriverState *bs, QDict *options, int flags,

return -EINVAL;

}

+ ret = blkio_connect(s->blkio);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "blkio_connect failed: %s",

+ blkio_get_error_msg());

+ return ret;

+ }

+

return 0;

}

-static int blkio_virtio_blk_common_open(BlockDriverState *bs,

- QDict *options, int flags, Error **errp)

+static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,

+ int flags, Error **errp)

{

const char *path = qdict_get_try_str(options, "path");

BDRVBlkioState *s = bs->opaque;

@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,

}

}

+ ret = blkio_connect(s->blkio);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "blkio_connect failed: %s",

+ blkio_get_error_msg());

+ return ret;

+ }

+

qdict_del(options, "path");

return 0;

@@ -XXX,XX +XXX,XX @@ static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,

return ret;

}

+ if (!(flags & BDRV_O_RDWR)) {

+ ret = blkio_set_bool(s->blkio, "read-only", true);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "failed to set read-only: %s",

+ blkio_get_error_msg());

+ blkio_destroy(&s->blkio);

+ return ret;

+ }

+ }

+

if (strcmp(blkio_driver, "io_uring") == 0) {

- ret = blkio_io_uring_open(bs, options, flags, errp);

+ ret = blkio_io_uring_connect(bs, options, flags, errp);

} else if (strcmp(blkio_driver, "nvme-io_uring") == 0) {

- ret = blkio_nvme_io_uring(bs, options, flags, errp);

+ ret = blkio_nvme_io_uring_connect(bs, options, flags, errp);

} else if (strcmp(blkio_driver, "virtio-blk-vfio-pci") == 0) {

- ret = blkio_virtio_blk_common_open(bs, options, flags, errp);

+ ret = blkio_virtio_blk_connect(bs, options, flags, errp);

} else if (strcmp(blkio_driver, "virtio-blk-vhost-user") == 0) {

- ret = blkio_virtio_blk_common_open(bs, options, flags, errp);

+ ret = blkio_virtio_blk_connect(bs, options, flags, errp);

} else if (strcmp(blkio_driver, "virtio-blk-vhost-vdpa") == 0) {

- ret = blkio_virtio_blk_common_open(bs, options, flags, errp);

+ ret = blkio_virtio_blk_connect(bs, options, flags, errp);

} else {

g_assert_not_reached();

}

@@ -XXX,XX +XXX,XX @@ static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,

return ret;

}

- if (!(flags & BDRV_O_RDWR)) {

- ret = blkio_set_bool(s->blkio, "read-only", true);

- if (ret < 0) {

- error_setg_errno(errp, -ret, "failed to set read-only: %s",

- blkio_get_error_msg());

- blkio_destroy(&s->blkio);

- return ret;

- }

- }

-

- ret = blkio_connect(s->blkio);

- if (ret < 0) {

- error_setg_errno(errp, -ret, "blkio_connect failed: %s",

- blkio_get_error_msg());

- blkio_destroy(&s->blkio);

- return ret;

- }

-

ret = blkio_get_bool(s->blkio,

"needs-mem-regions",

&s->needs_mem_regions);

--

2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

libblkio 1.3.0 added support of "fd" property for virtio-blk-vhost-vdpa

driver. In QEMU, starting from commit cad2ccc395 ("block/blkio: use

qemu_open() to support fd passing for virtio-blk") we are using

`blkio_get_int(..., "fd")` to check if the "fd" property is supported

for all the virtio-blk-* driver.

Unfortunately that property is also available for those driver that do

not support it, such as virtio-blk-vhost-user.

So, `blkio_get_int()` is not enough to check whether the driver supports

the `fd` property or not. This is because the virito-blk common libblkio

driver only checks whether or not `fd` is set during `blkio_connect()`

and fails with -EINVAL for those transports that do not support it

(all except vhost-vdpa for now).

So let's handle the `blkio_connect()` failure, retrying it using `path`

directly.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")

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

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-id: 20230727161020.84213-3-sgarzare@redhat.com

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

---

block/blkio.c | 29 +++++++++++++++++++++++++++++

1 file changed, 29 insertions(+)

diff --git a/block/blkio.c b/block/blkio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/blkio.c

+++ b/block/blkio.c

@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,

}

ret = blkio_connect(s->blkio);

+ /*

+ * If the libblkio driver doesn't support the `fd` property, blkio_connect()

+ * will fail with -EINVAL. So let's try calling blkio_connect() again by

+ * directly setting `path`.

+ */

+ if (fd_supported && ret == -EINVAL) {

+ qemu_close(fd);

+

+ /*

+ * We need to clear the `fd` property we set previously by setting

+ * it to -1.

+ */

+ ret = blkio_set_int(s->blkio, "fd", -1);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "failed to set fd: %s",

+ blkio_get_error_msg());

+ return ret;

+ }

+

+ ret = blkio_set_str(s->blkio, "path", path);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "failed to set path: %s",

+ blkio_get_error_msg());

+ return ret;

+ }

+

+ ret = blkio_connect(s->blkio);

+ }

+

if (ret < 0) {

error_setg_errno(errp, -ret, "blkio_connect failed: %s",

blkio_get_error_msg());

--

2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

qemu_open() fails if called with an unix domain socket in this way:

-blockdev node-name=drive0,driver=virtio-blk-vhost-user,path=vhost-user-blk.sock,cache.direct=on: Could not open 'vhost-user-blk.sock': No such device or address

Since virtio-blk-vhost-user does not support fd passing, let`s always fall back

on using `path` if we fail the fd passing.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")

Reported-by: Qing Wang <qinwang@redhat.com>

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-id: 20230727161020.84213-4-sgarzare@redhat.com

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

---

block/blkio.c | 20 ++++++++++----------

1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/blkio.c

+++ b/block/blkio.c

@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,

* In order to open the device read-only, we are using the `read-only`

* property of the libblkio driver in blkio_file_open().

*/

- fd = qemu_open(path, O_RDWR, errp);

+ fd = qemu_open(path, O_RDWR, NULL);

if (fd < 0) {

- return -EINVAL;

+ fd_supported = false;

+ } else {

+ ret = blkio_set_int(s->blkio, "fd", fd);

+ if (ret < 0) {

+ fd_supported = false;

+ qemu_close(fd);

+ }

}

+ }

- ret = blkio_set_int(s->blkio, "fd", fd);

- if (ret < 0) {

- error_setg_errno(errp, -ret, "failed to set fd: %s",

- blkio_get_error_msg());

- qemu_close(fd);

- return ret;

- }

- } else {

+ if (!fd_supported) {

ret = blkio_set_str(s->blkio, "path", path);

if (ret < 0) {

error_setg_errno(errp, -ret, "failed to set path: %s",

--

2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

Setting the `fd` property fails with virtio-blk-* libblkio drivers

that do not support fd passing since

https://gitlab.com/libblkio/libblkio/-/merge_requests/208.

Getting the `fd` property, on the other hand, always succeeds for

virtio-blk-* libblkio drivers even when they don't support fd passing.

This patch switches to setting the `fd` property because it is a

better mechanism for probing fd passing support than getting the `fd`

property.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-id: 20230727161020.84213-5-sgarzare@redhat.com

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

---

block/blkio.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blkio.c b/block/blkio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/blkio.c

+++ b/block/blkio.c

@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,

return -EINVAL;

}

- if (blkio_get_int(s->blkio, "fd", &fd) == 0) {

+ if (blkio_set_int(s->blkio, "fd", -1) == 0) {

fd_supported = true;

}

--

2.41.0