From b2b109041ecd1095384f5be5bb9badd13c1cf286 Mon Sep 17 00:00:00 2001 From: Jean-Louis Dupond Date: Tue, 3 Oct 2023 14:52:37 +0200 Subject: [PATCH 1/3] qcow2: keep reference on zeroize with discard-no-unref enabled When the discard-no-unref flag is enabled, we keep the reference for normal discard requests. But when a discard is executed on a snapshot/qcow2 image with backing, the discards are saved as zero clusters in the snapshot image. When committing the snapshot to the backing file, not discard_in_l2_slice is called but zero_in_l2_slice. Which did not had any logic to keep the reference when discard-no-unref is enabled. Therefor we add logic in the zero_in_l2_slice call to keep the reference on commit. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1621 Signed-off-by: Jean-Louis Dupond Message-Id: <20231003125236.216473-2-jean-louis@dupond.be> [hreitz: Made the documentation change more verbose, as discussed on-list] Signed-off-by: Hanna Czenczek --- block/qcow2-cluster.c | 22 ++++++++++++++++++---- qapi/block-core.json | 24 ++++++++++++++---------- qemu-options.hx | 10 +++++++--- 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 904f00d1b3..5af439bd11 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1983,7 +1983,7 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters, /* If we keep the reference, pass on the discard still */ bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, s->cluster_size); - } + } } qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice); @@ -2061,9 +2061,15 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset, QCow2ClusterType type = qcow2_get_cluster_type(bs, old_l2_entry); bool unmap = (type == QCOW2_CLUSTER_COMPRESSED) || ((flags & BDRV_REQ_MAY_UNMAP) && qcow2_cluster_is_allocated(type)); - uint64_t new_l2_entry = unmap ? 0 : old_l2_entry; + bool keep_reference = + (s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED); + uint64_t new_l2_entry = old_l2_entry; uint64_t new_l2_bitmap = old_l2_bitmap; + if (unmap && !keep_reference) { + new_l2_entry = 0; + } + if (has_subclusters(s)) { new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES; } else { @@ -2081,9 +2087,17 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset, set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap); } - /* Then decrease the refcount */ if (unmap) { - qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST); + if (!keep_reference) { + /* Then decrease the refcount */ + qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST); + } else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] && + (type == QCOW2_CLUSTER_NORMAL || + type == QCOW2_CLUSTER_ZERO_ALLOC)) { + /* If we keep the reference, pass on the discard still */ + bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK, + s->cluster_size); + } } } diff --git a/qapi/block-core.json b/qapi/block-core.json index 99961256f2..ca390c5700 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3528,16 +3528,20 @@ # @pass-discard-other: whether discard requests for the data source # should be issued on other occasions where a cluster gets freed # -# @discard-no-unref: when enabled, discards from the guest will not -# cause cluster allocations to be relinquished. This prevents -# qcow2 fragmentation that would be caused by such discards. -# Besides potential performance degradation, such fragmentation -# can lead to increased allocation of clusters past the end of the -# image file, resulting in image files whose file length can grow -# much larger than their guest disk size would suggest. If image -# file length is of concern (e.g. when storing qcow2 images -# directly on block devices), you should consider enabling this -# option. (since 8.1) +# @discard-no-unref: when enabled, data clusters will remain +# preallocated when they are no longer used, e.g. because they are +# discarded or converted to zero clusters. As usual, whether the +# old data is discarded or kept on the protocol level (i.e. in the +# image file) depends on the setting of the pass-discard-request +# option. Keeping the clusters preallocated prevents qcow2 +# fragmentation that would otherwise be caused by freeing and +# re-allocating them later. Besides potential performance +# degradation, such fragmentation can lead to increased allocation +# of clusters past the end of the image file, resulting in image +# files whose file length can grow much larger than their guest disk +# size would suggest. If image file length is of concern (e.g. when +# storing qcow2 images directly on block devices), you should +# consider enabling this option. (since 8.1) # # @overlap-check: which overlap checks to perform for writes to the # image, defaults to 'cached' (since 2.2) diff --git a/qemu-options.hx b/qemu-options.hx index e26230bac5..7809036d8c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1457,9 +1457,13 @@ SRST (on/off; default: off) ``discard-no-unref`` - When enabled, discards from the guest will not cause cluster - allocations to be relinquished. This prevents qcow2 fragmentation - that would be caused by such discards. Besides potential + When enabled, data clusters will remain preallocated when they are + no longer used, e.g. because they are discarded or converted to + zero clusters. As usual, whether the old data is discarded or kept + on the protocol level (i.e. in the image file) depends on the + setting of the pass-discard-request option. Keeping the clusters + preallocated prevents qcow2 fragmentation that would otherwise be + caused by freeing and re-allocating them later. Besides potential performance degradation, such fragmentation can lead to increased allocation of clusters past the end of the image file, resulting in image files whose file length can grow much larger From 10b9e0802a074c991e1ce485631d75641d0b0f9e Mon Sep 17 00:00:00 2001 From: Sam Li Date: Fri, 25 Aug 2023 12:05:56 +0800 Subject: [PATCH 2/3] block/file-posix: fix update_zones_wp() caller When the zoned request fail, it needs to update only the wp of the target zones for not disrupting the in-flight writes on these other zones. The wp is updated successfully after the request completes. Fixed the callers with right offset and nr_zones. Signed-off-by: Sam Li Message-Id: <20230825040556.4217-1-faithilikerun@gmail.com> Reviewed-by: Stefan Hajnoczi [hreitz: Rebased and fixed comment spelling] Signed-off-by: Hanna Czenczek --- block/file-posix.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index 50e2b20d5c..3c0ce9c258 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2523,7 +2523,10 @@ out: } } } else { - update_zones_wp(bs, s->fd, 0, 1); + /* + * write and append write are not allowed to cross zone boundaries + */ + update_zones_wp(bs, s->fd, offset, 1); } qemu_co_mutex_unlock(&wps->colock); @@ -3470,7 +3473,7 @@ static int coroutine_fn raw_co_zone_mgmt(BlockDriverState *bs, BlockZoneOp op, len >> BDRV_SECTOR_BITS); ret = raw_thread_pool_submit(handle_aiocb_zone_mgmt, &acb); if (ret != 0) { - update_zones_wp(bs, s->fd, offset, i); + update_zones_wp(bs, s->fd, offset, nrz); error_report("ioctl %s failed %d", op_name, ret); return ret; } From ad4feaca61d76fecad784e6d5e7bae40d0411c46 Mon Sep 17 00:00:00 2001 From: Naohiro Aota Date: Mon, 30 Oct 2023 16:38:53 +0900 Subject: [PATCH 3/3] file-posix: fix over-writing of returning zone_append offset raw_co_zone_append() sets "s->offset" where "BDRVRawState *s". This pointer is used later at raw_co_prw() to save the block address where the data is written. When multiple IOs are on-going at the same time, a later IO's raw_co_zone_append() call over-writes a former IO's offset address before raw_co_prw() completes. As a result, the former zone append IO returns the initial value (= the start address of the writing zone), instead of the proper address. Fix the issue by passing the offset pointer to raw_co_prw() instead of passing it through s->offset. Also, remove "offset" from BDRVRawState as there is no usage anymore. Fixes: 4751d09adcc3 ("block: introduce zone append write for zoned devices") Signed-off-by: Naohiro Aota Message-Id: <20231030073853.2601162-1-naohiro.aota@wdc.com> Reviewed-by: Sam Li Reviewed-by: Stefan Hajnoczi Signed-off-by: Hanna Czenczek --- block/file-posix.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index 3c0ce9c258..b862406c71 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -160,7 +160,6 @@ typedef struct BDRVRawState { bool has_write_zeroes:1; bool use_linux_aio:1; bool use_linux_io_uring:1; - int64_t *offset; /* offset of zone append operation */ int page_cache_inconsistent; /* errno from fdatasync failure */ bool has_fallocate; bool needs_alignment; @@ -2445,12 +2444,13 @@ static bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov) return true; } -static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset, +static int coroutine_fn raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, uint64_t bytes, QEMUIOVector *qiov, int type) { BDRVRawState *s = bs->opaque; RawPosixAIOData acb; int ret; + uint64_t offset = *offset_ptr; if (fd_open(bs) < 0) return -EIO; @@ -2513,8 +2513,8 @@ out: uint64_t *wp = &wps->wp[offset / bs->bl.zone_size]; if (!BDRV_ZT_IS_CONV(*wp)) { if (type & QEMU_AIO_ZONE_APPEND) { - *s->offset = *wp; - trace_zbd_zone_append_complete(bs, *s->offset + *offset_ptr = *wp; + trace_zbd_zone_append_complete(bs, *offset_ptr >> BDRV_SECTOR_BITS); } /* Advance the wp if needed */ @@ -2539,14 +2539,14 @@ static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) { - return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_READ); + return raw_co_prw(bs, &offset, bytes, qiov, QEMU_AIO_READ); } static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes, QEMUIOVector *qiov, BdrvRequestFlags flags) { - return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_WRITE); + return raw_co_prw(bs, &offset, bytes, qiov, QEMU_AIO_WRITE); } static int coroutine_fn raw_co_flush_to_disk(BlockDriverState *bs) @@ -3509,8 +3509,6 @@ static int coroutine_fn raw_co_zone_append(BlockDriverState *bs, int64_t zone_size_mask = bs->bl.zone_size - 1; int64_t iov_len = 0; int64_t len = 0; - BDRVRawState *s = bs->opaque; - s->offset = offset; if (*offset & zone_size_mask) { error_report("sector offset %" PRId64 " is not aligned to zone size " @@ -3531,7 +3529,7 @@ static int coroutine_fn raw_co_zone_append(BlockDriverState *bs, } trace_zbd_zone_append(bs, *offset >> BDRV_SECTOR_BITS); - return raw_co_prw(bs, *offset, len, qiov, QEMU_AIO_ZONE_APPEND); + return raw_co_prw(bs, offset, len, qiov, QEMU_AIO_ZONE_APPEND); } #endif