The following changes since commit 813bac3d8d70d85cb7835f7945eb9eed84c2d8d0:

Merge tag '2023q3-bsd-user-pull-request' of https://gitlab.com/bsdimp/qemu into staging (2023-08-29 08:58:00 -0400)

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

for you to fetch changes up to 87ec6f55af38e29be5b2b65a8acf84da73e06d06:

aio-posix: zero out io_uring sqe user_data (2023-08-30 07:39:59 -0400)

Pull request

v3:

- Drop UFS emulation due to CI failures

- Add "aio-posix: zero out io_uring sqe user_data"

Andrey Drobyshev (3):

block: add subcluster_size field to BlockDriverInfo

block/io: align requests to subcluster_size

tests/qemu-iotests/197: add testcase for CoR with subclusters

Fabiano Rosas (1):

block-migration: Ensure we don't crash during migration cleanup

aio-posix: zero out io_uring sqe user_data

include/block/block-common.h | 5 ++++

include/block/block-io.h | 8 +++---

block.c | 7 +++++

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

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

block/qcow2.c | 1 +

migration/block.c | 11 ++++++--

util/fdmon-io_uring.c | 2 ++

tests/qemu-iotests/197 | 29 +++++++++++++++++++++

tests/qemu-iotests/197.out | 24 +++++++++++++++++

10 files changed, 110 insertions(+), 35 deletions(-)

2.41.0

From: Fabiano Rosas <farosas@suse.de>

We can fail the blk_insert_bs() at init_blk_migration(), leaving the

BlkMigDevState without a dirty_bitmap and BlockDriverState. Account

for the possibly missing elements when doing cleanup.

Fix the following crashes:

Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.

0x0000555555ec83ef in bdrv_release_dirty_bitmap (bitmap=0x0) at ../block/dirty-bitmap.c:359

359 BlockDriverState *bs = bitmap->bs;

#0 0x0000555555ec83ef in bdrv_release_dirty_bitmap (bitmap=0x0) at ../block/dirty-bitmap.c:359

#1 0x0000555555bba331 in unset_dirty_tracking () at ../migration/block.c:371

#2 0x0000555555bbad98 in block_migration_cleanup_bmds () at ../migration/block.c:681

Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.

0x0000555555e971ff in bdrv_op_unblock (bs=0x0, op=BLOCK_OP_TYPE_BACKUP_SOURCE, reason=0x0) at ../block.c:7073

7073 QLIST_FOREACH_SAFE(blocker, &bs->op_blockers[op], list, next) {

#0 0x0000555555e971ff in bdrv_op_unblock (bs=0x0, op=BLOCK_OP_TYPE_BACKUP_SOURCE, reason=0x0) at ../block.c:7073

#1 0x0000555555e9734a in bdrv_op_unblock_all (bs=0x0, reason=0x0) at ../block.c:7095

#2 0x0000555555bbae13 in block_migration_cleanup_bmds () at ../migration/block.c:690

Signed-off-by: Fabiano Rosas <farosas@suse.de>

Message-id: 20230731203338.27581-1-farosas@suse.de

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

migration/block.c | 11 +++++++++--

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

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

--- a/migration/block.c

+++ b/migration/block.c

@@ -XXX,XX +XXX,XX @@ static void unset_dirty_tracking(void)

BlkMigDevState *bmds;

QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {

- bdrv_release_dirty_bitmap(bmds->dirty_bitmap);

+ if (bmds->dirty_bitmap) {

+ bdrv_release_dirty_bitmap(bmds->dirty_bitmap);

+ }

@@ -XXX,XX +XXX,XX @@ static int64_t get_remaining_dirty(void)

static void block_migration_cleanup_bmds(void)

BlkMigDevState *bmds;

+ BlockDriverState *bs;

AioContext *ctx;

unset_dirty_tracking();

while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {

QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);

- bdrv_op_unblock_all(blk_bs(bmds->blk), bmds->blocker);

+ bs = blk_bs(bmds->blk);

+ if (bs) {

+ bdrv_op_unblock_all(bs, bmds->blocker);

+ }

error_free(bmds->blocker);

/* Save ctx, because bmds->blk can disappear during blk_unref. */

2.41.0

From: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

This is going to be used in the subsequent commit as requests alignment

(in particular, during copy-on-read). This value only makes sense for

the formats which support subclusters (currently QCOW2 only). If this

field isn't set by driver's own bdrv_get_info() implementation, we

simply set it equal to the cluster size thus treating each cluster as

having a single subcluster.

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Denis V. Lunev <den@openvz.org>

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Message-ID: <20230711172553.234055-2-andrey.drobyshev@virtuozzo.com>

include/block/block-common.h | 5 +++++

block.c | 7 +++++++

block/qcow2.c | 1 +

3 files changed, 13 insertions(+)

@@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)

memset(bdi, 0, sizeof(*bdi));

ret = drv->bdrv_co_get_info(bs, bdi);

+ if (bdi->subcluster_size == 0) {

+ /*

+ * If the driver left this unset, subclusters are not supported.

+ * Then it is safe to treat each cluster as having only one subcluster.

+ */

+ bdi->subcluster_size = bdi->cluster_size;

+ }

if (ret < 0) {

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

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ qcow2_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)

{

BDRVQcow2State *s = bs->opaque;

bdi->cluster_size = s->cluster_size;

+ bdi->subcluster_size = s->subcluster_size;

bdi->vm_state_offset = qcow2_vm_state_offset(s);

bdi->is_dirty = s->incompatible_features & QCOW2_INCOMPAT_DIRTY;

2.41.0

From: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

When target image is using subclusters, and we align the request during

copy-on-read, it makes sense to align to subcluster_size rather than

cluster_size. Otherwise we end up with unnecessary allocations.

This commit renames bdrv_round_to_clusters() to bdrv_round_to_subclusters()

and utilizes subcluster_size field of BlockDriverInfo to make necessary

alignments. It affects copy-on-read as well as mirror job (which is

using bdrv_round_to_clusters()).

This change also fixes the following bug with failing assert (covered by

the test in the subsequent commit):

qemu-img create -f qcow2 base.qcow2 64K

qemu-img create -f qcow2 -o extended_l2=on,backing_file=base.qcow2,backing_fmt=qcow2 img.qcow2 64K

qemu-io -c "write -P 0xaa 0 2K" img.qcow2

qemu-io -C -c "read -P 0x00 2K 62K" img.qcow2

qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Denis V. Lunev <den@openvz.org>

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Message-ID: <20230711172553.234055-3-andrey.drobyshev@virtuozzo.com>

include/block/block-io.h | 8 +++----

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

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

3 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/include/block/block-io.h b/include/block/block-io.h

--- a/include/block/block-io.h

+++ b/include/block/block-io.h

@@ -XXX,XX +XXX,XX @@ bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);

ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,

Error **errp);

BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);

-void bdrv_round_to_clusters(BlockDriverState *bs,

- int64_t offset, int64_t bytes,

- int64_t *cluster_offset,

- int64_t *cluster_bytes);

+void bdrv_round_to_subclusters(BlockDriverState *bs,

+ int64_t offset, int64_t bytes,

+ int64_t *cluster_offset,

+ int64_t *cluster_bytes);

void bdrv_get_backing_filename(BlockDriverState *bs,

char *filename, int filename_size);

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

index XXXXXXX..XXXXXXX 100644

--- a/block/io.c

+++ b/block/io.c

@@ -XXX,XX +XXX,XX @@ BdrvTrackedRequest *coroutine_fn bdrv_co_get_self_request(BlockDriverState *bs)

/**

- * Round a region to cluster boundaries

+ * Round a region to subcluster (if supported) or cluster boundaries

*/

void coroutine_fn GRAPH_RDLOCK

-bdrv_round_to_clusters(BlockDriverState *bs, int64_t offset, int64_t bytes,

- int64_t *cluster_offset, int64_t *cluster_bytes)

+bdrv_round_to_subclusters(BlockDriverState *bs, int64_t offset, int64_t bytes,

+ int64_t *align_offset, int64_t *align_bytes)

{

BlockDriverInfo bdi;

IO_CODE();

- if (bdrv_co_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) {

- *cluster_offset = offset;

- *cluster_bytes = bytes;

+ if (bdrv_co_get_info(bs, &bdi) < 0 || bdi.subcluster_size == 0) {

+ *align_offset = offset;

+ *align_bytes = bytes;

} else {

- int64_t c = bdi.cluster_size;

- *cluster_offset = QEMU_ALIGN_DOWN(offset, c);

- *cluster_bytes = QEMU_ALIGN_UP(offset - *cluster_offset + bytes, c);

+ int64_t c = bdi.subcluster_size;

+ *align_offset = QEMU_ALIGN_DOWN(offset, c);

+ *align_bytes = QEMU_ALIGN_UP(offset - *align_offset + bytes, c);

@@ -XXX,XX +XXX,XX @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,

void *bounce_buffer = NULL;

BlockDriver *drv = bs->drv;

- int64_t cluster_offset;

- int64_t cluster_bytes;

+ int64_t align_offset;

+ int64_t align_bytes;

int64_t skip_bytes;

int ret;

int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,

@@ -XXX,XX +XXX,XX @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,

* BDRV_REQUEST_MAX_BYTES (even when the original read did not), which

* is one reason we loop rather than doing it all at once.

*/

- bdrv_round_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);

- skip_bytes = offset - cluster_offset;

+ bdrv_round_to_subclusters(bs, offset, bytes, &align_offset, &align_bytes);

+ skip_bytes = offset - align_offset;

trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,

- cluster_offset, cluster_bytes);

+ align_offset, align_bytes);

- while (cluster_bytes) {

+ while (align_bytes) {

int64_t pnum;

if (skip_write) {

ret = 1; /* "already allocated", so nothing will be copied */

- pnum = MIN(cluster_bytes, max_transfer);

+ pnum = MIN(align_bytes, max_transfer);

} else {

- ret = bdrv_is_allocated(bs, cluster_offset,

- MIN(cluster_bytes, max_transfer), &pnum);

+ ret = bdrv_is_allocated(bs, align_offset,

+ MIN(align_bytes, max_transfer), &pnum);

if (ret < 0) {

/*

* Safe to treat errors in querying allocation as if

* unallocated; we'll probably fail again soon on the

* read, but at least that will set a decent errno.

*/

- pnum = MIN(cluster_bytes, max_transfer);

+ pnum = MIN(align_bytes, max_transfer);

}

/* Stop at EOF if the image ends in the middle of the cluster */

@@ -XXX,XX +XXX,XX @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,

/* Must copy-on-read; use the bounce buffer */

pnum = MIN(pnum, MAX_BOUNCE_BUFFER);

if (!bounce_buffer) {

- int64_t max_we_need = MAX(pnum, cluster_bytes - pnum);

+ int64_t max_we_need = MAX(pnum, align_bytes - pnum);

int64_t max_allowed = MIN(max_transfer, MAX_BOUNCE_BUFFER);

int64_t bounce_buffer_len = MIN(max_we_need, max_allowed);

@@ -XXX,XX +XXX,XX @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,

}

qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);

- ret = bdrv_driver_preadv(bs, cluster_offset, pnum,

+ ret = bdrv_driver_preadv(bs, align_offset, pnum,

&local_qiov, 0, 0);

if (ret < 0) {

goto err;

@@ -XXX,XX +XXX,XX @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,

/* FIXME: Should we (perhaps conditionally) be setting

* BDRV_REQ_MAY_UNMAP, if it will allow for a sparser copy

* that still correctly reads as zero? */

- ret = bdrv_co_do_pwrite_zeroes(bs, cluster_offset, pnum,

+ ret = bdrv_co_do_pwrite_zeroes(bs, align_offset, pnum,

BDRV_REQ_WRITE_UNCHANGED);

} else {

/* This does not change the data on the disk, it is not

* necessary to flush even in cache=writethrough mode.

*/

- ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,

+ ret = bdrv_driver_pwritev(bs, align_offset, pnum,

&local_qiov, 0,

BDRV_REQ_WRITE_UNCHANGED);

}

@@ -XXX,XX +XXX,XX @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,

}

}

- cluster_offset += pnum;

- cluster_bytes -= pnum;

+ align_offset += pnum;

+ align_bytes -= pnum;

progress += pnum - skip_bytes;

skip_bytes = 0;

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

index XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn mirror_cow_align(MirrorBlockJob *s, int64_t *offset,

need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,

s->cow_bitmap);

if (need_cow) {

- bdrv_round_to_clusters(blk_bs(s->target), *offset, *bytes,

- &align_offset, &align_bytes);

+ bdrv_round_to_subclusters(blk_bs(s->target), *offset, *bytes,

+ &align_offset, &align_bytes);

}

if (align_bytes > max_bytes) {

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn mirror_iteration(MirrorBlockJob *s)

int64_t target_offset;

int64_t target_bytes;

WITH_GRAPH_RDLOCK_GUARD() {

- bdrv_round_to_clusters(blk_bs(s->target), offset, io_bytes,

- &target_offset, &target_bytes);

+ bdrv_round_to_subclusters(blk_bs(s->target), offset, io_bytes,

+ &target_offset, &target_bytes);

}

if (target_offset == offset &&

target_bytes == io_bytes) {

2.41.0

From: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Add testcase which checks that allocations during copy-on-read are

performed on the subcluster basis when subclusters are enabled in target

image.

This testcase also triggers the following assert with previous commit

not being applied, so we check that as well:

qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Denis V. Lunev <den@openvz.org>

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

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

Message-ID: <20230711172553.234055-4-andrey.drobyshev@virtuozzo.com>

tests/qemu-iotests/197 | 29 +++++++++++++++++++++++++++++

tests/qemu-iotests/197.out | 24 ++++++++++++++++++++++++

2 files changed, 53 insertions(+)

diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197

--- a/tests/qemu-iotests/197

+++ b/tests/qemu-iotests/197

@@ -XXX,XX +XXX,XX @@ $QEMU_IO -f qcow2 -C -c 'read 0 1024' "$TEST_WRAP" | _filter_qemu_io

$QEMU_IO -f qcow2 -c map "$TEST_WRAP"

_check_test_img

+echo '=== Copy-on-read with subclusters ==='

+# Create base and top images 64K (1 cluster) each. Make subclusters enabled

+# for the top image

+_make_test_img 64K

+IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \

+ _make_test_img --no-opts -o extended_l2=true -F "$IMGFMT" -b "$TEST_IMG" \

+ 64K | _filter_img_create

+$QEMU_IO -c "write -P 0xaa 0 64k" "$TEST_IMG" | _filter_qemu_io

+# Allocate individual subclusters in the top image, and not the whole cluster

+$QEMU_IO -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \

+ | _filter_qemu_io

+# Only 2 subclusters should be allocated in the top image at this point

+$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map

+

+# Actual copy-on-read operation

+$QEMU_IO -C -c "read -P 0xaa 30K 4K" "$TEST_WRAP" | _filter_qemu_io

+

+# And here we should have 4 subclusters allocated right in the middle of the

+# top image. Make sure the whole cluster remains unallocated

+$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map

+

+_check_test_img

echo '*** done'

status=0

diff --git a/tests/qemu-iotests/197.out b/tests/qemu-iotests/197.out

--- a/tests/qemu-iotests/197.out

+++ b/tests/qemu-iotests/197.out

@@ -XXX,XX +XXX,XX @@ read 1024/1024 bytes at offset 0

1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)

No errors were found on the image.

+=== Copy-on-read with subclusters ===

+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536

+Formatting 'TEST_DIR/t.wrap.IMGFMT', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT

+wrote 65536/65536 bytes at offset 0

+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+wrote 2048/2048 bytes at offset 28672

+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+wrote 2048/2048 bytes at offset 34816

+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+Offset Length File

+0 0x7000 TEST_DIR/t.IMGFMT

+0x7000 0x800 TEST_DIR/t.wrap.IMGFMT

+0x7800 0x1000 TEST_DIR/t.IMGFMT

+0x8800 0x800 TEST_DIR/t.wrap.IMGFMT

+0x9000 0x7000 TEST_DIR/t.IMGFMT

+read 4096/4096 bytes at offset 30720

+Offset Length File

+0 0x7000 TEST_DIR/t.IMGFMT

+0x7000 0x2000 TEST_DIR/t.wrap.IMGFMT

+0x9000 0x7000 TEST_DIR/t.IMGFMT

+No errors were found on the image.

2.41.0

liburing does not clear sqe->user_data. We must do it ourselves to avoid

undefined behavior in process_cqe() when user_data is used.

Note that fdmon-io_uring is currently disabled, so this is a latent bug

that does not affect users. Let's merge this fix now to make it easier

to enable fdmon-io_uring in the future (and I'm working on that).

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

Message-ID: <20230426212639.82310-1-stefanha@redhat.com>

---

util/fdmon-io_uring.c | 2 ++

1 file changed, 2 insertions(+)

diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c

--- a/util/fdmon-io_uring.c

+++ b/util/fdmon-io_uring.c

@@ -XXX,XX +XXX,XX @@ static void add_poll_remove_sqe(AioContext *ctx, AioHandler *node)

#else

io_uring_prep_poll_remove(sqe, node);

#endif

+ io_uring_sqe_set_data(sqe, NULL);

/* Add a timeout that self-cancels when another cqe becomes ready */

@@ -XXX,XX +XXX,XX @@ static void add_timeout_sqe(AioContext *ctx, int64_t ns)

sqe = get_sqe(ctx);

io_uring_prep_timeout(sqe, &ts, 1, 0);

+ io_uring_sqe_set_data(sqe, NULL);