[PULL 0/8] Block layer patches

[PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:

  Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:

  iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)

----------------------------------------------------------------
Block layer patches:

- Fix crash on write to read-only devices
- iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
  non-numeric test case names

----------------------------------------------------------------
Kevin Wolf (2):
      block: Separate blk_is_writable() and blk_supports_write_perm()
      virtio-scsi-test: Test writing to scsi-cd device

Vladimir Sementsov-Ogievskiy (6):
      iotests: 146: drop extra whitespaces from .out file
      iotests: add findtests.py
      iotests: add testenv.py
      iotests: add testrunner.py
      iotests: rewrite check into python
      iotests: rename and move 169 and 199 tests

 docs/devel/testing.rst                             |   50 +-
 include/sysemu/block-backend.h                     |    3 +-
 block/block-backend.c                              |   19 +-
 hw/block/dataplane/xen-block.c                     |    2 +-
 hw/block/fdc.c                                     |    9 +-
 hw/block/m25p80.c                                  |    6 +-
 hw/block/nand.c                                    |    2 +-
 hw/block/nvme-ns.c                                 |    7 +-
 hw/block/onenand.c                                 |    2 +-
 hw/block/pflash_cfi01.c                            |    2 +-
 hw/block/pflash_cfi02.c                            |    2 +-
 hw/block/swim.c                                    |    6 +-
 hw/block/virtio-blk.c                              |    6 +-
 hw/block/xen-block.c                               |    2 +-
 hw/ide/core.c                                      |    2 +-
 hw/misc/sifive_u_otp.c                             |    2 +-
 hw/ppc/pnv_pnor.c                                  |    2 +-
 hw/scsi/scsi-disk.c                                |   10 +-
 hw/scsi/scsi-generic.c                             |    4 +-
 hw/sd/sd.c                                         |    6 +-
 hw/usb/dev-storage.c                               |    4 +-
 tests/qtest/virtio-scsi-test.c                     |   39 +
 tests/qemu-iotests/findtests.py                    |  159 +++
 tests/qemu-iotests/iotests.py                      |    8 +
 tests/qemu-iotests/testenv.py                      |  281 +++++
 tests/qemu-iotests/testrunner.py                   |  367 +++++++
 Makefile                                           |    1 -
 tests/check-block.sh                               |    3 +-
 tests/qemu-iotests/146.out                         |  780 +++++++-------
 tests/qemu-iotests/check                           | 1095 +++-----------------
 tests/qemu-iotests/common.env.in                   |    3 -
 tests/qemu-iotests/group                           |  323 ------
 tests/qemu-iotests/meson.build                     |    3 -
 .../{199 => tests/migrate-bitmaps-postcopy-test}   |    0
 .../migrate-bitmaps-postcopy-test.out}             |    0
 .../{169 => tests/migrate-bitmaps-test}            |    0
 .../{169.out => tests/migrate-bitmaps-test.out}    |    0
 37 files changed, 1481 insertions(+), 1729 deletions(-)
 create mode 100644 tests/qemu-iotests/findtests.py
 create mode 100644 tests/qemu-iotests/testenv.py
 create mode 100644 tests/qemu-iotests/testrunner.py
 delete mode 100644 tests/qemu-iotests/common.env.in
 delete mode 100644 tests/qemu-iotests/group
 rename tests/qemu-iotests/{199 => tests/migrate-bitmaps-postcopy-test} (100%)
 rename tests/qemu-iotests/{199.out => tests/migrate-bitmaps-postcopy-test.out} (100%)
 rename tests/qemu-iotests/{169 => tests/migrate-bitmaps-test} (100%)
 rename tests/qemu-iotests/{169.out => tests/migrate-bitmaps-test.out} (100%)

Re: [PULL 0/8] Block layer patches

[Peter Maydell]

On Wed, 27 Jan 2021 at 19:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:
>
>   Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:
>
>   iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Fix crash on write to read-only devices
> - iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
>   non-numeric test case names



Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM

Re: [PULL 0/8] Block layer patches

[Peter Maydell]

On Wed, 27 Jan 2021 at 19:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:
>
>   Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:
>
>   iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Fix crash on write to read-only devices
> - iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
>   non-numeric test case names
>
> ----------------------------------------------------------------

I somehow failed to notice before applying this, but this
breaks 'make check' on the netbsd and freebsd VMs.

As usual the build log is pretty opaque, but this looks like the
probable culprit:

env: python3: No such file or directory
gmake: *** [/home/qemu/qemu-test.nU2bcG/src/tests/Makefile.include:144:
check-block] Error 1

The python in the netbsd VM is /usr/pkg/bin/python3.7. Something
seems to be ignoring the --python= passed into our configure
and assuming that "python3" is always a valid executable.

thanks
-- PMM

Re: [PULL 0/8] Block layer patches

[Vladimir Sementsov-Ogievskiy]

28.01.2021 21:19, Peter Maydell wrote:
> On Wed, 27 Jan 2021 at 19:58, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> The following changes since commit bf159f0bdc7b8e7aa8342dedb3829ca744c1b612:
>>
>>    Merge remote-tracking branch 'remotes/edgar/tags/edgar/xilinx-next-2021-01-27.for-upstream' into staging (2021-01-27 17:40:25 +0000)
>>
>> are available in the Git repository at:
>>
>>    git://repo.or.cz/qemu/kevin.git tags/for-upstream
>>
>> for you to fetch changes up to a44be0334beae3a9affb4a3a92cc6852993d7a84:
>>
>>    iotests: rename and move 169 and 199 tests (2021-01-27 20:53:14 +0100)
>>
>> ----------------------------------------------------------------
>> Block layer patches:
>>
>> - Fix crash on write to read-only devices
>> - iotests: Rewrite 'check' in Python, get rid of 'groups' and allow
>>    non-numeric test case names
>>
>> ----------------------------------------------------------------
> 
> I somehow failed to notice before applying this, but this
> breaks 'make check' on the netbsd and freebsd VMs.
> 
> As usual the build log is pretty opaque, but this looks like the
> probable culprit:
> 
> env: python3: No such file or directory
> gmake: *** [/home/qemu/qemu-test.nU2bcG/src/tests/Makefile.include:144:
> check-block] Error 1
> 
> The python in the netbsd VM is /usr/pkg/bin/python3.7. Something
> seems to be ignoring the --python= passed into our configure
> and assuming that "python3" is always a valid executable.
> 

Seems, that's shows that the fact that "PYTHON" variable in check-block.sh works for me doesn't mean that it works everywhere.. My fault, I look closer tomorrow.

-- 
Best regards,
Vladimir

[PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit 80e8f0602168f451a93e71cbb1d59e93d745e62e:

  Merge tag 'bsd-user-misc-2024q2-pull-request' of gitlab.com:bsdimp/qemu into staging (2024-06-09 11:21:55 -0700)

are available in the Git repository at:

  https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 3ab0f063e58ed9224237d69c4211ca83335164c4:

  crypto/block: drop qcrypto_block_open() n_threads argument (2024-06-10 11:05:43 +0200)

----------------------------------------------------------------
Block layer patches

- crypto: Fix crash when used with multiqueue devices
- linux-aio: add IO_CMD_FDSYNC command support
- copy-before-write: Avoid integer overflows for timeout > 4s
- Fix crash with QMP block_resize and iothreads
- qemu-io: add cvtnum() error handling for zone commands
- Code cleanup

----------------------------------------------------------------
Denis V. Lunev via (1):
      block: drop force_dup parameter of raw_reconfigure_getfd()

Fiona Ebner (1):
      block/copy-before-write: use uint64_t for timeout in nanoseconds

Prasad J Pandit (1):
      linux-aio: add IO_CMD_FDSYNC command support

Stefan Hajnoczi (5):
      Revert "monitor: use aio_co_reschedule_self()"
      aio: warn about iohandler_ctx special casing
      qemu-io: add cvtnum() error handling for zone commands
      block/crypto: create ciphers on demand
      crypto/block: drop qcrypto_block_open() n_threads argument

 crypto/blockpriv.h             |  13 +++--
 include/block/aio.h            |   6 +++
 include/block/raw-aio.h        |   1 +
 include/crypto/block.h         |   2 -
 block/copy-before-write.c      |   2 +-
 block/crypto.c                 |   1 -
 block/file-posix.c             |  17 ++++--
 block/linux-aio.c              |  21 +++++++-
 block/qcow.c                   |   2 +-
 block/qcow2.c                  |   5 +-
 crypto/block-luks.c            |   4 +-
 crypto/block-qcow.c            |   8 ++-
 crypto/block.c                 | 114 ++++++++++++++++++++++++-----------------
 qapi/qmp-dispatch.c            |   7 ++-
 qemu-io-cmds.c                 |  48 ++++++++++++++++-
 tests/unit/test-crypto-block.c |   4 --
 16 files changed, 176 insertions(+), 79 deletions(-)

[PULL 1/8] block: drop force_dup parameter of raw_reconfigure_getfd()

[Kevin Wolf]

From: "Denis V. Lunev via" <qemu-block@nongnu.org>

Since commit 72373e40fbc, this parameter is always passed as 'false'
from the caller.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
Message-ID: <20240430170213.148558-1-den@openvz.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..5c46938936 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1039,8 +1039,7 @@ static int fcntl_setfl(int fd, int flag)
 }
 
 static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
-                                 int *open_flags, uint64_t perm, bool force_dup,
-                                 Error **errp)
+                                 int *open_flags, uint64_t perm, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int fd = -1;
@@ -1068,7 +1067,7 @@ static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
     assert((s->open_flags & O_ASYNC) == 0);
 #endif
 
-    if (!force_dup && *open_flags == s->open_flags) {
+    if (*open_flags == s->open_flags) {
         /* We're lucky, the existing fd is fine */
         return s->fd;
     }
@@ -3748,8 +3747,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
     int ret;
 
     /* We may need a new fd if auto-read-only switches the mode */
-    ret = raw_reconfigure_getfd(bs, input_flags, &open_flags, perm,
-                                false, errp);
+    ret = raw_reconfigure_getfd(bs, input_flags, &open_flags, perm, errp);
     if (ret < 0) {
         return ret;
     } else if (ret != s->fd) {
-- 
2.45.2

[PULL 2/8] Revert "monitor: use aio_co_reschedule_self()"

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

Commit 1f25c172f837 ("monitor: use aio_co_reschedule_self()") was a code
cleanup that uses aio_co_reschedule_self() instead of open coding
coroutine rescheduling.

Bug RHEL-34618 was reported and Kevin Wolf <kwolf@redhat.com> identified
the root cause. I missed that aio_co_reschedule_self() ->
qemu_get_current_aio_context() only knows about
qemu_aio_context/IOThread AioContexts and not about iohandler_ctx. It
does not function correctly when going back from the iohandler_ctx to
qemu_aio_context.

Go back to open coding the AioContext transitions to avoid this bug.

This reverts commit 1f25c172f83704e350c0829438d832384084a74d.

Cc: qemu-stable@nongnu.org
Buglink: https://issues.redhat.com/browse/RHEL-34618
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240506190622.56095-2-stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/qmp-dispatch.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index f3488afeef..176b549473 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -212,7 +212,8 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
              * executing the command handler so that it can make progress if it
              * involves an AIO_WAIT_WHILE().
              */
-            aio_co_reschedule_self(qemu_get_aio_context());
+            aio_co_schedule(qemu_get_aio_context(), qemu_coroutine_self());
+            qemu_coroutine_yield();
         }
 
         monitor_set_cur(qemu_coroutine_self(), cur_mon);
@@ -226,7 +227,9 @@ QDict *coroutine_mixed_fn qmp_dispatch(const QmpCommandList *cmds, QObject *requ
              * Move back to iohandler_ctx so that nested event loops for
              * qemu_aio_context don't start new monitor commands.
              */
-            aio_co_reschedule_self(iohandler_get_aio_context());
+            aio_co_schedule(iohandler_get_aio_context(),
+                            qemu_coroutine_self());
+            qemu_coroutine_yield();
         }
     } else {
        /*
-- 
2.45.2

[PULL 3/8] aio: warn about iohandler_ctx special casing

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

The main loop has two AioContexts: qemu_aio_context and iohandler_ctx.
The main loop runs them both, but nested aio_poll() calls on
qemu_aio_context exclude iohandler_ctx.

Which one should qemu_get_current_aio_context() return when called from
the main loop? Document that it's always qemu_aio_context.

This has subtle effects on functions that use
qemu_get_current_aio_context(). For example, aio_co_reschedule_self()
does not work when moving from iohandler_ctx to qemu_aio_context because
qemu_get_current_aio_context() does not differentiate these two
AioContexts.

Document this in order to reduce the chance of future bugs.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240506190622.56095-3-stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/aio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/block/aio.h b/include/block/aio.h
index 8378553eb9..4ee81936ed 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -629,6 +629,9 @@ void aio_co_schedule(AioContext *ctx, Coroutine *co);
  *
  * Move the currently running coroutine to new_ctx. If the coroutine is already
  * running in new_ctx, do nothing.
+ *
+ * Note that this function cannot reschedule from iohandler_ctx to
+ * qemu_aio_context.
  */
 void coroutine_fn aio_co_reschedule_self(AioContext *new_ctx);
 
@@ -661,6 +664,9 @@ void aio_co_enter(AioContext *ctx, Coroutine *co);
  * If called from an IOThread this will be the IOThread's AioContext.  If
  * called from the main thread or with the "big QEMU lock" taken it
  * will be the main loop AioContext.
+ *
+ * Note that the return value is never the main loop's iohandler_ctx and the
+ * return value is the main loop AioContext instead.
  */
 AioContext *qemu_get_current_aio_context(void);
 
-- 
2.45.2

[PULL 4/8] qemu-io: add cvtnum() error handling for zone commands

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

cvtnum() parses positive int64_t values and returns a negative errno on
failure. Print errors and return early when cvtnum() fails.

While we're at it, also reject nr_zones values greater or equal to 2^32
since they cannot be represented.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Sam Li <faithilikerun@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240507180558.377233-1-stefanha@redhat.com>
Reviewed-by: Sam Li <faithilikerun@gmail.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index f5d7202a13..e2fab57183 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1739,12 +1739,26 @@ static int zone_report_f(BlockBackend *blk, int argc, char **argv)
 {
     int ret;
     int64_t offset;
+    int64_t val;
     unsigned int nr_zones;
 
     ++optind;
     offset = cvtnum(argv[optind]);
+    if (offset < 0) {
+        print_cvtnum_err(offset, argv[optind]);
+        return offset;
+    }
     ++optind;
-    nr_zones = cvtnum(argv[optind]);
+    val = cvtnum(argv[optind]);
+    if (val < 0) {
+        print_cvtnum_err(val, argv[optind]);
+        return val;
+    }
+    if (val > UINT_MAX) {
+        printf("Number of zones must be less than 2^32\n");
+        return -ERANGE;
+    }
+    nr_zones = val;
 
     g_autofree BlockZoneDescriptor *zones = NULL;
     zones = g_new(BlockZoneDescriptor, nr_zones);
@@ -1780,8 +1794,16 @@ static int zone_open_f(BlockBackend *blk, int argc, char **argv)
     int64_t offset, len;
     ++optind;
     offset = cvtnum(argv[optind]);
+    if (offset < 0) {
+        print_cvtnum_err(offset, argv[optind]);
+        return offset;
+    }
     ++optind;
     len = cvtnum(argv[optind]);
+    if (len < 0) {
+        print_cvtnum_err(len, argv[optind]);
+        return len;
+    }
     ret = blk_zone_mgmt(blk, BLK_ZO_OPEN, offset, len);
     if (ret < 0) {
         printf("zone open failed: %s\n", strerror(-ret));
@@ -1805,8 +1827,16 @@ static int zone_close_f(BlockBackend *blk, int argc, char **argv)
     int64_t offset, len;
     ++optind;
     offset = cvtnum(argv[optind]);
+    if (offset < 0) {
+        print_cvtnum_err(offset, argv[optind]);
+        return offset;
+    }
     ++optind;
     len = cvtnum(argv[optind]);
+    if (len < 0) {
+        print_cvtnum_err(len, argv[optind]);
+        return len;
+    }
     ret = blk_zone_mgmt(blk, BLK_ZO_CLOSE, offset, len);
     if (ret < 0) {
         printf("zone close failed: %s\n", strerror(-ret));
@@ -1830,8 +1860,16 @@ static int zone_finish_f(BlockBackend *blk, int argc, char **argv)
     int64_t offset, len;
     ++optind;
     offset = cvtnum(argv[optind]);
+    if (offset < 0) {
+        print_cvtnum_err(offset, argv[optind]);
+        return offset;
+    }
     ++optind;
     len = cvtnum(argv[optind]);
+    if (len < 0) {
+        print_cvtnum_err(len, argv[optind]);
+        return len;
+    }
     ret = blk_zone_mgmt(blk, BLK_ZO_FINISH, offset, len);
     if (ret < 0) {
         printf("zone finish failed: %s\n", strerror(-ret));
@@ -1855,8 +1893,16 @@ static int zone_reset_f(BlockBackend *blk, int argc, char **argv)
     int64_t offset, len;
     ++optind;
     offset = cvtnum(argv[optind]);
+    if (offset < 0) {
+        print_cvtnum_err(offset, argv[optind]);
+        return offset;
+    }
     ++optind;
     len = cvtnum(argv[optind]);
+    if (len < 0) {
+        print_cvtnum_err(len, argv[optind]);
+        return len;
+    }
     ret = blk_zone_mgmt(blk, BLK_ZO_RESET, offset, len);
     if (ret < 0) {
         printf("zone reset failed: %s\n", strerror(-ret));
-- 
2.45.2

[PULL 5/8] block/copy-before-write: use uint64_t for timeout in nanoseconds

[Kevin Wolf]

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

rather than the uint32_t for which the maximum is slightly more than 4
seconds and larger values would overflow. The QAPI interface allows
specifying the number of seconds, so only values 0 to 4 are safe right
now, other values lead to a much lower timeout than a user expects.

The block_copy() call where this is used already takes a uint64_t for
the timeout, so no change required there.

Fixes: 6db7fd1ca9 ("block/copy-before-write: implement cbw-timeout option")
Reported-by: Friedrich Weber <f.weber@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Message-ID: <20240429141934.442154-1-f.ebner@proxmox.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/copy-before-write.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index cd65524e26..853e01a1eb 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -43,7 +43,7 @@ typedef struct BDRVCopyBeforeWriteState {
     BlockCopyState *bcs;
     BdrvChild *target;
     OnCbwError on_cbw_error;
-    uint32_t cbw_timeout_ns;
+    uint64_t cbw_timeout_ns;
     bool discard_source;
 
     /*
-- 
2.45.2

[PULL 6/8] linux-aio: add IO_CMD_FDSYNC command support

[Kevin Wolf]

From: Prasad Pandit <pjp@fedoraproject.org>

Libaio defines IO_CMD_FDSYNC command to sync all outstanding
asynchronous I/O operations, by flushing out file data to the
disk storage. Enable linux-aio to submit such aio request.

When using aio=native without fdsync() support, QEMU creates
pthreads, and destroying these pthreads results in TLB flushes.
In a real-time guest environment, TLB flushes cause a latency
spike. This patch helps to avoid such spikes.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
Message-ID: <20240425070412.37248-1-ppandit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/raw-aio.h |  1 +
 block/file-posix.c      |  9 +++++++++
 block/linux-aio.c       | 21 ++++++++++++++++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 20e000b8ef..626706827f 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -60,6 +60,7 @@ void laio_cleanup(LinuxAioState *s);
 int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
                                 int type, uint64_t dev_max_batch);
 
+bool laio_has_fdsync(int);
 void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);
 void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);
 #endif
diff --git a/block/file-posix.c b/block/file-posix.c
index 5c46938936..be25e35ff6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -159,6 +159,7 @@ typedef struct BDRVRawState {
     bool has_discard:1;
     bool has_write_zeroes:1;
     bool use_linux_aio:1;
+    bool has_laio_fdsync:1;
     bool use_linux_io_uring:1;
     int page_cache_inconsistent; /* errno from fdatasync failure */
     bool has_fallocate;
@@ -718,6 +719,9 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         ret = -EINVAL;
         goto fail;
     }
+    if (s->use_linux_aio) {
+        s->has_laio_fdsync = laio_has_fdsync(s->fd);
+    }
 #else
     if (s->use_linux_aio) {
         error_setg(errp, "aio=native was specified, but is not supported "
@@ -2598,6 +2602,11 @@ static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs)
     if (raw_check_linux_io_uring(s)) {
         return luring_co_submit(bs, s->fd, 0, NULL, QEMU_AIO_FLUSH);
     }
+#endif
+#ifdef CONFIG_LINUX_AIO
+    if (s->has_laio_fdsync && raw_check_linux_aio(s)) {
+        return laio_co_submit(s->fd, 0, NULL, QEMU_AIO_FLUSH, 0);
+    }
 #endif
     return raw_thread_pool_submit(handle_aiocb_flush, &acb);
 }
diff --git a/block/linux-aio.c b/block/linux-aio.c
index ec05d946f3..e3b5ec9aba 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -384,6 +384,9 @@ static int laio_do_submit(int fd, struct qemu_laiocb *laiocb, off_t offset,
     case QEMU_AIO_READ:
         io_prep_preadv(iocbs, fd, qiov->iov, qiov->niov, offset);
         break;
+    case QEMU_AIO_FLUSH:
+        io_prep_fdsync(iocbs, fd);
+        break;
     /* Currently Linux kernel does not support other operations */
     default:
         fprintf(stderr, "%s: invalid AIO request type 0x%x.\n",
@@ -412,7 +415,7 @@ int coroutine_fn laio_co_submit(int fd, uint64_t offset, QEMUIOVector *qiov,
     AioContext *ctx = qemu_get_current_aio_context();
     struct qemu_laiocb laiocb = {
         .co         = qemu_coroutine_self(),
-        .nbytes     = qiov->size,
+        .nbytes     = qiov ? qiov->size : 0,
         .ctx        = aio_get_linux_aio(ctx),
         .ret        = -EINPROGRESS,
         .is_read    = (type == QEMU_AIO_READ),
@@ -486,3 +489,19 @@ void laio_cleanup(LinuxAioState *s)
     }
     g_free(s);
 }
+
+bool laio_has_fdsync(int fd)
+{
+    struct iocb cb;
+    struct iocb *cbs[] = {&cb, NULL};
+
+    io_context_t ctx = 0;
+    io_setup(1, &ctx);
+
+    /* check if host kernel supports IO_CMD_FDSYNC */
+    io_prep_fdsync(&cb, fd);
+    int ret = io_submit(ctx, 1, cbs);
+
+    io_destroy(ctx);
+    return (ret == -EINVAL) ? false : true;
+}
-- 
2.45.2

[PULL 7/8] block/crypto: create ciphers on demand

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

Ciphers are pre-allocated by qcrypto_block_init_cipher() depending on
the given number of threads. The -device
virtio-blk-pci,iothread-vq-mapping= feature allows users to assign
multiple IOThreads to a virtio-blk device, but the association between
the virtio-blk device and the block driver happens after the block
driver is already open.

When the number of threads given to qcrypto_block_init_cipher() is
smaller than the actual number of threads at runtime, the
block->n_free_ciphers > 0 assertion in qcrypto_block_pop_cipher() can
fail.

Get rid of qcrypto_block_init_cipher() n_thread's argument and allocate
ciphers on demand.

Reported-by: Qing Wang <qinwang@redhat.com>
Buglink: https://issues.redhat.com/browse/RHEL-36159
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240527155851.892885-2-stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 crypto/blockpriv.h  |  12 +++--
 crypto/block-luks.c |   3 +-
 crypto/block-qcow.c |   2 +-
 crypto/block.c      | 111 ++++++++++++++++++++++++++------------------
 4 files changed, 78 insertions(+), 50 deletions(-)

diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 836f3b4726..4bf6043d5d 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -32,8 +32,14 @@ struct QCryptoBlock {
     const QCryptoBlockDriver *driver;
     void *opaque;
 
-    QCryptoCipher **ciphers;
-    size_t n_ciphers;
+    /* Cipher parameters */
+    QCryptoCipherAlgorithm alg;
+    QCryptoCipherMode mode;
+    uint8_t *key;
+    size_t nkey;
+
+    QCryptoCipher **free_ciphers;
+    size_t max_free_ciphers;
     size_t n_free_ciphers;
     QCryptoIVGen *ivgen;
     QemuMutex mutex;
@@ -130,7 +136,7 @@ int qcrypto_block_init_cipher(QCryptoBlock *block,
                               QCryptoCipherAlgorithm alg,
                               QCryptoCipherMode mode,
                               const uint8_t *key, size_t nkey,
-                              size_t n_threads, Error **errp);
+                              Error **errp);
 
 void qcrypto_block_free_cipher(QCryptoBlock *block);
 
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3ee928fb5a..3357852c0a 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1262,7 +1262,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
                                       luks->cipher_mode,
                                       masterkey,
                                       luks->header.master_key_len,
-                                      n_threads,
                                       errp) < 0) {
             goto fail;
         }
@@ -1456,7 +1455,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     /* Setup the block device payload encryption objects */
     if (qcrypto_block_init_cipher(block, luks_opts.cipher_alg,
                                   luks_opts.cipher_mode, masterkey,
-                                  luks->header.master_key_len, 1, errp) < 0) {
+                                  luks->header.master_key_len, errp) < 0) {
         goto error;
     }
 
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 4d7cf36a8f..02305058e3 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -75,7 +75,7 @@ qcrypto_block_qcow_init(QCryptoBlock *block,
     ret = qcrypto_block_init_cipher(block, QCRYPTO_CIPHER_ALG_AES_128,
                                     QCRYPTO_CIPHER_MODE_CBC,
                                     keybuf, G_N_ELEMENTS(keybuf),
-                                    n_threads, errp);
+                                    errp);
     if (ret < 0) {
         ret = -ENOTSUP;
         goto fail;
diff --git a/crypto/block.c b/crypto/block.c
index 506ea1d1a3..ba6d1cebc7 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/lockable.h"
 #include "blockpriv.h"
 #include "block-qcow.h"
 #include "block-luks.h"
@@ -57,6 +58,8 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
 {
     QCryptoBlock *block = g_new0(QCryptoBlock, 1);
 
+    qemu_mutex_init(&block->mutex);
+
     block->format = options->format;
 
     if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -76,8 +79,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
         return NULL;
     }
 
-    qemu_mutex_init(&block->mutex);
-
     return block;
 }
 
@@ -92,6 +93,8 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
 {
     QCryptoBlock *block = g_new0(QCryptoBlock, 1);
 
+    qemu_mutex_init(&block->mutex);
+
     block->format = options->format;
 
     if (options->format >= G_N_ELEMENTS(qcrypto_block_drivers) ||
@@ -111,8 +114,6 @@ QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options,
         return NULL;
     }
 
-    qemu_mutex_init(&block->mutex);
-
     return block;
 }
 
@@ -227,37 +228,42 @@ QCryptoCipher *qcrypto_block_get_cipher(QCryptoBlock *block)
      * This function is used only in test with one thread (it's safe to skip
      * pop/push interface), so it's enough to assert it here:
      */
-    assert(block->n_ciphers <= 1);
-    return block->ciphers ? block->ciphers[0] : NULL;
+    assert(block->max_free_ciphers <= 1);
+    return block->free_ciphers ? block->free_ciphers[0] : NULL;
 }
 
 
-static QCryptoCipher *qcrypto_block_pop_cipher(QCryptoBlock *block)
+static QCryptoCipher *qcrypto_block_pop_cipher(QCryptoBlock *block,
+                                               Error **errp)
 {
-    QCryptoCipher *cipher;
-
-    qemu_mutex_lock(&block->mutex);
-
-    assert(block->n_free_ciphers > 0);
-    block->n_free_ciphers--;
-    cipher = block->ciphers[block->n_free_ciphers];
-
-    qemu_mutex_unlock(&block->mutex);
+    /* Usually there is a free cipher available */
+    WITH_QEMU_LOCK_GUARD(&block->mutex) {
+        if (block->n_free_ciphers > 0) {
+            block->n_free_ciphers--;
+            return block->free_ciphers[block->n_free_ciphers];
+        }
+    }
 
-    return cipher;
+    /* Otherwise allocate a new cipher */
+    return qcrypto_cipher_new(block->alg, block->mode, block->key,
+                              block->nkey, errp);
 }
 
 
 static void qcrypto_block_push_cipher(QCryptoBlock *block,
                                       QCryptoCipher *cipher)
 {
-    qemu_mutex_lock(&block->mutex);
+    QEMU_LOCK_GUARD(&block->mutex);
 
-    assert(block->n_free_ciphers < block->n_ciphers);
-    block->ciphers[block->n_free_ciphers] = cipher;
-    block->n_free_ciphers++;
+    if (block->n_free_ciphers == block->max_free_ciphers) {
+        block->max_free_ciphers++;
+        block->free_ciphers = g_renew(QCryptoCipher *,
+                                      block->free_ciphers,
+                                      block->max_free_ciphers);
+    }
 
-    qemu_mutex_unlock(&block->mutex);
+    block->free_ciphers[block->n_free_ciphers] = cipher;
+    block->n_free_ciphers++;
 }
 
 
@@ -265,24 +271,31 @@ int qcrypto_block_init_cipher(QCryptoBlock *block,
                               QCryptoCipherAlgorithm alg,
                               QCryptoCipherMode mode,
                               const uint8_t *key, size_t nkey,
-                              size_t n_threads, Error **errp)
+                              Error **errp)
 {
-    size_t i;
+    QCryptoCipher *cipher;
 
-    assert(!block->ciphers && !block->n_ciphers && !block->n_free_ciphers);
+    assert(!block->free_ciphers && !block->max_free_ciphers &&
+           !block->n_free_ciphers);
 
-    block->ciphers = g_new0(QCryptoCipher *, n_threads);
+    /* Stash away cipher parameters for qcrypto_block_pop_cipher() */
+    block->alg = alg;
+    block->mode = mode;
+    block->key = g_memdup2(key, nkey);
+    block->nkey = nkey;
 
-    for (i = 0; i < n_threads; i++) {
-        block->ciphers[i] = qcrypto_cipher_new(alg, mode, key, nkey, errp);
-        if (!block->ciphers[i]) {
-            qcrypto_block_free_cipher(block);
-            return -1;
-        }
-        block->n_ciphers++;
-        block->n_free_ciphers++;
+    /*
+     * Create a new cipher to validate the parameters now. This reduces the
+     * chance of cipher creation failing at I/O time.
+     */
+    cipher = qcrypto_block_pop_cipher(block, errp);
+    if (!cipher) {
+        g_free(block->key);
+        block->key = NULL;
+        return -1;
     }
 
+    qcrypto_block_push_cipher(block, cipher);
     return 0;
 }
 
@@ -291,19 +304,23 @@ void qcrypto_block_free_cipher(QCryptoBlock *block)
 {
     size_t i;
 
-    if (!block->ciphers) {
+    g_free(block->key);
+    block->key = NULL;
+
+    if (!block->free_ciphers) {
         return;
     }
 
-    assert(block->n_ciphers == block->n_free_ciphers);
+    /* All popped ciphers were eventually pushed back */
+    assert(block->n_free_ciphers == block->max_free_ciphers);
 
-    for (i = 0; i < block->n_ciphers; i++) {
-        qcrypto_cipher_free(block->ciphers[i]);
+    for (i = 0; i < block->max_free_ciphers; i++) {
+        qcrypto_cipher_free(block->free_ciphers[i]);
     }
 
-    g_free(block->ciphers);
-    block->ciphers = NULL;
-    block->n_ciphers = block->n_free_ciphers = 0;
+    g_free(block->free_ciphers);
+    block->free_ciphers = NULL;
+    block->max_free_ciphers = block->n_free_ciphers = 0;
 }
 
 QCryptoIVGen *qcrypto_block_get_ivgen(QCryptoBlock *block)
@@ -311,7 +328,7 @@ QCryptoIVGen *qcrypto_block_get_ivgen(QCryptoBlock *block)
     /* ivgen should be accessed under mutex. However, this function is used only
      * in test with one thread, so it's enough to assert it here:
      */
-    assert(block->n_ciphers <= 1);
+    assert(block->max_free_ciphers <= 1);
     return block->ivgen;
 }
 
@@ -446,7 +463,10 @@ int qcrypto_block_decrypt_helper(QCryptoBlock *block,
                                  Error **errp)
 {
     int ret;
-    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block);
+    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block, errp);
+    if (!cipher) {
+        return -1;
+    }
 
     ret = do_qcrypto_block_cipher_encdec(cipher, block->niv, block->ivgen,
                                          &block->mutex, sectorsize, offset, buf,
@@ -465,7 +485,10 @@ int qcrypto_block_encrypt_helper(QCryptoBlock *block,
                                  Error **errp)
 {
     int ret;
-    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block);
+    QCryptoCipher *cipher = qcrypto_block_pop_cipher(block, errp);
+    if (!cipher) {
+        return -1;
+    }
 
     ret = do_qcrypto_block_cipher_encdec(cipher, block->niv, block->ivgen,
                                          &block->mutex, sectorsize, offset, buf,
-- 
2.45.2

[PULL 8/8] crypto/block: drop qcrypto_block_open() n_threads argument

[Kevin Wolf]

From: Stefan Hajnoczi <stefanha@redhat.com>

The n_threads argument is no longer used since the previous commit.
Remove it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20240527155851.892885-3-stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Acked-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 crypto/blockpriv.h             | 1 -
 include/crypto/block.h         | 2 --
 block/crypto.c                 | 1 -
 block/qcow.c                   | 2 +-
 block/qcow2.c                  | 5 ++---
 crypto/block-luks.c            | 1 -
 crypto/block-qcow.c            | 6 ++----
 crypto/block.c                 | 3 +--
 tests/unit/test-crypto-block.c | 4 ----
 9 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/crypto/blockpriv.h b/crypto/blockpriv.h
index 4bf6043d5d..b8f77cb5eb 100644
--- a/crypto/blockpriv.h
+++ b/crypto/blockpriv.h
@@ -59,7 +59,6 @@ struct QCryptoBlockDriver {
                 QCryptoBlockReadFunc readfunc,
                 void *opaque,
                 unsigned int flags,
-                size_t n_threads,
                 Error **errp);
 
     int (*create)(QCryptoBlock *block,
diff --git a/include/crypto/block.h b/include/crypto/block.h
index 92e823c9f2..5b5d039800 100644
--- a/include/crypto/block.h
+++ b/include/crypto/block.h
@@ -76,7 +76,6 @@ typedef enum {
  * @readfunc: callback for reading data from the volume
  * @opaque: data to pass to @readfunc
  * @flags: bitmask of QCryptoBlockOpenFlags values
- * @n_threads: allow concurrent I/O from up to @n_threads threads
  * @errp: pointer to a NULL-initialized error object
  *
  * Create a new block encryption object for an existing
@@ -113,7 +112,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
                                  QCryptoBlockReadFunc readfunc,
                                  void *opaque,
                                  unsigned int flags,
-                                 size_t n_threads,
                                  Error **errp);
 
 typedef enum {
diff --git a/block/crypto.c b/block/crypto.c
index 21eed909c1..4eed3ffa6a 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -363,7 +363,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
                                        block_crypto_read_func,
                                        bs,
                                        cflags,
-                                       1,
                                        errp);
 
     if (!crypto->block) {
diff --git a/block/qcow.c b/block/qcow.c
index ca8e1d5ec8..c2f89db055 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -211,7 +211,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
             s->crypto = qcrypto_block_open(crypto_opts, "encrypt.",
-                                           NULL, NULL, cflags, 1, errp);
+                                           NULL, NULL, cflags, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 956128b409..10883a2494 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -321,7 +321,7 @@ qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
                                            qcow2_crypto_hdr_read_func,
-                                           bs, cflags, QCOW2_MAX_THREADS, errp);
+                                           bs, cflags, errp);
             if (!s->crypto) {
                 return -EINVAL;
             }
@@ -1701,8 +1701,7 @@ qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
             s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
-                                           NULL, NULL, cflags,
-                                           QCOW2_MAX_THREADS, errp);
+                                           NULL, NULL, cflags, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3357852c0a..5b777c15d3 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1189,7 +1189,6 @@ qcrypto_block_luks_open(QCryptoBlock *block,
                         QCryptoBlockReadFunc readfunc,
                         void *opaque,
                         unsigned int flags,
-                        size_t n_threads,
                         Error **errp)
 {
     QCryptoBlockLUKS *luks = NULL;
diff --git a/crypto/block-qcow.c b/crypto/block-qcow.c
index 02305058e3..42e9556e42 100644
--- a/crypto/block-qcow.c
+++ b/crypto/block-qcow.c
@@ -44,7 +44,6 @@ qcrypto_block_qcow_has_format(const uint8_t *buf G_GNUC_UNUSED,
 static int
 qcrypto_block_qcow_init(QCryptoBlock *block,
                         const char *keysecret,
-                        size_t n_threads,
                         Error **errp)
 {
     char *password;
@@ -100,7 +99,6 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
                         QCryptoBlockReadFunc readfunc G_GNUC_UNUSED,
                         void *opaque G_GNUC_UNUSED,
                         unsigned int flags,
-                        size_t n_threads,
                         Error **errp)
 {
     if (flags & QCRYPTO_BLOCK_OPEN_NO_IO) {
@@ -115,7 +113,7 @@ qcrypto_block_qcow_open(QCryptoBlock *block,
             return -1;
         }
         return qcrypto_block_qcow_init(block, options->u.qcow.key_secret,
-                                       n_threads, errp);
+                                       errp);
     }
 }
 
@@ -135,7 +133,7 @@ qcrypto_block_qcow_create(QCryptoBlock *block,
         return -1;
     }
     /* QCow2 has no special header, since everything is hardwired */
-    return qcrypto_block_qcow_init(block, options->u.qcow.key_secret, 1, errp);
+    return qcrypto_block_qcow_init(block, options->u.qcow.key_secret, errp);
 }
 
 
diff --git a/crypto/block.c b/crypto/block.c
index ba6d1cebc7..3bcc4270c3 100644
--- a/crypto/block.c
+++ b/crypto/block.c
@@ -53,7 +53,6 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
                                  QCryptoBlockReadFunc readfunc,
                                  void *opaque,
                                  unsigned int flags,
-                                 size_t n_threads,
                                  Error **errp)
 {
     QCryptoBlock *block = g_new0(QCryptoBlock, 1);
@@ -73,7 +72,7 @@ QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options,
     block->driver = qcrypto_block_drivers[options->format];
 
     if (block->driver->open(block, options, optprefix,
-                            readfunc, opaque, flags, n_threads, errp) < 0)
+                            readfunc, opaque, flags, errp) < 0)
     {
         g_free(block);
         return NULL;
diff --git a/tests/unit/test-crypto-block.c b/tests/unit/test-crypto-block.c
index 6cfc817a92..42cfab6067 100644
--- a/tests/unit/test-crypto-block.c
+++ b/tests/unit/test-crypto-block.c
@@ -303,7 +303,6 @@ static void test_block(gconstpointer opaque)
                              test_block_read_func,
                              &header,
                              0,
-                             1,
                              NULL);
     g_assert(blk == NULL);
 
@@ -312,7 +311,6 @@ static void test_block(gconstpointer opaque)
                              test_block_read_func,
                              &header,
                              QCRYPTO_BLOCK_OPEN_NO_IO,
-                             1,
                              &error_abort);
 
     g_assert(qcrypto_block_get_cipher(blk) == NULL);
@@ -327,7 +325,6 @@ static void test_block(gconstpointer opaque)
                              test_block_read_func,
                              &header,
                              0,
-                             1,
                              &error_abort);
     g_assert(blk);
 
@@ -384,7 +381,6 @@ test_luks_bad_header(gconstpointer data)
                              test_block_read_func,
                              &buf,
                              0,
-                             1,
                              &err);
     g_assert(!blk);
     g_assert(err);
-- 
2.45.2

Re: [PULL 0/8] Block layer patches

[Richard Henderson]

On 6/11/24 10:36, Kevin Wolf wrote:
> The following changes since commit 80e8f0602168f451a93e71cbb1d59e93d745e62e:
> 
>    Merge tag 'bsd-user-misc-2024q2-pull-request' of gitlab.com:bsdimp/qemu into staging (2024-06-09 11:21:55 -0700)
> 
> are available in the Git repository at:
> 
>    https://repo.or.cz/qemu/kevin.git  tags/for-upstream
> 
> for you to fetch changes up to 3ab0f063e58ed9224237d69c4211ca83335164c4:
> 
>    crypto/block: drop qcrypto_block_open() n_threads argument (2024-06-10 11:05:43 +0200)
> 
> ----------------------------------------------------------------
> Block layer patches
> 
> - crypto: Fix crash when used with multiqueue devices
> - linux-aio: add IO_CMD_FDSYNC command support
> - copy-before-write: Avoid integer overflows for timeout > 4s
> - Fix crash with QMP block_resize and iothreads
> - qemu-io: add cvtnum() error handling for zone commands
> - Code cleanup

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate.


r~

[PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:

  Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)

are available in the Git repository at:

  https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:

  vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)

----------------------------------------------------------------
Block layer patches

- Fix qmp_device_add() to not throw non-scalar options away (fixes
  iothread-vq-mapping being silently ignored in device_add)
- iotests: Fix mypy failure
- parallels: Avoid potential integer overflow
- Fix crash in migration_is_running()

----------------------------------------------------------------
Dmitry Frolov (1):
      parallels: fix possible int overflow

John Snow (4):
      iotests: reflow ReproducibleTestRunner arguments
      iotests: correct resultclass type in ReproducibleTestRunner
      python: disable too-many-positional-arguments warning
      python: silence pylint raising-non-exception error

Peter Xu (1):
      migration: Check current_migration in migration_is_running()

Stefan Hajnoczi (2):
      qdev-monitor: avoid QemuOpts in QMP device_add
      vl: use qmp_device_add() in qemu_create_cli_devices()

 block/parallels.c             |  4 ++--
 migration/migration.c         |  4 ++++
 system/qdev-monitor.c         | 42 ++++++++++++++++++++++++++++--------------
 system/vl.c                   | 14 ++++----------
 python/scripts/mkvenv.py      |  3 +++
 tests/qemu-iotests/iotests.py | 11 +++++++----
 python/setup.cfg              |  1 +
 tests/qemu-iotests/pylintrc   |  1 +
 8 files changed, 50 insertions(+), 30 deletions(-)

Re: [PULL 0/8] Block layer patches

[Peter Maydell]

On Thu, 14 Nov 2024 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
>
>   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:
>
>   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Fix qmp_device_add() to not throw non-scalar options away (fixes
>   iothread-vq-mapping being silently ignored in device_add)
> - iotests: Fix mypy failure
> - parallels: Avoid potential integer overflow
> - Fix crash in migration_is_running()
>

Hi; this seems to cause an error for the avocado test
tests/avocado/hotplug_blk.py:HotPlug.test

https://gitlab.com/qemu-project/qemu/-/jobs/8387009365
https://gitlab.com/qemu-project/qemu/-/jobs/8387009383

(12/51) tests/avocado/hotplug_blk.py:HotPlug.test: STARTED
(12/51) tests/avocado/hotplug_blk.py:HotPlug.test: ERROR: Could not
perform graceful shutdown (17.16 s)

If you dig through the build artefacts you can find the debug log:
https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log

and it seems like the test sends a device_add command over
QMP and the result is that QEMU dies with an assertion failure.
The relevant device_add is

[stdlog]   "execute": "device_add",
[stdlog]   "arguments": {
[stdlog]     "driver": "virtio-blk-pci",
[stdlog]     "drive": "disk",
[stdlog]     "id": "virtio-disk0",
[stdlog]     "bus": "pci.1",
[stdlog]     "addr": 1
[stdlog]   },
[stdlog]   "id": "__qmp#00002"
[stdlog] }

Avocado helpfully hides the assertion message under a rock
in a different log file:
https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/7f00b63ed810.log

qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143:
qobject_input_try_get_object: Assertion `removed' failed.


I'm guessing this is Stefan's patches since they touch
the device_add path.

thanks
-- PMM

Re: [PULL 0/8] Block layer patches

[Kevin Wolf]

Am 15.11.2024 um 21:16 hat Peter Maydell geschrieben:
> On Thu, 14 Nov 2024 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> >
> >   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> >
> > are available in the Git repository at:
> >
> >   https://repo.or.cz/qemu/kevin.git tags/for-upstream
> >
> > for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:
> >
> >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)
> >
> > ----------------------------------------------------------------
> > Block layer patches
> >
> > - Fix qmp_device_add() to not throw non-scalar options away (fixes
> >   iothread-vq-mapping being silently ignored in device_add)
> > - iotests: Fix mypy failure
> > - parallels: Avoid potential integer overflow
> > - Fix crash in migration_is_running()
> >
> 
> Hi; this seems to cause an error for the avocado test
> tests/avocado/hotplug_blk.py:HotPlug.test
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/8387009365
> https://gitlab.com/qemu-project/qemu/-/jobs/8387009383
> 
> (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: STARTED
> (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: ERROR: Could not
> perform graceful shutdown (17.16 s)
> 
> If you dig through the build artefacts you can find the debug log:
> https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> 
> and it seems like the test sends a device_add command over
> QMP and the result is that QEMU dies with an assertion failure.
> The relevant device_add is
> 
> [stdlog]   "execute": "device_add",
> [stdlog]   "arguments": {
> [stdlog]     "driver": "virtio-blk-pci",
> [stdlog]     "drive": "disk",
> [stdlog]     "id": "virtio-disk0",
> [stdlog]     "bus": "pci.1",
> [stdlog]     "addr": 1
> [stdlog]   },
> [stdlog]   "id": "__qmp#00002"
> [stdlog] }
> 
> Avocado helpfully hides the assertion message under a rock
> in a different log file:
> https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/7f00b63ed810.log
> 
> qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143:
> qobject_input_try_get_object: Assertion `removed' failed.
> 
> 
> I'm guessing this is Stefan's patches since they touch
> the device_add path.

Yes, this is Stefan's patches exposing a preexisting bug on a new code
path. You can already trigger the same bug on the command line with git
master:

$ ./qemu-system-x86_64 -blockdev null-co,node-name=disk -device '{ "driver": "virtio-blk-pci", "drive": "disk", "id": "virtio-disk0", "addr": 1 }'
qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143: QObject *qobject_input_try_get_object(QObjectInputVisitor *, const char *, _Bool): Assertion `removed' failed.

The problem is that set_pci_devfn() visits the same field twice, which
is not allowed. Apparently the QemuOpts visitor accepts it anyway, but
the QObject one doesn't. I'll write a patch to use the proper alternate
mechanism instead, that should fix it.

Kevin

Re: [PULL 0/8] Block layer patches

[Stefan Hajnoczi]

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

On Tue, Nov 19, 2024 at 12:25:26PM +0100, Kevin Wolf wrote:
> Am 15.11.2024 um 21:16 hat Peter Maydell geschrieben:
> > On Thu, 14 Nov 2024 at 16:58, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > The following changes since commit f0a5a31c33a8109061c2493e475c8a2f4d022432:
> > >
> > >   Update version for v9.2.0-rc0 release (2024-11-13 21:44:45 +0000)
> > >
> > > are available in the Git repository at:
> > >
> > >   https://repo.or.cz/qemu/kevin.git tags/for-upstream
> > >
> > > for you to fetch changes up to 378a645b2f6125b1bdbd1fae3e8f30452d5b5934:
> > >
> > >   vl: use qmp_device_add() in qemu_create_cli_devices() (2024-11-14 17:55:51 +0100)
> > >
> > > ----------------------------------------------------------------
> > > Block layer patches
> > >
> > > - Fix qmp_device_add() to not throw non-scalar options away (fixes
> > >   iothread-vq-mapping being silently ignored in device_add)
> > > - iotests: Fix mypy failure
> > > - parallels: Avoid potential integer overflow
> > > - Fix crash in migration_is_running()
> > >
> > 
> > Hi; this seems to cause an error for the avocado test
> > tests/avocado/hotplug_blk.py:HotPlug.test
> > 
> > https://gitlab.com/qemu-project/qemu/-/jobs/8387009365
> > https://gitlab.com/qemu-project/qemu/-/jobs/8387009383
> > 
> > (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: STARTED
> > (12/51) tests/avocado/hotplug_blk.py:HotPlug.test: ERROR: Could not
> > perform graceful shutdown (17.16 s)
> > 
> > If you dig through the build artefacts you can find the debug log:
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/debug.log
> > 
> > and it seems like the test sends a device_add command over
> > QMP and the result is that QEMU dies with an assertion failure.
> > The relevant device_add is
> > 
> > [stdlog]   "execute": "device_add",
> > [stdlog]   "arguments": {
> > [stdlog]     "driver": "virtio-blk-pci",
> > [stdlog]     "drive": "disk",
> > [stdlog]     "id": "virtio-disk0",
> > [stdlog]     "bus": "pci.1",
> > [stdlog]     "addr": 1
> > [stdlog]   },
> > [stdlog]   "id": "__qmp#00002"
> > [stdlog] }
> > 
> > Avocado helpfully hides the assertion message under a rock
> > in a different log file:
> > https://qemu-project.gitlab.io/-/qemu/-/jobs/8387009383/artifacts/build/tests/results/latest/test-results/12-tests_avocado_hotplug_blk.py_HotPlug.test/7f00b63ed810.log
> > 
> > qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143:
> > qobject_input_try_get_object: Assertion `removed' failed.
> > 
> > 
> > I'm guessing this is Stefan's patches since they touch
> > the device_add path.
> 
> Yes, this is Stefan's patches exposing a preexisting bug on a new code
> path. You can already trigger the same bug on the command line with git
> master:
> 
> $ ./qemu-system-x86_64 -blockdev null-co,node-name=disk -device '{ "driver": "virtio-blk-pci", "drive": "disk", "id": "virtio-disk0", "addr": 1 }'
> qemu-system-x86_64: ../qapi/qobject-input-visitor.c:143: QObject *qobject_input_try_get_object(QObjectInputVisitor *, const char *, _Bool): Assertion `removed' failed.
> 
> The problem is that set_pci_devfn() visits the same field twice, which
> is not allowed. Apparently the QemuOpts visitor accepts it anyway, but
> the QObject one doesn't. I'll write a patch to use the proper alternate
> mechanism instead, that should fix it.

Thank you, Kevin!

Stefan

> 
> Kevin
> 

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