diff mbox series

[v3,12/12] qcow2: add discard-subclusters option

Message ID 20240913163942.423050-13-andrey.drobyshev@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series qcow2: make subclusters discardable | expand

Commit Message

Andrey Drobyshev Sept. 13, 2024, 4:39 p.m. UTC 
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.drobyshev@virtuozzo.com>
---
 block/qcow2.c          | 21 ++++++++++++++++++++-
 block/qcow2.h          |  2 ++
 tests/qemu-iotests/271 | 10 ++++++----
 3 files changed, 28 insertions(+), 5 deletions(-)

Comments

Andrey Drobyshev Sept. 16, 2024, 6:29 a.m. UTC | #1 
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.drobyshev@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"
diff mbox series

Patch

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;
+    }
+
     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"