On 9/13/24 7:39 PM, Andrey Drobyshev wrote:
> Introduce Qcow2 runtime boolean option "discard-subclusters". This
> option influences discard alignment value (either cluster_size or
> subcluster_size) and essentially makes subcluster-based discard optional.
> We disable it by default.
>
> Also tweak iotests/271 to enable this option and really test subcluster
> based discards.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com>
> ---
> block/qcow2.c | 21 ++++++++++++++++++++-
> block/qcow2.h | 2 ++
> tests/qemu-iotests/271 | 10 ++++++----
> 3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c2086d0bd1..7c38a5be41 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -685,6 +685,7 @@ static const char *const mutable_opts[] = {
> QCOW2_OPT_DISCARD_SNAPSHOT,
> QCOW2_OPT_DISCARD_OTHER,
> QCOW2_OPT_DISCARD_NO_UNREF,
> + QCOW2_OPT_DISCARD_SUBCLUSTERS,
> QCOW2_OPT_OVERLAP,
> QCOW2_OPT_OVERLAP_TEMPLATE,
> QCOW2_OPT_OVERLAP_MAIN_HEADER,
> @@ -734,6 +735,11 @@ static QemuOptsList qcow2_runtime_opts = {
> .type = QEMU_OPT_BOOL,
> .help = "Do not unreference discarded clusters",
> },
> + {
> + .name = QCOW2_OPT_DISCARD_SUBCLUSTERS,
> + .type = QEMU_OPT_BOOL,
> + .help = "Allow subcluster aligned discard requests",
> + },
> {
> .name = QCOW2_OPT_OVERLAP,
> .type = QEMU_OPT_STRING,
> @@ -978,6 +984,7 @@ typedef struct Qcow2ReopenState {
> int overlap_check;
> bool discard_passthrough[QCOW2_DISCARD_MAX];
> bool discard_no_unref;
> + bool discard_subclusters;
> uint64_t cache_clean_interval;
> QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options
> */
> } Qcow2ReopenState;
> @@ -1157,6 +1164,16 @@ qcow2_update_options_prepare(BlockDriverState *bs,
> Qcow2ReopenState *r,
> goto fail;
> }
>
> + r->discard_subclusters =
> + qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SUBCLUSTERS, false);
> + if (r->discard_subclusters && !has_subclusters(s)) {
> + error_setg(errp,
> + "Image doesn't have extended L2 entries, but option "
> + "'discard-subclusters' is enabled");
> + ret = -EINVAL;
> + goto fail;
I realized that failing here might not be the best course of action,
since non-presence of extended L2 entries in an image is an external
condition which we can't control. I guess we can just do warn_report()
instead.
> + }
> +
> switch (s->crypt_method_header) {
> case QCOW_CRYPT_NONE:
> if (encryptfmt) {
> @@ -1238,6 +1255,7 @@ static void
> qcow2_update_options_commit(BlockDriverState *bs,
> }
>
> s->discard_no_unref = r->discard_no_unref;
> + s->discard_subclusters = r->discard_subclusters;
>
> if (s->cache_clean_interval != r->cache_clean_interval) {
> cache_clean_timer_del(bs);
> @@ -1981,7 +1999,8 @@ static void qcow2_refresh_limits(BlockDriverState *bs,
> Error **errp)
> bs->bl.request_alignment = qcrypto_block_get_sector_size(s->crypto);
> }
> bs->bl.pwrite_zeroes_alignment = s->subcluster_size;
> - bs->bl.pdiscard_alignment = s->subcluster_size;
> + bs->bl.pdiscard_alignment = s->discard_subclusters ?
> + s->subcluster_size : s->cluster_size;
> }
>
> static int GRAPH_UNLOCKED
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a65c185b51..4e91bdde3f 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -134,6 +134,7 @@
> #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
> #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other"
> #define QCOW2_OPT_DISCARD_NO_UNREF "discard-no-unref"
> +#define QCOW2_OPT_DISCARD_SUBCLUSTERS "discard-subclusters"
> #define QCOW2_OPT_OVERLAP "overlap-check"
> #define QCOW2_OPT_OVERLAP_TEMPLATE "overlap-check.template"
> #define QCOW2_OPT_OVERLAP_MAIN_HEADER "overlap-check.main-header"
> @@ -387,6 +388,7 @@ typedef struct BDRVQcow2State {
> bool discard_passthrough[QCOW2_DISCARD_MAX];
>
> bool discard_no_unref;
> + bool discard_subclusters;
>
> int overlap_check; /* bitmask of Qcow2MetadataOverlap values */
> bool signaled_corruption;
> diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
> index 8b80682cff..d7cf3c459b 100755
> --- a/tests/qemu-iotests/271
> +++ b/tests/qemu-iotests/271
> @@ -100,13 +100,14 @@ _filter_trace_fallocate()
> # discard -> discard
> _run_test()
> {
> - unset c sc off len cmd opt
> + unset c sc off len cmd trace opt
> for var in "$@"; do eval "$var"; done
> case "${cmd:-write}" in
> zero)
> cmd="write -q -z";;
> unmap)
> - opt="--trace enable=file_do_fallocate"
> + trace="--trace enable=file_do_fallocate"
> + opt="-c reopen -o discard-subclusters=on"
> cmd="write -q -z -u";;
> compress)
> pat=$((${pat:-0} + 1))
> @@ -115,7 +116,8 @@ _run_test()
> pat=$((${pat:-0} + 1))
> cmd="write -q -P ${pat}";;
> discard)
> - opt="--trace enable=file_do_fallocate"
> + trace="--trace enable=file_do_fallocate"
> + opt="-c reopen -o discard-subclusters=on"
> cmd="discard -q";;
> *)
> echo "Unknown option $cmd"
> @@ -129,7 +131,7 @@ _run_test()
> cmd="$cmd ${offset} ${len}"
> raw_cmd=$(echo $cmd | sed s/-c//) # Raw images don't support -c
> echo $cmd | sed 's/-P [0-9][0-9]\?/-P PATTERN/'
> - $QEMU_IO $opt -c "$cmd" "$TEST_IMG" 2>&1 | _filter_qemu_io |
> _filter_trace_fallocate
> + $QEMU_IO $trace ${opt:+ "$opt"} -c "$cmd" "$TEST_IMG" 2>&1 |
> _filter_qemu_io | _filter_trace_fallocate
> $QEMU_IO -c "$raw_cmd" -f raw "$TEST_IMG.raw" | _filter_qemu_io
> _verify_img
> _verify_l2_bitmap "$c"
