All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] qcow2: add keep-dirty open option
@ 2022-01-20 16:20 Vladimir Sementsov-Ogievskiy
  2022-01-20 16:20 ` [PATCH 1/2] " Vladimir Sementsov-Ogievskiy
  2022-01-20 16:20 ` [PATCH 2/2] iotests: add qcow2-keep-dirty Vladimir Sementsov-Ogievskiy
  0 siblings, 2 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-20 16:20 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, armbru, eblake, hreitz, kwolf, vsementsov, den, ktkhai, igor

Hi all! Here is suggestion of a new option which we need for our
developments in Virtuozzo.

For details look at patch 01.

Vladimir Sementsov-Ogievskiy (2):
  qcow2: add keep-dirty open option
  iotests: add qcow2-keep-dirty

 qapi/block-core.json                          |   5 +
 block/qcow2.h                                 |   2 +
 block/qcow2.c                                 |  66 +++++++++--
 tests/qemu-iotests/tests/qcow2-keep-dirty     | 104 ++++++++++++++++++
 tests/qemu-iotests/tests/qcow2-keep-dirty.out |  34 ++++++
 5 files changed, 199 insertions(+), 12 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/qcow2-keep-dirty
 create mode 100644 tests/qemu-iotests/tests/qcow2-keep-dirty.out

-- 
2.31.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] qcow2: add keep-dirty open option
  2022-01-20 16:20 [PATCH 0/2] qcow2: add keep-dirty open option Vladimir Sementsov-Ogievskiy
@ 2022-01-20 16:20 ` Vladimir Sementsov-Ogievskiy
  2022-01-27  9:43   ` Kirill Tkhai
  2022-01-20 16:20 ` [PATCH 2/2] iotests: add qcow2-keep-dirty Vladimir Sementsov-Ogievskiy
  1 sibling, 1 reply; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-20 16:20 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, armbru, eblake, hreitz, kwolf, vsementsov, den, ktkhai, igor

Consider the case:

Thirdparty component works with qcow2 image, and dirty bit is set.

Thirdparty component want to start qemu-img to do some manipulation.
Ofcourse, third party component flushes refcounts and other metadata
before starting QEMU.

But the component don't want to clear dirty bit, as this breaks
transactionability of the operation: we'll have to set it again but it
may fail. Clearing the dirty bit is unrecoverable action and can't be
transactional. That's a problem.

The solution is a new qcow2 open option: keep-dirty. When set:
1. On qcow2 open, ignore dirty bit and don't do check: caller is
   responsible for refcounts being valid.
2. Never clear dirty bit during QEMU execution, including close.

Details:

1. For simplicity let's just not allow keep-dirty without lazy
   refcounts.

2. Don't allow to open with keep-dirty when dirty bit is unset. This
   may mean some error in user logic.

3. For implementation do the following: dirty flag
   in s->incompatible_features behaves same way as without keep-dirty
   option: it actually designate status of refcounts dirtiness. But we
   never clear the flag in the image, and we remember that it is always
   set.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json |  5 ++++
 block/qcow2.h        |  2 ++
 block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++--------
 3 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9a5a3641d0..3e35357182 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3228,6 +3228,10 @@
 # @lazy-refcounts: whether to enable the lazy refcounts
 #                  feature (default is taken from the image file)
 #
+# @keep-dirty: whether to not touch dirty bit. When set, QEMU doesn't
+#              check refcounts on qcow2 open (ignoring dirty bit) and doesn't
+#              clear dirty bit on qcow2 close. (since 7.0)
+#
 # @pass-discard-request: whether discard requests to the qcow2
 #                        device should be forwarded to the data source
 #
@@ -3276,6 +3280,7 @@
 { 'struct': 'BlockdevOptionsQcow2',
   'base': 'BlockdevOptionsGenericCOWFormat',
   'data': { '*lazy-refcounts': 'bool',
+            '*keep-dirty': 'bool',
             '*pass-discard-request': 'bool',
             '*pass-discard-snapshot': 'bool',
             '*pass-discard-other': 'bool',
diff --git a/block/qcow2.h b/block/qcow2.h
index fd48a89d45..696e13377a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -130,6 +130,7 @@
 
 #define QCOW2_OPT_DATA_FILE "data-file"
 #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
+#define QCOW2_OPT_KEEP_DIRTY "keep-dirty"
 #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
 #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
 #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other"
@@ -376,6 +377,7 @@ typedef struct BDRVQcow2State {
     int flags;
     int qcow_version;
     bool use_lazy_refcounts;
+    bool keep_dirty;
     int refcount_order;
     int refcount_bits;
     uint64_t refcount_max;
diff --git a/block/qcow2.c b/block/qcow2.c
index d509016756..1c42103fb9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -514,15 +514,17 @@ int qcow2_mark_dirty(BlockDriverState *bs)
         return 0; /* already dirty */
     }
 
-    val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
-    ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
-                      &val, sizeof(val));
-    if (ret < 0) {
-        return ret;
-    }
-    ret = bdrv_flush(bs->file->bs);
-    if (ret < 0) {
-        return ret;
+    if (!s->keep_dirty) {
+        val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
+        ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
+                          &val, sizeof(val));
+        if (ret < 0) {
+            return ret;
+        }
+        ret = bdrv_flush(bs->file->bs);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     /* Only treat image as dirty if the header was updated successfully */
@@ -549,7 +551,13 @@ static int qcow2_mark_clean(BlockDriverState *bs)
             return ret;
         }
 
-        return qcow2_update_header(bs);
+        if (!s->keep_dirty) {
+            /*
+             * No reason to update the header if we don't want to clear dirty
+             * bit.
+             */
+            return qcow2_update_header(bs);
+        }
     }
     return 0;
 }
@@ -709,6 +717,11 @@ static QemuOptsList qcow2_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Postpone refcount updates",
         },
+        {
+            .name = QCOW2_OPT_KEEP_DIRTY,
+            .type = QEMU_OPT_BOOL,
+            .help = "Keep dirty bit untouched",
+        },
         {
             .name = QCOW2_OPT_DISCARD_REQUEST,
             .type = QEMU_OPT_BOOL,
@@ -966,6 +979,7 @@ typedef struct Qcow2ReopenState {
     Qcow2Cache *refcount_block_cache;
     int l2_slice_size; /* Number of entries in a slice of the L2 table */
     bool use_lazy_refcounts;
+    bool keep_dirty;
     int overlap_check;
     bool discard_passthrough[QCOW2_DISCARD_MAX];
     uint64_t cache_clean_interval;
@@ -1088,6 +1102,13 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
         }
     }
 
+    r->keep_dirty = qemu_opt_get_bool(opts, QCOW2_OPT_KEEP_DIRTY, false);
+    if (r->keep_dirty && !r->use_lazy_refcounts) {
+        error_setg(errp, "keep-dirty requires lazy refcounts");
+        ret = -EINVAL;
+        goto fail;
+    }
+
     /* Overlap check options */
     opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
     opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
@@ -1214,6 +1235,7 @@ static void qcow2_update_options_commit(BlockDriverState *bs,
 
     s->overlap_check = r->overlap_check;
     s->use_lazy_refcounts = r->use_lazy_refcounts;
+    s->keep_dirty = r->keep_dirty;
 
     for (i = 0; i < QCOW2_DISCARD_MAX; i++) {
         s->discard_passthrough[i] = r->discard_passthrough[i];
@@ -1810,7 +1832,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     bs->supported_truncate_flags = BDRV_REQ_ZERO_WRITE;
 
     /* Repair image if dirty */
-    if (!(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) &&
+    if (!s->keep_dirty && !(flags & BDRV_O_CHECK) && bdrv_is_writable(bs) &&
         (s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
         BdrvCheckResult result = {0};
 
@@ -1825,6 +1847,20 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         }
     }
 
+    if (s->keep_dirty) {
+        if (!(s->incompatible_features & QCOW2_INCOMPAT_DIRTY)) {
+            error_setg(errp, "keep-dirty behaviour is requested but image "
+                       "is not dirty");
+            ret = -EINVAL;
+            goto fail;
+        }
+        /*
+         * User guarantees that refcounts are valid. So, consider them valid,
+         * keeping dirty bit set in the header.
+         */
+        s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY;
+    }
+
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
@@ -2826,6 +2862,7 @@ int qcow2_update_header(BlockDriverState *bs)
     uint32_t refcount_table_clusters;
     size_t header_length;
     Qcow2UnknownHeaderExtension *uext;
+    uint64_t incompatible_features;
 
     buf = qemu_blockalign(bs, buflen);
 
@@ -2846,6 +2883,11 @@ int qcow2_update_header(BlockDriverState *bs)
         goto fail;
     }
 
+    incompatible_features = s->incompatible_features;
+    if (s->keep_dirty) {
+        incompatible_features |= QCOW2_INCOMPAT_DIRTY;
+    }
+
     *header = (QCowHeader) {
         /* Version 2 fields */
         .magic                  = cpu_to_be32(QCOW_MAGIC),
@@ -2863,7 +2905,7 @@ int qcow2_update_header(BlockDriverState *bs)
         .snapshots_offset       = cpu_to_be64(s->snapshots_offset),
 
         /* Version 3 fields */
-        .incompatible_features  = cpu_to_be64(s->incompatible_features),
+        .incompatible_features  = cpu_to_be64(incompatible_features),
         .compatible_features    = cpu_to_be64(s->compatible_features),
         .autoclear_features     = cpu_to_be64(s->autoclear_features),
         .refcount_order         = cpu_to_be32(s->refcount_order),
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] iotests: add qcow2-keep-dirty
  2022-01-20 16:20 [PATCH 0/2] qcow2: add keep-dirty open option Vladimir Sementsov-Ogievskiy
  2022-01-20 16:20 ` [PATCH 1/2] " Vladimir Sementsov-Ogievskiy
@ 2022-01-20 16:20 ` Vladimir Sementsov-Ogievskiy
  1 sibling, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-20 16:20 UTC (permalink / raw)
  To: qemu-block
  Cc: qemu-devel, armbru, eblake, hreitz, kwolf, vsementsov, den, ktkhai, igor

Test new qcow2 open option: keep-dirty.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/tests/qcow2-keep-dirty     | 104 ++++++++++++++++++
 tests/qemu-iotests/tests/qcow2-keep-dirty.out |  34 ++++++
 2 files changed, 138 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/qcow2-keep-dirty
 create mode 100644 tests/qemu-iotests/tests/qcow2-keep-dirty.out

diff --git a/tests/qemu-iotests/tests/qcow2-keep-dirty b/tests/qemu-iotests/tests/qcow2-keep-dirty
new file mode 100755
index 0000000000..101a82bd28
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-keep-dirty
@@ -0,0 +1,104 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test qcow2 keep-dirty option
+#
+# Copyright (c) 2022 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=vsementsov@virtuozzo.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ../common.rc
+. ../common.filter
+. ../common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+# This test does not make much sense with external data files
+_unsupported_imgopts data_file
+
+check_dirty_bit() {
+    $QEMU_IMG info --output=json "$TEST_IMG" | grep 'dirty-flag'
+}
+
+set_dirty_bit() {
+    echo Set dirty bit
+    $PYTHON ../qcow2.py "$TEST_IMG" set-feature-bit incompatible 0
+    check_dirty_bit
+}
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+keep_dirty_opts="driver=qcow2,keep-dirty=true,file.filename=$TEST_IMG"
+
+size=10M
+
+_make_test_img $size
+
+echo Check that keep-dirty not allowed without lazy refcounts
+$QEMU_IO -c 'quit' --image-opts "$keep_dirty_opts"
+
+_make_test_img -o "lazy_refcounts=on" $size
+
+echo
+echo Check that keep-dirty not allowed without dirty bit
+$QEMU_IO -c 'quit' --image-opts "$keep_dirty_opts"
+
+echo
+echo Check that usual access clears dirty bit
+set_dirty_bit
+$QEMU_IO -c 'quit' "$TEST_IMG"
+check_dirty_bit
+
+echo
+echo Check keep-dirty
+set_dirty_bit
+$QEMU_IO -c 'quit' --image-opts "$keep_dirty_opts"
+check_dirty_bit
+
+echo
+echo Check that usual qemu-img check clears dirty bit
+set_dirty_bit
+$QEMU_IMG check -r all "$TEST_IMG"
+check_dirty_bit
+
+echo
+echo Test qemu-img check with keep-dirty
+set_dirty_bit
+# also set corrupt bit
+$PYTHON ../qcow2.py "$TEST_IMG" set-feature-bit incompatible 1
+$PYTHON ../qcow2.py "$TEST_IMG" dump-header | grep incompatible
+$QEMU_IMG check -r all --image-opts "$keep_dirty_opts"
+$PYTHON ../qcow2.py "$TEST_IMG" dump-header | grep incompatible
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/qcow2-keep-dirty.out b/tests/qemu-iotests/tests/qcow2-keep-dirty.out
new file mode 100644
index 0000000000..4d2bf40521
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-keep-dirty.out
@@ -0,0 +1,34 @@
+QA output created by qcow2-keep-dirty
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=10485760
+Check that keep-dirty not allowed without lazy refcounts
+qemu-io: can't open: keep-dirty requires lazy refcounts
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=10485760
+
+Check that keep-dirty not allowed without dirty bit
+qemu-io: can't open: keep-dirty behaviour is requested but image is not dirty
+
+Check that usual access clears dirty bit
+Set dirty bit
+    "dirty-flag": true
+    "dirty-flag": false
+
+Check keep-dirty
+Set dirty bit
+    "dirty-flag": true
+    "dirty-flag": true
+
+Check that usual qemu-img check clears dirty bit
+Set dirty bit
+    "dirty-flag": true
+No errors were found on the image.
+Image end offset: 262144
+    "dirty-flag": false
+
+Test qemu-img check with keep-dirty
+Set dirty bit
+    "dirty-flag": true
+incompatible_features     [0, 1]
+No errors were found on the image.
+Image end offset: 262144
+incompatible_features     [0]
+*** done
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] qcow2: add keep-dirty open option
  2022-01-20 16:20 ` [PATCH 1/2] " Vladimir Sementsov-Ogievskiy
@ 2022-01-27  9:43   ` Kirill Tkhai
  2022-01-27  9:52     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 5+ messages in thread
From: Kirill Tkhai @ 2022-01-27  9:43 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, qemu-block
  Cc: qemu-devel, armbru, eblake, hreitz, kwolf, den, igor

On 20.01.2022 19:20, Vladimir Sementsov-Ogievskiy wrote:
> Consider the case:
> 
> Thirdparty component works with qcow2 image, and dirty bit is set.
> 
> Thirdparty component want to start qemu-img to do some manipulation.
> Ofcourse, third party component flushes refcounts and other metadata
> before starting QEMU.
> 
> But the component don't want to clear dirty bit, as this breaks
> transactionability of the operation: we'll have to set it again but it
> may fail. Clearing the dirty bit is unrecoverable action and can't be
> transactional. That's a problem.
> 
> The solution is a new qcow2 open option: keep-dirty. When set:
> 1. On qcow2 open, ignore dirty bit and don't do check: caller is
>    responsible for refcounts being valid.
> 2. Never clear dirty bit during QEMU execution, including close.
> 
> Details:
> 
> 1. For simplicity let's just not allow keep-dirty without lazy
>    refcounts.
> 
> 2. Don't allow to open with keep-dirty when dirty bit is unset. This
>    may mean some error in user logic.
> 
> 3. For implementation do the following: dirty flag
>    in s->incompatible_features behaves same way as without keep-dirty
>    option: it actually designate status of refcounts dirtiness. But we
>    never clear the flag in the image, and we remember that it is always
>    set.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json |  5 ++++
>  block/qcow2.h        |  2 ++
>  block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++--------
>  3 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9a5a3641d0..3e35357182 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3228,6 +3228,10 @@
>  # @lazy-refcounts: whether to enable the lazy refcounts
>  #                  feature (default is taken from the image file)
>  #
> +# @keep-dirty: whether to not touch dirty bit. When set, QEMU doesn't
> +#              check refcounts on qcow2 open (ignoring dirty bit) and doesn't
> +#              clear dirty bit on qcow2 close. (since 7.0)
> +#
>  # @pass-discard-request: whether discard requests to the qcow2
>  #                        device should be forwarded to the data source
>  #
> @@ -3276,6 +3280,7 @@
>  { 'struct': 'BlockdevOptionsQcow2',
>    'base': 'BlockdevOptionsGenericCOWFormat',
>    'data': { '*lazy-refcounts': 'bool',
> +            '*keep-dirty': 'bool',
>              '*pass-discard-request': 'bool',
>              '*pass-discard-snapshot': 'bool',
>              '*pass-discard-other': 'bool',
> diff --git a/block/qcow2.h b/block/qcow2.h
> index fd48a89d45..696e13377a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -130,6 +130,7 @@
>  
>  #define QCOW2_OPT_DATA_FILE "data-file"
>  #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
> +#define QCOW2_OPT_KEEP_DIRTY "keep-dirty"
>  #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
>  #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
>  #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other"
> @@ -376,6 +377,7 @@ typedef struct BDRVQcow2State {
>      int flags;
>      int qcow_version;
>      bool use_lazy_refcounts;
> +    bool keep_dirty;
>      int refcount_order;
>      int refcount_bits;
>      uint64_t refcount_max;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d509016756..1c42103fb9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -514,15 +514,17 @@ int qcow2_mark_dirty(BlockDriverState *bs)
>          return 0; /* already dirty */
>      }
>  
> -    val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
> -    ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
> -                      &val, sizeof(val));
> -    if (ret < 0) {
> -        return ret;
> -    }
> -    ret = bdrv_flush(bs->file->bs);
> -    if (ret < 0) {
> -        return ret;
> +    if (!s->keep_dirty) {
> +        val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
> +        ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
> +                          &val, sizeof(val));
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        ret = bdrv_flush(bs->file->bs);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      /* Only treat image as dirty if the header was updated successfully */
> @@ -549,7 +551,13 @@ static int qcow2_mark_clean(BlockDriverState *bs)
>              return ret;
>          }
>  
> -        return qcow2_update_header(bs);
> +        if (!s->keep_dirty) {
> +            /*
> +             * No reason to update the header if we don't want to clear dirty
> +             * bit.
> +             */
> +            return qcow2_update_header(bs);
> +        }
>      }
>      return 0;
>  }
> @@ -709,6 +717,11 @@ static QemuOptsList qcow2_runtime_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "Postpone refcount updates",
>          },
> +        {
> +            .name = QCOW2_OPT_KEEP_DIRTY,
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Keep dirty bit untouched",
> +        },
>          {
>              .name = QCOW2_OPT_DISCARD_REQUEST,
>              .type = QEMU_OPT_BOOL,
> @@ -966,6 +979,7 @@ typedef struct Qcow2ReopenState {
>      Qcow2Cache *refcount_block_cache;
>      int l2_slice_size; /* Number of entries in a slice of the L2 table */
>      bool use_lazy_refcounts;
> +    bool keep_dirty;
>      int overlap_check;
>      bool discard_passthrough[QCOW2_DISCARD_MAX];
>      uint64_t cache_clean_interval;
> @@ -1088,6 +1102,13 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
>          }
>      }
>  
> +    r->keep_dirty = qemu_opt_get_bool(opts, QCOW2_OPT_KEEP_DIRTY, false);
> +    if (r->keep_dirty && !r->use_lazy_refcounts) {
> +        error_setg(errp, "keep-dirty requires lazy refcounts");
> +        ret = -EINVAL;
> +        goto fail;

Vladimir, according to documentation dirty bit and lazy refcounts are independent:
                    
                    incompatible_features:
                    Bit 0:      Dirty bit.  If this bit is set then refcounts
                                may be inconsistent, make sure to scan L1/L2
                                tables to repair refcounts before accessing the
                                image.

                    compatible_features:
                    Bit 0:      Lazy refcounts bit.  If this bit is set then
                                lazy refcount updates can be used.  This means
                                marking the image file dirty and postponing
                                refcount metadata updates.

while this patch assumes existence of some dependence between them.
Is there a real reason to introduce such the connection?

[snip]

Kirill


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] qcow2: add keep-dirty open option
  2022-01-27  9:43   ` Kirill Tkhai
@ 2022-01-27  9:52     ` Vladimir Sementsov-Ogievskiy
  0 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-01-27  9:52 UTC (permalink / raw)
  To: Kirill Tkhai, qemu-block
  Cc: qemu-devel, armbru, eblake, hreitz, kwolf, den, igor

27.01.2022 12:43, Kirill Tkhai wrote:
> On 20.01.2022 19:20, Vladimir Sementsov-Ogievskiy wrote:
>> Consider the case:
>>
>> Thirdparty component works with qcow2 image, and dirty bit is set.
>>
>> Thirdparty component want to start qemu-img to do some manipulation.
>> Ofcourse, third party component flushes refcounts and other metadata
>> before starting QEMU.
>>
>> But the component don't want to clear dirty bit, as this breaks
>> transactionability of the operation: we'll have to set it again but it
>> may fail. Clearing the dirty bit is unrecoverable action and can't be
>> transactional. That's a problem.
>>
>> The solution is a new qcow2 open option: keep-dirty. When set:
>> 1. On qcow2 open, ignore dirty bit and don't do check: caller is
>>     responsible for refcounts being valid.
>> 2. Never clear dirty bit during QEMU execution, including close.
>>
>> Details:
>>
>> 1. For simplicity let's just not allow keep-dirty without lazy
>>     refcounts.
>>
>> 2. Don't allow to open with keep-dirty when dirty bit is unset. This
>>     may mean some error in user logic.
>>
>> 3. For implementation do the following: dirty flag
>>     in s->incompatible_features behaves same way as without keep-dirty
>>     option: it actually designate status of refcounts dirtiness. But we
>>     never clear the flag in the image, and we remember that it is always
>>     set.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json |  5 ++++
>>   block/qcow2.h        |  2 ++
>>   block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++--------
>>   3 files changed, 61 insertions(+), 12 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 9a5a3641d0..3e35357182 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3228,6 +3228,10 @@
>>   # @lazy-refcounts: whether to enable the lazy refcounts
>>   #                  feature (default is taken from the image file)
>>   #
>> +# @keep-dirty: whether to not touch dirty bit. When set, QEMU doesn't
>> +#              check refcounts on qcow2 open (ignoring dirty bit) and doesn't
>> +#              clear dirty bit on qcow2 close. (since 7.0)
>> +#
>>   # @pass-discard-request: whether discard requests to the qcow2
>>   #                        device should be forwarded to the data source
>>   #
>> @@ -3276,6 +3280,7 @@
>>   { 'struct': 'BlockdevOptionsQcow2',
>>     'base': 'BlockdevOptionsGenericCOWFormat',
>>     'data': { '*lazy-refcounts': 'bool',
>> +            '*keep-dirty': 'bool',
>>               '*pass-discard-request': 'bool',
>>               '*pass-discard-snapshot': 'bool',
>>               '*pass-discard-other': 'bool',
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index fd48a89d45..696e13377a 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -130,6 +130,7 @@
>>   
>>   #define QCOW2_OPT_DATA_FILE "data-file"
>>   #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
>> +#define QCOW2_OPT_KEEP_DIRTY "keep-dirty"
>>   #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
>>   #define QCOW2_OPT_DISCARD_SNAPSHOT "pass-discard-snapshot"
>>   #define QCOW2_OPT_DISCARD_OTHER "pass-discard-other"
>> @@ -376,6 +377,7 @@ typedef struct BDRVQcow2State {
>>       int flags;
>>       int qcow_version;
>>       bool use_lazy_refcounts;
>> +    bool keep_dirty;
>>       int refcount_order;
>>       int refcount_bits;
>>       uint64_t refcount_max;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index d509016756..1c42103fb9 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -514,15 +514,17 @@ int qcow2_mark_dirty(BlockDriverState *bs)
>>           return 0; /* already dirty */
>>       }
>>   
>> -    val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
>> -    ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
>> -                      &val, sizeof(val));
>> -    if (ret < 0) {
>> -        return ret;
>> -    }
>> -    ret = bdrv_flush(bs->file->bs);
>> -    if (ret < 0) {
>> -        return ret;
>> +    if (!s->keep_dirty) {
>> +        val = cpu_to_be64(s->incompatible_features | QCOW2_INCOMPAT_DIRTY);
>> +        ret = bdrv_pwrite(bs->file, offsetof(QCowHeader, incompatible_features),
>> +                          &val, sizeof(val));
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +        ret = bdrv_flush(bs->file->bs);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>>       }
>>   
>>       /* Only treat image as dirty if the header was updated successfully */
>> @@ -549,7 +551,13 @@ static int qcow2_mark_clean(BlockDriverState *bs)
>>               return ret;
>>           }
>>   
>> -        return qcow2_update_header(bs);
>> +        if (!s->keep_dirty) {
>> +            /*
>> +             * No reason to update the header if we don't want to clear dirty
>> +             * bit.
>> +             */
>> +            return qcow2_update_header(bs);
>> +        }
>>       }
>>       return 0;
>>   }
>> @@ -709,6 +717,11 @@ static QemuOptsList qcow2_runtime_opts = {
>>               .type = QEMU_OPT_BOOL,
>>               .help = "Postpone refcount updates",
>>           },
>> +        {
>> +            .name = QCOW2_OPT_KEEP_DIRTY,
>> +            .type = QEMU_OPT_BOOL,
>> +            .help = "Keep dirty bit untouched",
>> +        },
>>           {
>>               .name = QCOW2_OPT_DISCARD_REQUEST,
>>               .type = QEMU_OPT_BOOL,
>> @@ -966,6 +979,7 @@ typedef struct Qcow2ReopenState {
>>       Qcow2Cache *refcount_block_cache;
>>       int l2_slice_size; /* Number of entries in a slice of the L2 table */
>>       bool use_lazy_refcounts;
>> +    bool keep_dirty;
>>       int overlap_check;
>>       bool discard_passthrough[QCOW2_DISCARD_MAX];
>>       uint64_t cache_clean_interval;
>> @@ -1088,6 +1102,13 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
>>           }
>>       }
>>   
>> +    r->keep_dirty = qemu_opt_get_bool(opts, QCOW2_OPT_KEEP_DIRTY, false);
>> +    if (r->keep_dirty && !r->use_lazy_refcounts) {
>> +        error_setg(errp, "keep-dirty requires lazy refcounts");
>> +        ret = -EINVAL;
>> +        goto fail;
> 
> Vladimir, according to documentation dirty bit and lazy refcounts are independent:
>                      
>                      incompatible_features:
>                      Bit 0:      Dirty bit.  If this bit is set then refcounts
>                                  may be inconsistent, make sure to scan L1/L2
>                                  tables to repair refcounts before accessing the
>                                  image.
> 
>                      compatible_features:
>                      Bit 0:      Lazy refcounts bit.  If this bit is set then
>                                  lazy refcount updates can be used.  This means
>                                  marking the image file dirty and postponing
>                                  refcount metadata updates.
> 
> while this patch assumes existence of some dependence between them.
> Is there a real reason to introduce such the connection?
> 

Hmm, somehow I thought that there was some documented dependency.. But seems you are right. Honestly, I don't know real scenarios where non-lazy refcounts make sense. In Virtuozzo we always use lazy refcounts turned on by bit in the image. OK, I'll drop a dependency for new option.


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-01-27 10:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-20 16:20 [PATCH 0/2] qcow2: add keep-dirty open option Vladimir Sementsov-Ogievskiy
2022-01-20 16:20 ` [PATCH 1/2] " Vladimir Sementsov-Ogievskiy
2022-01-27  9:43   ` Kirill Tkhai
2022-01-27  9:52     ` Vladimir Sementsov-Ogievskiy
2022-01-20 16:20 ` [PATCH 2/2] iotests: add qcow2-keep-dirty Vladimir Sementsov-Ogievskiy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.