Series comparison

-[Qemu-devel] [PULL for-4.0 0/1] Block patches
+[PULL for-6.1 0/3] Block patches
-The following changes since commit 3f3bbfc7cef4490c5ed5550766a81e7d18f08db1:
+The following changes since commit 3521ade3510eb5cefb2e27a101667f25dad89935:
-  Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2019-03-12' into staging (2019-03-12 21:06:26 +0000)
+  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:
-  git://github.com/stefanha/qemu.git tags/block-pull-request
+  https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to f357fcd890a8d6ced6d261338b859a41414561e9:
+for you to fetch changes up to cc8eecd7f105a1dff5876adeb238a14696061a4a:
-  file-posix: add drop-cache=on|off option (2019-03-13 10:54:55 +0000)
+  MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver (2021-07-29 17:17:34 +0100)
 ----------------------------------------------------------------
 Pull request
- * Add 'drop-cache=on|off' option to file-posix.c.  The default is on.
+The main fix here is for io_uring. Spurious -EAGAIN errors can happen and the
-   Disabling the option fixes a QEMU 3.0.0 performance regression when live
+request needs to be resubmitted.
-   migrating on the same host with cache.direct=off.
 The MAINTAINERS changes carry no risk and we might as well include them in QEMU
 .1.
 ----------------------------------------------------------------
-Stefan Hajnoczi (1):
+Fabian Ebner (1):
-  file-posix: add drop-cache=on|off option
+  block/io_uring: resubmit when result is -EAGAIN
- qapi/block-core.json |  6 ++++++
+Philippe Mathieu-Daudé (1):
- block/file-posix.c   | 16 ++++++++++++++++
+  MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver
-files changed, 22 insertions(+)
 Stefano Garzarella (1):
   MAINTAINERS: add Stefano Garzarella as io_uring reviewer
  MAINTAINERS      |  2 ++
  block/io_uring.c | 16 +++++++++++++++-
 files changed, 17 insertions(+), 1 deletion(-)
 --
-.20.1
+.31.1

-New patch
+[PULL for-6.1 1/3] MAINTAINERS: add Stefano Garzarella as io_uring reviewer
+From: Stefano Garzarella <sgarzare@redhat.com>
+I've been working with io_uring for a while so I'd like to help
+with reviews.
+Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
+Message-Id: <20210728131515.131045-1-sgarzare@redhat.com>
+Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+---
+ MAINTAINERS | 1 +
+file changed, 1 insertion(+)
+diff --git a/MAINTAINERS b/MAINTAINERS
+index XXXXXXX..XXXXXXX 100644
+--- a/MAINTAINERS
++++ b/MAINTAINERS
+@@ -XXX,XX +XXX,XX @@ Linux io_uring
+ M: Aarushi Mehta <mehta.aaru20@gmail.com>
+ M: Julia Suvorova <jusual@redhat.com>
+ M: Stefan Hajnoczi <stefanha@redhat.com>
++R: Stefano Garzarella <sgarzare@redhat.com>
+ L: qemu-block@nongnu.org
+ S: Maintained
+ F: block/io_uring.c
+--
+.31.1

-New patch
+[PULL for-6.1 2/3] block/io_uring: resubmit when result is -EAGAIN
+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

-[Qemu-devel] [PULL for-4.0 1/1] file-posix: add drop-cache=on|off option
+[PULL for-6.1 3/3] MAINTAINERS: Added myself as a reviewer for the NVMe Block Driver
-Commit dd577a26ff03b6829721b1ffbbf9e7c411b72378 ("block/file-posix:
+From: Philippe Mathieu-Daudé <philmd@redhat.com>
 implement bdrv_co_invalidate_cache() on Linux") introduced page cache
 invalidation so that cache.direct=off live migration is safe on Linux.
-The invalidation takes a significant amount of time when the file is
+I'm interested in following the activity around the NVMe bdrv.
 large and present in the page cache.  Normally this is not the case for
 cross-host live migration but it can happen when migrating between QEMU
 processes on the same host.
-On same-host migration we don't need to invalidate pages for correctness
+Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
-anyway, so an option to skip page cache invalidation is useful.  I
+Message-id: 20210728183340.2018313-1-philmd@redhat.com
 investigated optimizing invalidation and detecting same-host migration,
 but both are hard to achieve so a user-visible option will suffice.
 As a bonus this option means that the cache invalidation feature will
 now be detectable by libvirt via QMP schema introspection.
 Suggested-by: Neil Skrypuch <neil@tembosocial.com>
 Tested-by: Neil Skrypuch <neil@tembosocial.com>
 Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
 Reviewed-by: Eric Blake <eblake@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 Message-id: 20190307164941.3322-1-stefanha@redhat.com
 Message-Id: <20190307164941.3322-1-stefanha@redhat.com>
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
- qapi/block-core.json |  6 ++++++
+ MAINTAINERS | 1 +
- block/file-posix.c   | 16 ++++++++++++++++
+file changed, 1 insertion(+)
 files changed, 22 insertions(+)
-diff --git a/qapi/block-core.json b/qapi/block-core.json
+diff --git a/MAINTAINERS b/MAINTAINERS
 index XXXXXXX..XXXXXXX 100644
---- a/qapi/block-core.json
+--- a/MAINTAINERS
-+++ b/qapi/block-core.json
++++ b/MAINTAINERS
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ F: block/null.c
- # @locking:     whether to enable file locking. If set to 'auto', only enable
+ NVMe Block Driver
- #               when Open File Descriptor (OFD) locking API is available
+ M: Stefan Hajnoczi <stefanha@redhat.com>
- #               (default: auto, since 2.10)
+ R: Fam Zheng <fam@euphon.net>
-+# @drop-cache:  invalidate page cache during live migration.  This prevents
++R: Philippe Mathieu-Daudé <philmd@redhat.com>
-+#               stale data on the migration destination with cache.direct=off.
+ L: qemu-block@nongnu.org
-+#               Currently only supported on Linux hosts.
+ S: Supported
-+#               (default: on, since: 4.0)
+ F: block/nvme*
  # @x-check-cache-dropped: whether to check that page cache was dropped on live
  #                         migration.  May cause noticeable delays if the image
  #                         file is large, do not use in production.
@@ -XXX,XX +XXX,XX @@
              '*pr-manager': 'str',
              '*locking': 'OnOffAuto',
              '*aio': 'BlockdevAioOptions',
 +        '*drop-cache': {'type': 'bool',
 +                        'if': 'defined(CONFIG_LINUX)'},
              '*x-check-cache-dropped': 'bool' } }
  ##
 diff --git a/block/file-posix.c b/block/file-posix.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/file-posix.c
 +++ b/block/file-posix.c
@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState {
      bool page_cache_inconsistent:1;
      bool has_fallocate;
      bool needs_alignment;
 +    bool drop_cache;
      bool check_cache_dropped;
      PRManager *pr_mgr;
@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState {
  typedef struct BDRVRawReopenState {
      int fd;
      int open_flags;
 +    bool drop_cache;
      bool check_cache_dropped;
  } BDRVRawReopenState;
@@ -XXX,XX +XXX,XX @@ static QemuOptsList raw_runtime_opts = {
              .type = QEMU_OPT_STRING,
              .help = "id of persistent reservation manager object (default: none)",
          },
 +#if defined(__linux__)
 +        {
 +            .name = "drop-cache",
 +            .type = QEMU_OPT_BOOL,
 +            .help = "invalidate page cache during live migration (default: on)",
 +        },
 +#endif
          {
              .name = "x-check-cache-dropped",
              .type = QEMU_OPT_BOOL,
@@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
          }
      }
 +    s->drop_cache = qemu_opt_get_bool(opts, "drop-cache", true);
      s->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
                                                 false);
@@ -XXX,XX +XXX,XX @@ static int raw_reopen_prepare(BDRVReopenState *state,
          goto out;
      }
 +    rs->drop_cache = qemu_opt_get_bool_del(opts, "drop-cache", true);
      rs->check_cache_dropped =
          qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
@@ -XXX,XX +XXX,XX @@ static void raw_reopen_commit(BDRVReopenState *state)
      BDRVRawState *s = state->bs->opaque;
      Error *local_err = NULL;
 +    s->drop_cache = rs->drop_cache;
      s->check_cache_dropped = rs->check_cache_dropped;
      s->open_flags = rs->open_flags;
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
          return;
      }
 +    if (!s->drop_cache) {
 +        return;
 +    }
 +
      if (s->open_flags & O_DIRECT) {
          return; /* No host kernel page cache */
      }
 --
-.20.1
+.31.1

Commit dd577a26ff03b6829721b1ffbbf9e7c411b72378 ("block/file-posix:
implement bdrv_co_invalidate_cache() on Linux") introduced page cache
invalidation so that cache.direct=off live migration is safe on Linux.

The invalidation takes a significant amount of time when the file is
large and present in the page cache.  Normally this is not the case for
cross-host live migration but it can happen when migrating between QEMU
processes on the same host.

On same-host migration we don't need to invalidate pages for correctness
anyway, so an option to skip page cache invalidation is useful.  I
investigated optimizing invalidation and detecting same-host migration,
but both are hard to achieve so a user-visible option will suffice.

As a bonus this option means that the cache invalidation feature will
now be detectable by libvirt via QMP schema introspection.

Suggested-by: Neil Skrypuch <neil@tembosocial.com>
Tested-by: Neil Skrypuch <neil@tembosocial.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20190307164941.3322-1-stefanha@redhat.com
Message-Id: <20190307164941.3322-1-stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/block-core.json |  6 ++++++
 block/file-posix.c   | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index XXXXXXX..XXXXXXX 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -XXX,XX +XXX,XX @@
 # @locking:     whether to enable file locking. If set to 'auto', only enable
 #               when Open File Descriptor (OFD) locking API is available
 #               (default: auto, since 2.10)
+# @drop-cache:  invalidate page cache during live migration.  This prevents
+#               stale data on the migration destination with cache.direct=off.
+#               Currently only supported on Linux hosts.
+#               (default: on, since: 4.0)
 # @x-check-cache-dropped: whether to check that page cache was dropped on live
 #                         migration.  May cause noticeable delays if the image
 #                         file is large, do not use in production.
@@ -XXX,XX +XXX,XX @@
             '*pr-manager': 'str',
             '*locking': 'OnOffAuto',
             '*aio': 'BlockdevAioOptions',
+	    '*drop-cache': {'type': 'bool',
+	                    'if': 'defined(CONFIG_LINUX)'},
             '*x-check-cache-dropped': 'bool' } }
 
 ##
diff --git a/block/file-posix.c b/block/file-posix.c
index XXXXXXX..XXXXXXX 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState {
     bool page_cache_inconsistent:1;
     bool has_fallocate;
     bool needs_alignment;
+    bool drop_cache;
     bool check_cache_dropped;
 
     PRManager *pr_mgr;
@@ -XXX,XX +XXX,XX @@ typedef struct BDRVRawState {
 typedef struct BDRVRawReopenState {
     int fd;
     int open_flags;
+    bool drop_cache;
     bool check_cache_dropped;
 } BDRVRawReopenState;
 
@@ -XXX,XX +XXX,XX @@ static QemuOptsList raw_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "id of persistent reservation manager object (default: none)",
         },
+#if defined(__linux__)
+        {
+            .name = "drop-cache",
+            .type = QEMU_OPT_BOOL,
+            .help = "invalidate page cache during live migration (default: on)",
+        },
+#endif
         {
             .name = "x-check-cache-dropped",
             .type = QEMU_OPT_BOOL,
@@ -XXX,XX +XXX,XX @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         }
     }
 
+    s->drop_cache = qemu_opt_get_bool(opts, "drop-cache", true);
     s->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
                                                false);
 
@@ -XXX,XX +XXX,XX @@ static int raw_reopen_prepare(BDRVReopenState *state,
         goto out;
     }
 
+    rs->drop_cache = qemu_opt_get_bool_del(opts, "drop-cache", true);
     rs->check_cache_dropped =
         qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
 
@@ -XXX,XX +XXX,XX @@ static void raw_reopen_commit(BDRVReopenState *state)
     BDRVRawState *s = state->bs->opaque;
     Error *local_err = NULL;
 
+    s->drop_cache = rs->drop_cache;
     s->check_cache_dropped = rs->check_cache_dropped;
     s->open_flags = rs->open_flags;
 
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn raw_co_invalidate_cache(BlockDriverState *bs,
         return;
     }
 
+    if (!s->drop_cache) {
+        return;
+    }
+
     if (s->open_flags & O_DIRECT) {
         return; /* No host kernel page cache */
     }
-- 
2.20.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