[PATCH v2 0/4] block/blkio: fix opening virtio-blk drivers

[PATCH v2 0/4] block/blkio: fix opening virtio-blk drivers

[Stefano Garzarella]

There is a problem with virtio-blk-vhost-vdpa.
The first patch does some preparation changes. The second and third patches
fix the issues, the last patch tries to prepare QEMU for a future version of
libblkio where we can use blkio_set_fd() to check whether the property is
supported or not.

While testing, I realized that the main problem was that qemu_open() does not
support UDS, but still the problem with blkio_connect() which can fail remains.

v2:
- added first patch in preparation of the others
- reworked patch 2 retrying blkio_connect [Stefan]
- added patch 3 since qemu_open() fails in UDS
- changed patch 4 commit description [Stefan]

v1: https://lore.kernel.org/qemu-devel/20230724154611.178858-1-sgarzare@redhat.com/

Based on stefanha/block branch.

Stefano Garzarella (4):
  block/blkio: move blkio_connect() in the drivers functions
  block/blkio: retry blkio_connect() if it fails using `fd`
  block/blkio: fall back on using `path` when `fd` setting fails
  block/blkio: use blkio_set_int("fd") to check fd support

 block/blkio.c | 108 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 75 insertions(+), 33 deletions(-)

-- 
2.41.0

[PATCH v2 1/4] block/blkio: move blkio_connect() in the drivers functions

[Stefano Garzarella]

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>
---
 block/blkio.c | 67 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 7eb1b94820..8ad7c0b575 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -603,8 +603,8 @@ 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;
@@ -627,11 +627,18 @@ 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;
@@ -655,11 +662,18 @@ 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;
@@ -718,6 +732,13 @@ 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;
@@ -737,24 +758,6 @@ static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
-    if (strcmp(blkio_driver, "io_uring") == 0) {
-        ret = blkio_io_uring_open(bs, options, flags, errp);
-    } else if (strcmp(blkio_driver, "nvme-io_uring") == 0) {
-        ret = blkio_nvme_io_uring(bs, options, flags, errp);
-    } else if (strcmp(blkio_driver, "virtio-blk-vfio-pci") == 0) {
-        ret = blkio_virtio_blk_common_open(bs, options, flags, errp);
-    } else if (strcmp(blkio_driver, "virtio-blk-vhost-user") == 0) {
-        ret = blkio_virtio_blk_common_open(bs, options, flags, errp);
-    } else if (strcmp(blkio_driver, "virtio-blk-vhost-vdpa") == 0) {
-        ret = blkio_virtio_blk_common_open(bs, options, flags, errp);
-    } else {
-        g_assert_not_reached();
-    }
-    if (ret < 0) {
-        blkio_destroy(&s->blkio);
-        return ret;
-    }
-
     if (!(flags & BDRV_O_RDWR)) {
         ret = blkio_set_bool(s->blkio, "read-only", true);
         if (ret < 0) {
@@ -765,10 +768,20 @@ static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    ret = blkio_connect(s->blkio);
+    if (strcmp(blkio_driver, "io_uring") == 0) {
+        ret = blkio_io_uring_connect(bs, options, flags, errp);
+    } else if (strcmp(blkio_driver, "nvme-io_uring") == 0) {
+        ret = blkio_nvme_io_uring_connect(bs, options, flags, errp);
+    } else if (strcmp(blkio_driver, "virtio-blk-vfio-pci") == 0) {
+        ret = blkio_virtio_blk_connect(bs, options, flags, errp);
+    } else if (strcmp(blkio_driver, "virtio-blk-vhost-user") == 0) {
+        ret = blkio_virtio_blk_connect(bs, options, flags, errp);
+    } else if (strcmp(blkio_driver, "virtio-blk-vhost-vdpa") == 0) {
+        ret = blkio_virtio_blk_connect(bs, options, flags, errp);
+    } else {
+        g_assert_not_reached();
+    }
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "blkio_connect failed: %s",
-                         blkio_get_error_msg());
         blkio_destroy(&s->blkio);
         return ret;
     }
-- 
2.41.0

[PATCH v2 2/4] block/blkio: retry blkio_connect() if it fails using `fd`

[Stefano Garzarella]

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>
---

Notes:
    v2:
    - reworked retrying blkio_connect() [Stefan]

 block/blkio.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/block/blkio.c b/block/blkio.c
index 8ad7c0b575..60d2d0f129 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -733,6 +733,35 @@ 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

[PATCH v2 3/4] block/blkio: fall back on using `path` when `fd` setting fails

[Stefano Garzarella]

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>
---
 block/blkio.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 60d2d0f129..72b46d61fd 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -711,19 +711,19 @@ 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

[PATCH v2 4/4] block/blkio: use blkio_set_int("fd") to check fd support

[Stefano Garzarella]

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>
---

Notes:
    v2:
    - changed commit description [Stefan]

 block/blkio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blkio.c b/block/blkio.c
index 72b46d61fd..8e7ce42c79 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -690,7 +690,7 @@ 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

Re: [PATCH v2 0/4] block/blkio: fix opening virtio-blk drivers

[Stefan Hajnoczi]

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

On Thu, Jul 27, 2023 at 06:10:16PM +0200, Stefano Garzarella wrote:
> There is a problem with virtio-blk-vhost-vdpa.
> The first patch does some preparation changes. The second and third patches
> fix the issues, the last patch tries to prepare QEMU for a future version of
> libblkio where we can use blkio_set_fd() to check whether the property is
> supported or not.
> 
> While testing, I realized that the main problem was that qemu_open() does not
> support UDS, but still the problem with blkio_connect() which can fail remains.
> 
> v2:
> - added first patch in preparation of the others
> - reworked patch 2 retrying blkio_connect [Stefan]
> - added patch 3 since qemu_open() fails in UDS
> - changed patch 4 commit description [Stefan]
> 
> v1: https://lore.kernel.org/qemu-devel/20230724154611.178858-1-sgarzare@redhat.com/
> 
> Based on stefanha/block branch.
> 
> Stefano Garzarella (4):
>   block/blkio: move blkio_connect() in the drivers functions
>   block/blkio: retry blkio_connect() if it fails using `fd`
>   block/blkio: fall back on using `path` when `fd` setting fails
>   block/blkio: use blkio_set_int("fd") to check fd support
> 
>  block/blkio.c | 108 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 75 insertions(+), 33 deletions(-)
> 
> -- 
> 2.41.0
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan

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