Series comparison

-[PULL for-6.1 0/3] Block patches
+[PULL for-8.1 0/2] Block patches
-The following changes since commit 3521ade3510eb5cefb2e27a101667f25dad89935:
+The following changes since commit 9ba37026fcf6b7f3f096c0cca3e1e7307802486b:
-  Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-07-29' into staging (2021-07-29 13:17:20 +0100)
+  Update version for v8.1.0-rc2 release (2023-08-02 08:22:45 -0700)
 are available in the Git repository at:
   https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to cc8eecd7f105a1dff5876adeb238a14696061a4a:
+for you to fetch changes up to 9b06d0d076271d76e5384d767ef94a676f0a9efd:
-  MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver (2021-07-29 17:17:34 +0100)
+  block/blkio: add more comments on the fd passing handling (2023-08-03 11:28:43 -0400)
 ----------------------------------------------------------------
 Pull request
-The main fix here is for io_uring. Spurious -EAGAIN errors can happen and the
+Fix for an fd leak in the blkio block driver.
 request needs to be resubmitted.
 The MAINTAINERS changes carry no risk and we might as well include them in QEMU
 .1.
 ----------------------------------------------------------------
-Fabian Ebner (1):
+Stefano Garzarella (2):
-  block/io_uring: resubmit when result is -EAGAIN
+  block/blkio: close the fd when blkio_connect() fails
   block/blkio: add more comments on the fd passing handling
-Philippe Mathieu-Daudé (1):
+ block/blkio.c | 28 +++++++++++++++++++++-------
-  MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver
+file changed, 21 insertions(+), 7 deletions(-)
 Stefano Garzarella (1):
   MAINTAINERS: add Stefano Garzarella as io_uring reviewer
  MAINTAINERS      |  2 ++
  block/io_uring.c | 16 +++++++++++++++-
 files changed, 17 insertions(+), 1 deletion(-)
 --
-.31.1
+.41.0

-[PULL for-6.1 3/3] MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver
+[PULL for-8.1 1/2] block/blkio: close the fd when blkio_connect() fails
-From: Philippe Mathieu-Daudé <philmd@redhat.com>
+From: Stefano Garzarella <sgarzare@redhat.com>
-I'm interested in following the activity around the NVMe bdrv.
+libblkio drivers take ownership of `fd` only after a successful
 blkio_connect(), so if it fails, we are still the owners.
-Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
+Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
-Message-id: 20210728183340.2018313-1-philmd@redhat.com
+Suggested-by: Hanna Czenczek <hreitz@redhat.com>
 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
 Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
 Message-id: 20230803082825.25293-2-sgarzare@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- MAINTAINERS | 1 +
+ block/blkio.c | 11 ++++++++---
-file changed, 1 insertion(+)
+file changed, 8 insertions(+), 3 deletions(-)
-diff --git a/MAINTAINERS b/MAINTAINERS
+diff --git a/block/blkio.c b/block/blkio.c
 index XXXXXXX..XXXXXXX 100644
---- a/MAINTAINERS
+--- a/block/blkio.c
-+++ b/MAINTAINERS
++++ b/block/blkio.c
-@@ -XXX,XX +XXX,XX @@ F: block/null.c
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
- NVMe Block Driver
+     const char *path = qdict_get_try_str(options, "path");
- M: Stefan Hajnoczi <stefanha@redhat.com>
+     BDRVBlkioState *s = bs->opaque;
- R: Fam Zheng <fam@euphon.net>
+     bool fd_supported = false;
-+R: Philippe Mathieu-Daudé <philmd@redhat.com>
+-    int fd, ret;
- L: qemu-block@nongnu.org
++    int fd = -1, ret;
- S: Supported
- F: block/nvme*
+     if (!path) {
          error_setg(errp, "missing 'path' option");
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
              if (ret < 0) {
                  fd_supported = false;
                  qemu_close(fd);
 +                fd = -1;
              }
          }
      }
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
      }
      ret = blkio_connect(s->blkio);
 +    if (ret < 0 && fd >= 0) {
 +        /* Failed to give the FD to libblkio, close it */
 +        qemu_close(fd);
 +        fd = -1;
 +    }
 +
      /*
       * 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.
 --
-.31.1
+.41.0

-[PULL for-6.1 1/3] MAINTAINERS: add Stefano Garzarella as io_uring reviewer
+[PULL for-8.1 2/2] block/blkio: add more comments on the fd passing handling
 From: Stefano Garzarella <sgarzare@redhat.com>
-I've been working with io_uring for a while so I'd like to help
+As Hanna pointed out, it is not clear in the code why qemu_open()
-with reviews.
+can fail, and why blkio_set_int("fd") is not enough to discover
 the `fd` property support.
+Let's fix them by adding more details in the code comments.
+Suggested-by: Hanna Czenczek <hreitz@redhat.com>
+Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
 Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
-Message-Id: <20210728131515.131045-1-sgarzare@redhat.com>
+Message-id: 20230803082825.25293-3-sgarzare@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- MAINTAINERS | 1 +
+ block/blkio.c | 15 ++++++++++++---
-file changed, 1 insertion(+)
+file changed, 12 insertions(+), 3 deletions(-)
-diff --git a/MAINTAINERS b/MAINTAINERS
+diff --git a/block/blkio.c b/block/blkio.c
 index XXXXXXX..XXXXXXX 100644
---- a/MAINTAINERS
+--- a/block/blkio.c
-+++ b/MAINTAINERS
++++ b/block/blkio.c
-@@ -XXX,XX +XXX,XX @@ Linux io_uring
+@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
- M: Aarushi Mehta <mehta.aaru20@gmail.com>
+          */
- M: Julia Suvorova <jusual@redhat.com>
+         fd = qemu_open(path, O_RDWR, NULL);
- M: Stefan Hajnoczi <stefanha@redhat.com>
+         if (fd < 0) {
-+R: Stefano Garzarella <sgarzare@redhat.com>
++            /*
- L: qemu-block@nongnu.org
++             * qemu_open() can fail if the user specifies a path that is not
- S: Maintained
++             * a file or device, for example in the case of Unix Domain Socket
- F: block/io_uring.c
++             * for the virtio-blk-vhost-user driver. In such cases let's have
 +             * libblkio open the path directly.
 +             */
              fd_supported = false;
          } else {
              ret = blkio_set_int(s->blkio, "fd", fd);
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
      }
      /*
 -     * 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`.
 +     * Before https://gitlab.com/libblkio/libblkio/-/merge_requests/208
 +     * (libblkio <= v1.3.0), setting the `fd` property is not enough to check
 +     * whether the driver supports the `fd` property or not. In that case,
 +     * blkio_connect() will fail with -EINVAL.
 +     * So let's try calling blkio_connect() again by directly setting `path`
 +     * to cover this scenario.
       */
      if (fd_supported && ret == -EINVAL) {
          /*
 --
-.31.1
+.41.0

-[PULL for-6.1 2/3] block/io_uring: resubmit when result is -EAGAIN
+Deleted patch
-From: Fabian Ebner <f.ebner@proxmox.com>
-Linux SCSI can throw spurious -EAGAIN in some corner cases in its
-completion path, which will end up being the result in the completed
-io_uring request.
-Resubmitting such requests should allow block jobs to complete, even
-if such spurious errors are encountered.
-Co-authored-by: Stefan Hajnoczi <stefanha@gmail.com>
-Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
-Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
-Message-id: 20210729091029.65369-1-f.ebner@proxmox.com
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
----
- block/io_uring.c | 16 +++++++++++++++-
-file changed, 15 insertions(+), 1 deletion(-)
-diff --git a/block/io_uring.c b/block/io_uring.c
-index XXXXXXX..XXXXXXX 100644
---- a/block/io_uring.c
-+++ b/block/io_uring.c
-@@ -XXX,XX +XXX,XX @@ static void luring_process_completions(LuringState *s)
-         total_bytes = ret + luringcb->total_read;
-         if (ret < 0) {
--            if (ret == -EINTR) {
-+            /*
-+             * Only writev/readv/fsync requests on regular files or host block
-+             * devices are submitted. Therefore -EAGAIN is not expected but it's
-+             * known to happen sometimes with Linux SCSI. Submit again and hope
-+             * the request completes successfully.
-+             *
-+             * For more information, see:
-+             * https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u
-+             *
-+             * If the code is changed to submit other types of requests in the
-+             * future, then this workaround may need to be extended to deal with
-+             * genuine -EAGAIN results that should not be resubmitted
-+             * immediately.
-+             */
-+            if (ret == -EINTR || ret == -EAGAIN) {
-                 luring_resubmit(s, luringcb);
-                 continue;
-             }
---
-.31.1

The following changes since commit 3521ade3510eb5cefb2e27a101667f25dad89935:

Merge remote-tracking branch 'remotes/thuth-gitlab/tags/pull-request-2021-07-29' into staging (2021-07-29 13:17:20 +0100)

are available in the Git repository at:

https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to cc8eecd7f105a1dff5876adeb238a14696061a4a:

MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver (2021-07-29 17:17:34 +0100)

----------------------------------------------------------------
Pull request

The main fix here is for io_uring. Spurious -EAGAIN errors can happen and the
request needs to be resubmitted.

The MAINTAINERS changes carry no risk and we might as well include them in QEMU
6.1.

----------------------------------------------------------------

Fabian Ebner (1):
  block/io_uring: resubmit when result is -EAGAIN

Philippe Mathieu-Daudé (1):
  MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver

Stefano Garzarella (1):
  MAINTAINERS: add Stefano Garzarella as io_uring reviewer

MAINTAINERS      |  2 ++
 block/io_uring.c | 16 +++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.31.1

From: Fabian Ebner <f.ebner@proxmox.com>

Linux SCSI can throw spurious -EAGAIN in some corner cases in its
completion path, which will end up being the result in the completed
io_uring request.

Resubmitting such requests should allow block jobs to complete, even
if such spurious errors are encountered.

Co-authored-by: Stefan Hajnoczi <stefanha@gmail.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
Message-id: 20210729091029.65369-1-f.ebner@proxmox.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io_uring.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/io_uring.c b/block/io_uring.c
index XXXXXXX..XXXXXXX 100644
--- a/block/io_uring.c
+++ b/block/io_uring.c
@@ -XXX,XX +XXX,XX @@ static void luring_process_completions(LuringState *s)
         total_bytes = ret + luringcb->total_read;
 
         if (ret < 0) {
-            if (ret == -EINTR) {
+            /*
+             * Only writev/readv/fsync requests on regular files or host block
+             * devices are submitted. Therefore -EAGAIN is not expected but it's
+             * known to happen sometimes with Linux SCSI. Submit again and hope
+             * the request completes successfully.
+             *
+             * For more information, see:
+             * https://lore.kernel.org/io-uring/20210727165811.284510-3-axboe@kernel.dk/T/#u
+             *
+             * If the code is changed to submit other types of requests in the
+             * future, then this workaround may need to be extended to deal with
+             * genuine -EAGAIN results that should not be resubmitted
+             * immediately.
+             */
+            if (ret == -EINTR || ret == -EAGAIN) {
                 luring_resubmit(s, luringcb);
                 continue;
             }
-- 
2.31.1

From: Stefano Garzarella <sgarzare@redhat.com>

libblkio drivers take ownership of `fd` only after a successful
blkio_connect(), so if it fails, we are still the owners.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for virtio-blk")
Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Message-id: 20230803082825.25293-2-sgarzare@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/blkio.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 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,
     const char *path = qdict_get_try_str(options, "path");
     BDRVBlkioState *s = bs->opaque;
     bool fd_supported = false;
-    int fd, ret;
+    int fd = -1, ret;
 
     if (!path) {
         error_setg(errp, "missing 'path' option");
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
             if (ret < 0) {
                 fd_supported = false;
                 qemu_close(fd);
+                fd = -1;
             }
         }
     }
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
     }
 
     ret = blkio_connect(s->blkio);
+    if (ret < 0 && fd >= 0) {
+        /* Failed to give the FD to libblkio, close it */
+        qemu_close(fd);
+        fd = -1;
+    }
+
     /*
      * 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.
-- 
2.41.0

From: Stefano Garzarella <sgarzare@redhat.com>

As Hanna pointed out, it is not clear in the code why qemu_open()
can fail, and why blkio_set_int("fd") is not enough to discover
the `fd` property support.

Let's fix them by adding more details in the code comments.

Suggested-by: Hanna Czenczek <hreitz@redhat.com>
Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Message-id: 20230803082825.25293-3-sgarzare@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/blkio.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 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,
          */
         fd = qemu_open(path, O_RDWR, NULL);
         if (fd < 0) {
+            /*
+             * qemu_open() can fail if the user specifies a path that is not
+             * a file or device, for example in the case of Unix Domain Socket
+             * for the virtio-blk-vhost-user driver. In such cases let's have
+             * libblkio open the path directly.
+             */
             fd_supported = false;
         } else {
             ret = blkio_set_int(s->blkio, "fd", fd);
@@ -XXX,XX +XXX,XX @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, QDict *options,
     }
 
     /*
-     * 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`.
+     * Before https://gitlab.com/libblkio/libblkio/-/merge_requests/208
+     * (libblkio <= v1.3.0), setting the `fd` property is not enough to check
+     * whether the driver supports the `fd` property or not. In that case,
+     * blkio_connect() will fail with -EINVAL.
+     * So let's try calling blkio_connect() again by directly setting `path`
+     * to cover this scenario.
      */
     if (fd_supported && ret == -EINVAL) {
         /*
-- 
2.41.0