[PATCH 00/14] block: blockdev-del force=false

[PATCH 00/14] block: blockdev-del force=false

[Vladimir Sementsov-Ogievskiy]

Hi all!

Currently close-path in block layer can't fail. Neither bdrv_close() nor
.bdrv_close() handlers have return value. So, when something fails (for
example cached metadata flush) the only thing we can is to print an
error to log.

This behavior is OK on VM stop: we want to stop the running process and
if it can't save metadata, we can do nothing about it. We only rely
on disk format safety: dirty bit or something like this, which will
force some checks on next VM start.

Still there are cases when we _can_ reasonably handle failures on close:

Consider backup: if something failed when we close target qcow2 image,
correct thing to do is to reject the resulting image and consider the
whole backup failed. Still, current behavior is to ignore such failures
which probably leads to creating inconsistent backup images.

Another thing (that actually brings me to creating this series) is dirty
bitmap manipulation: assume we start Qemu (or qemu-storage-daemon) to do
some manipulations with bitmaps and prepare some new qcow2 image with
these bitmaps. Native way to store bitmaps is just to close qcow2 node,
so we either call blockdev-del or just terminate the process. But in
either case there is no way to get the result: is everything stored
correctly or something failed?

So, welcome the new series, that provide a new interface
blockdev-del(force=false) to either get a clean error or be sure that
everything (bitmaps, L2 caches, refcounts) correctly stored and flushed.

Intended usage:
 - blockdev-create / blockdev-add to add a qcow2 node
 - do some operations with it (for example backup job
   or bitmap manipulations)
 - blockdev-del(force=false)
    - If it fails
      - report the error to user
      - repeat the command with force=true (or just
        terminate Qemu process if no more operations to do),
      - _remove_ the prepared qcow2 image from FS
    - If it succeeds
      - Great, now we are sure that qcow2 image prepared as we want and
        correctly closed/saved/flushed. As well, node is removed and
        Qemu will not touch it anymore for sure. So, report success!

The series could be found here:
  git: https://src.openvz.org/scm/~vsementsov/qemu.git  tag  up-block-safe-remove-v1
  browse: https://src.openvz.org/users/vsementsov/repos/qemu/browse?at=refs%2Ftags%2Fup-block-safe-remove-v1

The series is based on my "[PATCH 00/14] block: cleanup backing and file handling"
Based-on: <20211203202553.3231580-1-vsementsov@virtuozzo.com>

Vladimir Sementsov-Ogievskiy (14):
  block: refactor bdrv_remove_file_or_backing_child to bdrv_remove_child
  block: drop bdrv_detach_child()
  block: bdrv_refresh_perms(): allow external tran
  block: add bdrv_try_set_aio_context_tran transaction action
  block: merge bdrv_delete and bdrv_close
  block: bdrv_delete(): drop unnecessary zeroing
  block: implemet bdrv_try_unref()
  qapi/block-core: add 'force' argument to blockdev-del
  qcow2: qcow2_inactivate(): use qcow2_flush_caches()
  qcow2: qcow2_inactivate(): don't call qcow2_mark_clean() when RO
  qcow2: refactor qcow2_inactivate
  qcow2: implement .bdrv_close_safe
  block/file-posix: implement .bdrv_close_safe
  iotests: add test for blockdev-del(force=false)

 qapi/block-core.json                          |   6 +-
 block/qcow2.h                                 |   6 +-
 include/block/block.h                         |   3 +
 include/block/block_int.h                     |   2 +
 block.c                                       | 438 +++++++++++++-----
 block/file-posix.c                            |  38 +-
 block/monitor/block-hmp-cmds.c                |   2 +-
 block/qcow2-bitmap.c                          |   2 +-
 block/qcow2-refcount.c                        |  22 +-
 block/qcow2.c                                 | 152 ++++--
 blockdev.c                                    |  17 +-
 hw/block/xen-block.c                          |   2 +-
 tests/qemu-iotests/026.out                    | 126 ++---
 tests/qemu-iotests/071.out                    |   3 +-
 tests/qemu-iotests/080.out                    |   4 +-
 .../tests/blockdev-del-close-failure          |  54 +++
 .../tests/blockdev-del-close-failure.out      |   4 +
 17 files changed, 611 insertions(+), 270 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/blockdev-del-close-failure
 create mode 100644 tests/qemu-iotests/tests/blockdev-del-close-failure.out

-- 
2.31.1

[PATCH 01/14] block: refactor bdrv_remove_file_or_backing_child to bdrv_remove_child

[Vladimir Sementsov-Ogievskiy]

Now the function can remove any child, so give it more common name.
Drop assertions and drop bs argument which becomes unused. Function
would be reused in a further commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c
index 0fddb57e78..d852daf7c8 100644
--- a/block.c
+++ b/block.c
@@ -89,9 +89,7 @@ static bool bdrv_recurse_has_child(BlockDriverState *bs,
 
 static void bdrv_replace_child_noperm(BdrvChild *child,
                                       BlockDriverState *new_bs);
-static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
-                                              BdrvChild *child,
-                                              Transaction *tran);
+static void bdrv_remove_child(BdrvChild *child, Transaction *tran);
 static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
                                             Transaction *tran);
 
@@ -3236,7 +3234,7 @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
 
     if (child) {
         bdrv_unset_inherits_from(parent_bs, child, tran);
-        bdrv_remove_file_or_backing_child(parent_bs, child, tran);
+        bdrv_remove_child(child, tran);
     }
 
     if (!child_bs) {
@@ -4891,25 +4889,21 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
     return ret;
 }
 
-static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
+static void bdrv_remove_child_commit(void *opaque)
 {
     bdrv_child_free(opaque);
 }
 
-static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
-    .commit = bdrv_remove_filter_or_cow_child_commit,
+static TransactionActionDrv bdrv_remove_child_drv = {
+    .commit = bdrv_remove_child_commit,
 };
 
 /*
  * A function to remove backing or file child of @bs.
  * Function doesn't update permissions, caller is responsible for this.
  */
-static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
-                                              BdrvChild *child,
-                                              Transaction *tran)
+static void bdrv_remove_child(BdrvChild *child, Transaction *tran)
 {
-    assert(child == bs->backing || child == bs->file);
-
     if (!child) {
         return;
     }
@@ -4918,7 +4912,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
         bdrv_replace_child_tran(child, NULL, tran);
     }
 
-    tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, child);
+    tran_add(tran, &bdrv_remove_child_drv, child);
 }
 
 /*
@@ -4929,7 +4923,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
 static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,
                                             Transaction *tran)
 {
-    bdrv_remove_file_or_backing_child(bs, bdrv_filter_or_cow_child(bs), tran);
+    bdrv_remove_child(bdrv_filter_or_cow_child(bs), tran);
 }
 
 static int bdrv_replace_node_noperm(BlockDriverState *from,
-- 
2.31.1

[PATCH 02/14] block: drop bdrv_detach_child()

[Vladimir Sementsov-Ogievskiy]

The only caller is bdrv_root_unref_child(), let's just do the logic
directly in it. It simplifies further convertion of
bdrv_root_unref_child() to transaction action.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index d852daf7c8..378841a546 100644
--- a/block.c
+++ b/block.c
@@ -2948,29 +2948,6 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
     return 0;
 }
 
-static void bdrv_detach_child(BdrvChild *child)
-{
-    BlockDriverState *old_bs = child->bs;
-
-    bdrv_replace_child_noperm(child, NULL);
-    bdrv_child_free(child);
-
-    if (old_bs) {
-        /*
-         * Update permissions for old node. We're just taking a parent away, so
-         * we're loosening restrictions. Errors of permission update are not
-         * fatal in this case, ignore them.
-         */
-        bdrv_refresh_perms(old_bs, NULL);
-
-        /*
-         * When the parent requiring a non-default AioContext is removed, the
-         * node moves back to the main AioContext
-         */
-        bdrv_try_set_aio_context(old_bs, qemu_get_aio_context(), NULL);
-    }
-}
-
 /*
  * This function steals the reference to child_bs from the caller.
  * That reference is later dropped by bdrv_root_unref_child().
@@ -3053,10 +3030,26 @@ out:
 /* Callers must ensure that child->frozen is false. */
 void bdrv_root_unref_child(BdrvChild *child)
 {
-    BlockDriverState *child_bs;
+    BlockDriverState *child_bs = child->bs;
+
+    bdrv_replace_child_noperm(child, NULL);
+    bdrv_child_free(child);
+
+    if (child_bs) {
+        /*
+         * Update permissions for old node. We're just taking a parent away, so
+         * we're loosening restrictions. Errors of permission update are not
+         * fatal in this case, ignore them.
+         */
+        bdrv_refresh_perms(child_bs, NULL);
+
+        /*
+         * When the parent requiring a non-default AioContext is removed, the
+         * node moves back to the main AioContext
+         */
+        bdrv_try_set_aio_context(child_bs, qemu_get_aio_context(), NULL);
+    }
 
-    child_bs = child->bs;
-    bdrv_detach_child(child);
     bdrv_unref(child_bs);
 }
 
-- 
2.31.1

[PATCH 03/14] block: bdrv_refresh_perms(): allow external tran

[Vladimir Sementsov-Ogievskiy]

Allow passing external Transaction pointer, stop creating extra
Transaction objects.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 378841a546..7b65c7c5c0 100644
--- a/block.c
+++ b/block.c
@@ -2475,14 +2475,23 @@ char *bdrv_perm_names(uint64_t perm)
 }
 
 
-static int bdrv_refresh_perms(BlockDriverState *bs, Error **errp)
+/* @tran is allowed to be NULL. In this case no rollback is possible */
+static int bdrv_refresh_perms(BlockDriverState *bs, Transaction *tran,
+                              Error **errp)
 {
     int ret;
-    Transaction *tran = tran_new();
+    Transaction *local_tran = NULL;
     g_autoptr(GSList) list = bdrv_topological_dfs(NULL, NULL, bs);
 
+    if (!tran) {
+        tran = local_tran = tran_new();
+    }
+
     ret = bdrv_list_refresh_perms(list, NULL, tran, errp);
-    tran_finalize(tran, ret);
+
+    if (local_tran) {
+        tran_finalize(local_tran, ret);
+    }
 
     return ret;
 }
@@ -2496,7 +2505,7 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
 
     bdrv_child_set_perm(c, perm, shared, tran);
 
-    ret = bdrv_refresh_perms(c->bs, &local_err);
+    ret = bdrv_refresh_perms(c->bs, tran, &local_err);
 
     tran_finalize(tran, ret);
 
@@ -2976,7 +2985,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
         goto out;
     }
 
-    ret = bdrv_refresh_perms(child_bs, errp);
+    ret = bdrv_refresh_perms(child_bs, tran, errp);
 
 out:
     tran_finalize(tran, ret);
@@ -3014,7 +3023,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
         goto out;
     }
 
-    ret = bdrv_refresh_perms(parent_bs, errp);
+    ret = bdrv_refresh_perms(parent_bs, tran, errp);
     if (ret < 0) {
         goto out;
     }
@@ -3041,7 +3050,7 @@ void bdrv_root_unref_child(BdrvChild *child)
          * we're loosening restrictions. Errors of permission update are not
          * fatal in this case, ignore them.
          */
-        bdrv_refresh_perms(child_bs, NULL);
+        bdrv_refresh_perms(child_bs, NULL, NULL);
 
         /*
          * When the parent requiring a non-default AioContext is removed, the
@@ -3277,7 +3286,7 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
         goto out;
     }
 
-    ret = bdrv_refresh_perms(bs, errp);
+    ret = bdrv_refresh_perms(bs, tran, errp);
 out:
     tran_finalize(tran, ret);
 
@@ -5067,7 +5076,7 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
         goto out;
     }
 
-    ret = bdrv_refresh_perms(bs_new, errp);
+    ret = bdrv_refresh_perms(bs_new, tran, errp);
 out:
     tran_finalize(tran, ret);
 
@@ -6297,7 +6306,7 @@ int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
      */
     if (bs->open_flags & BDRV_O_INACTIVE) {
         bs->open_flags &= ~BDRV_O_INACTIVE;
-        ret = bdrv_refresh_perms(bs, errp);
+        ret = bdrv_refresh_perms(bs, NULL, errp);
         if (ret < 0) {
             bs->open_flags |= BDRV_O_INACTIVE;
             return ret;
@@ -6422,7 +6431,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
      * We only tried to loosen restrictions, so errors are not fatal, ignore
      * them.
      */
-    bdrv_refresh_perms(bs, NULL);
+    bdrv_refresh_perms(bs, NULL, NULL);
 
     /* Recursively inactivate children */
     QLIST_FOREACH(child, &bs->children, next) {
-- 
2.31.1

[PATCH 04/14] block: add bdrv_try_set_aio_context_tran transaction action

[Vladimir Sementsov-Ogievskiy]

To be used in further commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/block.c b/block.c
index 7b65c7c5c0..82fbf81a3c 100644
--- a/block.c
+++ b/block.c
@@ -2791,6 +2791,54 @@ static void bdrv_child_free(BdrvChild *child)
     g_free(child);
 }
 
+typedef struct BdrvTrySetAioContextState {
+    BlockDriverState *bs;
+    AioContext *old_ctx;
+} BdrvTrySetAioContextState;
+
+static void bdrv_try_set_aio_context_abort(void *opaque)
+{
+    BdrvTrySetAioContextState *s = opaque;
+
+    if (bdrv_get_aio_context(s->bs) != s->old_ctx) {
+        bdrv_try_set_aio_context(s->bs, s->old_ctx, &error_abort);
+    }
+}
+
+static TransactionActionDrv bdrv_try_set_aio_context_drv = {
+    .abort = bdrv_try_set_aio_context_abort,
+    .clean = g_free,
+};
+
+__attribute__((unused))
+static int bdrv_try_set_aio_context_tran(BlockDriverState *bs,
+                                         AioContext *new_ctx,
+                                         Transaction *tran,
+                                         Error **errp)
+{
+    AioContext *old_ctx = bdrv_get_aio_context(bs);
+    BdrvTrySetAioContextState *s;
+    int ret;
+
+    if (old_ctx == new_ctx) {
+        return 0;
+    }
+
+    ret = bdrv_try_set_aio_context(bs, new_ctx, errp);
+    if (ret < 0) {
+        return ret;
+    }
+
+    s = g_new(BdrvTrySetAioContextState, 1);
+    *s = (BdrvTrySetAioContextState) {
+        .bs = bs,
+        .old_ctx = old_ctx,
+    };
+    tran_add(tran, &bdrv_try_set_aio_context_drv, s);
+
+    return 0;
+}
+
 typedef struct BdrvAttachChildCommonState {
     BdrvChild *child;
     AioContext *old_parent_ctx;
-- 
2.31.1

[PATCH 05/14] block: merge bdrv_delete and bdrv_close

[Vladimir Sementsov-Ogievskiy]

bdrv_delete() is the only caller of bdrv_close(). Let's merge them to
simplify further commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 82fbf81a3c..71a5aec24c 100644
--- a/block.c
+++ b/block.c
@@ -4785,12 +4785,19 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 }
 
 
-static void bdrv_close(BlockDriverState *bs)
+static void bdrv_delete(BlockDriverState *bs)
 {
     BdrvAioNotifier *ban, *ban_next;
     BdrvChild *child, *next;
 
     assert(!bs->refcnt);
+    assert(bdrv_op_blocker_is_empty(bs));
+
+    /* remove from list, if necessary */
+    if (bs->node_name[0] != '\0') {
+        QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
+    }
+    QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list);
 
     bdrv_drained_begin(bs); /* complete I/O */
     bdrv_flush(bs);
@@ -4844,6 +4851,8 @@ static void bdrv_close(BlockDriverState *bs)
     if (bs->quiesce_counter) {
         bdrv_drain_all_end_quiesce(bs);
     }
+
+    g_free(bs);
 }
 
 void bdrv_close_all(void)
@@ -5164,22 +5173,6 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
     return ret;
 }
 
-static void bdrv_delete(BlockDriverState *bs)
-{
-    assert(bdrv_op_blocker_is_empty(bs));
-    assert(!bs->refcnt);
-
-    /* remove from list, if necessary */
-    if (bs->node_name[0] != '\0') {
-        QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
-    }
-    QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list);
-
-    bdrv_close(bs);
-
-    g_free(bs);
-}
-
 
 /*
  * Replace @bs by newly created block node.
-- 
2.31.1

[PATCH 06/14] block: bdrv_delete(): drop unnecessary zeroing

[Vladimir Sementsov-Ogievskiy]

No need to zero all these things before g_free(bs). Move memory freeing
to the end of the function to simplify further conversion to
transaction action.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 71a5aec24c..231d1fc3ea 100644
--- a/block.c
+++ b/block.c
@@ -4815,32 +4815,6 @@ static void bdrv_delete(BlockDriverState *bs)
         bdrv_unref_child(bs, child);
     }
 
-    assert(!bs->backing);
-    assert(!bs->file);
-    g_free(bs->opaque);
-    bs->opaque = NULL;
-    qatomic_set(&bs->copy_on_read, 0);
-    bs->backing_file[0] = '\0';
-    bs->backing_format[0] = '\0';
-    bs->total_sectors = 0;
-    bs->encrypted = false;
-    bs->sg = false;
-    qobject_unref(bs->options);
-    qobject_unref(bs->explicit_options);
-    bs->options = NULL;
-    bs->explicit_options = NULL;
-    qobject_unref(bs->full_open_options);
-    bs->full_open_options = NULL;
-    g_free(bs->block_status_cache);
-    bs->block_status_cache = NULL;
-
-    bdrv_release_named_dirty_bitmaps(bs);
-    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
-
-    QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_next) {
-        g_free(ban);
-    }
-    QLIST_INIT(&bs->aio_notifiers);
     bdrv_drained_end(bs);
 
     /*
@@ -4852,6 +4826,20 @@ static void bdrv_delete(BlockDriverState *bs)
         bdrv_drain_all_end_quiesce(bs);
     }
 
+    /* Free memory */
+    g_free(bs->opaque);
+    qobject_unref(bs->options);
+    qobject_unref(bs->explicit_options);
+    qobject_unref(bs->full_open_options);
+    g_free(bs->block_status_cache);
+
+    bdrv_release_named_dirty_bitmaps(bs);
+    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
+
+    QLIST_FOREACH_SAFE(ban, &bs->aio_notifiers, list, ban_next) {
+        g_free(ban);
+    }
+
     g_free(bs);
 }
 
-- 
2.31.1

[PATCH 07/14] block: implemet bdrv_try_unref()

[Vladimir Sementsov-Ogievskiy]

Make a version of bdrv_unref() that honestly report any failure.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h     |   1 +
 include/block/block_int.h |   2 +
 block.c                   | 247 +++++++++++++++++++++++++++++++-------
 3 files changed, 208 insertions(+), 42 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 768273b2db..42d78a7a31 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -672,6 +672,7 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
+int bdrv_try_unref(BlockDriverState *bs, Error **errp);
 BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              BlockDriverState *child_bs,
                              const char *child_name,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 767825aec4..3126868633 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -188,6 +188,8 @@ struct BlockDriver {
     int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
+    int (*bdrv_close_safe)(BlockDriverState *bs, Transaction *tran,
+                           Error **errp);
 
 
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
diff --git a/block.c b/block.c
index 231d1fc3ea..187732c6f8 100644
--- a/block.c
+++ b/block.c
@@ -101,6 +101,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state);
 
 static bool bdrv_backing_overridden(BlockDriverState *bs);
 
+static int bdrv_unref_safe(BlockDriverState *bs, Transaction *tran,
+                           Error **errp);
+
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
 
@@ -3084,30 +3087,60 @@ out:
     return ret < 0 ? NULL : child;
 }
 
-/* Callers must ensure that child->frozen is false. */
-void bdrv_root_unref_child(BdrvChild *child)
+/*
+ * When @tran is NULL, function never fail and returns 0. Still, some states
+ * may not be saved correctly.
+ *
+ * When @tran is not NULL, first failure is returned and the action may be
+ * rolled back.
+ */
+static int bdrv_root_unref_child_safe(BdrvChild *child, Transaction *tran,
+                                      Error **errp)
 {
+    int ret;
     BlockDriverState *child_bs = child->bs;
 
-    bdrv_replace_child_noperm(child, NULL);
-    bdrv_child_free(child);
+    if (tran) {
+        bdrv_remove_child(child, tran);
+    } else {
+        bdrv_replace_child_noperm(child, NULL);
+        bdrv_child_free(child);
+    }
 
     if (child_bs) {
         /*
          * Update permissions for old node. We're just taking a parent away, so
          * we're loosening restrictions. Errors of permission update are not
-         * fatal in this case, ignore them.
+         * fatal in this case, ignore them when tran is NULL.
          */
-        bdrv_refresh_perms(child_bs, NULL, NULL);
+        ret = bdrv_refresh_perms(child_bs, tran, tran ? errp : NULL);
+        if (tran && ret < 0) {
+            return ret;
+        }
 
         /*
          * When the parent requiring a non-default AioContext is removed, the
          * node moves back to the main AioContext
          */
-        bdrv_try_set_aio_context(child_bs, qemu_get_aio_context(), NULL);
+        if (tran) {
+            ret = bdrv_try_set_aio_context_tran(child_bs,
+                                                qemu_get_aio_context(),
+                                                tran, errp);
+            if (ret < 0) {
+                return ret;
+            }
+        } else {
+            bdrv_try_set_aio_context(child_bs, qemu_get_aio_context(), NULL);
+        }
     }
 
-    bdrv_unref(child_bs);
+    return bdrv_unref_safe(child_bs, tran, errp);
+}
+
+/* Callers must ensure that child->frozen is false. */
+void bdrv_root_unref_child(BdrvChild *child)
+{
+    bdrv_root_unref_child_safe(child, NULL, &error_abort);
 }
 
 typedef struct BdrvSetInheritsFrom {
@@ -3176,15 +3209,28 @@ static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child,
     }
 }
 
+/*
+ * When @tran is NULL, function never fail and returns 0. Still, some states
+ * may not be saved correctly.
+ *
+ * When @tran is not NULL, first failure is returned and the action may be
+ * rolled back.
+ */
+static int bdrv_unref_child_safe(BlockDriverState *parent, BdrvChild *child,
+                                 Transaction *tran, Error **errp)
+{
+    if (child == NULL) {
+        return 0;
+    }
+
+    bdrv_unset_inherits_from(parent, child, tran);
+    return bdrv_root_unref_child_safe(child, tran, errp);
+}
+
 /* Callers must ensure that child->frozen is false. */
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child)
 {
-    if (child == NULL) {
-        return;
-    }
-
-    bdrv_unset_inherits_from(parent, child, NULL);
-    bdrv_root_unref_child(child);
+    bdrv_unref_child_safe(parent, child, NULL, &error_abort);
 }
 
 
@@ -4784,14 +4830,17 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
     }
 }
 
+static void bdrv_delete_abort(void *opaque)
+{
+    BlockDriverState *bs = opaque;
 
-static void bdrv_delete(BlockDriverState *bs)
+    bdrv_drained_end(bs);
+}
+
+static void bdrv_delete_commit(void *opaque)
 {
     BdrvAioNotifier *ban, *ban_next;
-    BdrvChild *child, *next;
-
-    assert(!bs->refcnt);
-    assert(bdrv_op_blocker_is_empty(bs));
+    BlockDriverState *bs = opaque;
 
     /* remove from list, if necessary */
     if (bs->node_name[0] != '\0') {
@@ -4799,22 +4848,6 @@ static void bdrv_delete(BlockDriverState *bs)
     }
     QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list);
 
-    bdrv_drained_begin(bs); /* complete I/O */
-    bdrv_flush(bs);
-    bdrv_drain(bs); /* in case flush left pending I/O */
-
-    if (bs->drv) {
-        if (bs->drv->bdrv_close) {
-            /* Must unfreeze all children, so bdrv_unref_child() works */
-            bs->drv->bdrv_close(bs);
-        }
-        bs->drv = NULL;
-    }
-
-    QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
-        bdrv_unref_child(bs, child);
-    }
-
     bdrv_drained_end(bs);
 
     /*
@@ -4843,6 +4876,88 @@ static void bdrv_delete(BlockDriverState *bs)
     g_free(bs);
 }
 
+static TransactionActionDrv bdrv_delete_drv = {
+    .commit = bdrv_delete_commit,
+    .abort = bdrv_delete_abort,
+};
+
+/*
+ * When @tran is NULL, function never fail and returns 0. Still, some states
+ * may not be saved correctly.
+ *
+ * When @tran is not NULL, first failure is returned and the action may be
+ * rolled back.
+ */
+static int bdrv_delete(BlockDriverState *bs, Transaction *tran, Error **errp)
+{
+    int ret;
+    BdrvChild *child, *next;
+
+    assert(!bs->refcnt);
+    assert(bdrv_op_blocker_is_empty(bs));
+
+    assert(!(bs->drv && bs->drv->bdrv_close_safe && bs->drv->bdrv_close));
+
+    if (tran && bs->drv && bs->drv->bdrv_close) {
+        /* .bdrv_close() is unsafe handler */
+        error_setg(errp, "Node '%s'(%s) doesn't support safe removing",
+                   bdrv_get_node_name(bs), bdrv_get_format_name(bs));
+        return -EINVAL;
+    }
+
+    if (tran && !bs->drv) {
+        /* Node without driver is a sign of something wrong */
+        error_setg(errp, "Node '%s' is broken", bdrv_get_node_name(bs));
+        return -EINVAL;
+    }
+
+    /* complete I/O */
+    bdrv_drained_begin(bs);
+    if (tran) {
+        /* Add it now, as we want bdrv_drained_end() on abort */
+        tran_add(tran, &bdrv_delete_drv, bs);
+    }
+
+    ret = bdrv_flush(bs);
+    if (ret < 0 && tran) {
+        error_setg(errp, "Failed to flush node '%s'", bdrv_get_node_name(bs));
+        return ret;
+    }
+
+    bdrv_drain(bs); /* in case flush left pending I/O */
+
+    /*
+     * .bdrv_close[_safe] Must unfreeze all children, so bdrv_unref_child()
+     * works.
+     */
+    if (bs->drv) {
+        if (bs->drv->bdrv_close) {
+            assert(!tran);
+            bs->drv->bdrv_close(bs);
+        } else if (bs->drv->bdrv_close_safe) {
+            ret = bs->drv->bdrv_close_safe(bs, tran, errp);
+            if (ret < 0) {
+                assert(tran);
+                return ret;
+            }
+        }
+    }
+
+    QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
+        ret = bdrv_unref_child_safe(bs, child, tran, errp);
+        if (ret < 0) {
+            assert(tran);
+            return ret;
+        }
+    }
+
+    if (!tran) {
+        bdrv_delete_commit(bs);
+    }
+
+    return 0;
+}
+
 void bdrv_close_all(void)
 {
     assert(job_next(NULL) == NULL);
@@ -6571,18 +6686,66 @@ void bdrv_ref(BlockDriverState *bs)
     bs->refcnt++;
 }
 
+static void bdrv_unref_safe_abort(void *opaque)
+{
+    bdrv_ref(opaque);
+}
+
+static TransactionActionDrv bdrv_unref_safe_drv = {
+    .abort = bdrv_unref_safe_abort,
+};
+
+/*
+ * When @tran is NULL, function never fail and returns 0. Still, some states
+ * may not be saved correctly.
+ *
+ * When @tran is not NULL, first failure is returned and the action may be
+ * rolled back.
+ */
+static int bdrv_unref_safe(BlockDriverState *bs, Transaction *tran,
+                           Error **errp)
+{
+    if (!bs) {
+        return 0;
+    }
+
+    assert(bs->refcnt > 0);
+
+    if (tran) {
+        tran_add(tran, &bdrv_unref_safe_drv, bs);
+    }
+
+    if (--bs->refcnt == 0) {
+        return bdrv_delete(bs, tran, errp);
+    }
+
+    return 0;
+}
+
 /* Release a previously grabbed reference to bs.
  * If after releasing, reference count is zero, the BlockDriverState is
  * deleted. */
 void bdrv_unref(BlockDriverState *bs)
 {
-    if (!bs) {
-        return;
-    }
-    assert(bs->refcnt > 0);
-    if (--bs->refcnt == 0) {
-        bdrv_delete(bs);
-    }
+    bdrv_unref_safe(bs, NULL, &error_abort);
+}
+
+/*
+ * Like bdrv_unref(), but don't ignore errors:
+ * On success, if node (nodes) were removed, it's guaranteed that all states
+ * are stored correctly (for example, metadata caches, persistent dirty
+ * bitmaps).
+ * On failure every change is rolled back, node is not unref'ed.
+ */
+int bdrv_try_unref(BlockDriverState *bs, Error **errp)
+{
+    int ret;
+    Transaction *tran = tran_new();
+
+    ret = bdrv_unref_safe(bs, tran, errp);
+    tran_finalize(tran, ret);
+
+    return ret;
 }
 
 struct BdrvOpBlocker {
-- 
2.31.1

[PATCH 08/14] qapi/block-core: add 'force' argument to blockdev-del

[Vladimir Sementsov-Ogievskiy]

Default behavior is force=true and it's unchanged. New behavior is
force=false, which makes it possible to be sure that node removal is
done successfully with no error and all metadata is stored and flushed
successfully.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json           |  6 +++++-
 block/monitor/block-hmp-cmds.c |  2 +-
 blockdev.c                     | 17 +++++++++++++++--
 hw/block/xen-block.c           |  2 +-
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9a5a3641d0..b37d195772 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4403,6 +4403,9 @@
 #
 # @node-name: Name of the graph node to delete.
 #
+# @force: Ignore failures when closing block-nodes, like failed IO
+#         when try to store metadata. Default true. (Since 7.0)
+#
 # Since: 2.9
 #
 # Example:
@@ -4425,7 +4428,8 @@
 # <- { "return": {} }
 #
 ##
-{ 'command': 'blockdev-del', 'data': { 'node-name': 'str' } }
+{ 'command': 'blockdev-del',
+  'data': { 'node-name': 'str', '*force': 'bool' } }
 
 ##
 # @BlockdevCreateOptionsFile:
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bfb3c043a0..1c35aa2d6f 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -145,7 +145,7 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
 
     bs = bdrv_find_node(id);
     if (bs) {
-        qmp_blockdev_del(id, &local_err);
+        qmp_blockdev_del(id, true, true, &local_err);
         if (local_err) {
             error_report_err(local_err);
         }
diff --git a/blockdev.c b/blockdev.c
index 8197165bb5..34a195b592 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3582,11 +3582,17 @@ fail:
     g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
 }
 
-void qmp_blockdev_del(const char *node_name, Error **errp)
+void qmp_blockdev_del(const char *node_name, bool has_force,
+                      bool force, Error **errp)
 {
     AioContext *aio_context;
     BlockDriverState *bs;
 
+    if (!has_force) {
+        /* Historical default is force remove */
+        force = true;
+    }
+
     bs = bdrv_find_node(node_name);
     if (!bs) {
         error_setg(errp, "Failed to find node with node-name='%s'", node_name);
@@ -3616,7 +3622,14 @@ void qmp_blockdev_del(const char *node_name, Error **errp)
     }
 
     QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
-    bdrv_unref(bs);
+    if (force) {
+        bdrv_unref(bs);
+    } else {
+        int ret = bdrv_try_unref(bs, errp);
+        if (ret < 0) {
+            bdrv_set_monitor_owned(bs);
+        }
+    }
 
 out:
     aio_context_release(aio_context);
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 674953f1ad..0ac9b599c0 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -649,7 +649,7 @@ static void xen_block_blockdev_del(const char *node_name, Error **errp)
 {
     trace_xen_block_blockdev_del(node_name);
 
-    qmp_blockdev_del(node_name, errp);
+    qmp_blockdev_del(node_name, true, true, errp);
 }
 
 static char *xen_block_blockdev_add(const char *id, QDict *qdict,
-- 
2.31.1

[PATCH 09/14] qcow2: qcow2_inactivate(): use qcow2_flush_caches()

[Vladimir Sementsov-Ogievskiy]

Maintaining similar logic of flushing caches in both
qcow2_inactivate() and in qcow2_flush_caches() is not good.

Let's refactor things to use qcow2_flush_caches() directly from
qcow2_inactivate(). For this we need two things: textual error messages
(add Error **) and possibility to unconditionally flush refcounts (add
force_refcounts argument).

Iotests output updated correspondingly.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h              |   6 +-
 block/qcow2-bitmap.c       |   2 +-
 block/qcow2-refcount.c     |  22 +++++--
 block/qcow2.c              |  25 +++-----
 tests/qemu-iotests/026.out | 126 +++++++++++++------------------------
 tests/qemu-iotests/071.out |   3 +-
 tests/qemu-iotests/080.out |   4 +-
 7 files changed, 76 insertions(+), 112 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index fd48a89d45..a83183d533 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -874,8 +874,10 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
 int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     int64_t l1_table_offset, int l1_size, int addend);
 
-int coroutine_fn qcow2_flush_caches(BlockDriverState *bs);
-int coroutine_fn qcow2_write_caches(BlockDriverState *bs);
+int coroutine_fn qcow2_flush_caches(BlockDriverState *bs, bool force_refcounts,
+                                    Error **errp);
+int coroutine_fn qcow2_write_caches(BlockDriverState *bs, bool force_refcounts,
+                                    Error **errp);
 int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                           BdrvCheckMode fix);
 
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 8fb4731551..0f463ced4e 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -899,7 +899,7 @@ static int update_ext_header_and_dir(BlockDriverState *bs,
             return ret;
         }
 
-        ret = qcow2_flush_caches(bs);
+        ret = qcow2_flush_caches(bs, false, NULL);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 4614572252..c8251412ce 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1203,19 +1203,23 @@ void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
     }
 }
 
-int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
+int coroutine_fn qcow2_write_caches(BlockDriverState *bs, bool force_refcounts,
+                                    Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     int ret;
 
     ret = qcow2_cache_write(bs, s->l2_table_cache);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to write the L2 table cache");
         return ret;
     }
 
-    if (qcow2_need_accurate_refcounts(s)) {
+    if (force_refcounts || qcow2_need_accurate_refcounts(s)) {
         ret = qcow2_cache_write(bs, s->refcount_block_cache);
         if (ret < 0) {
+            error_setg_errno(errp, -ret,
+                             "Failed to write the refcount block cache");
             return ret;
         }
     }
@@ -1223,14 +1227,22 @@ int coroutine_fn qcow2_write_caches(BlockDriverState *bs)
     return 0;
 }
 
-int coroutine_fn qcow2_flush_caches(BlockDriverState *bs)
+int coroutine_fn qcow2_flush_caches(BlockDriverState *bs, bool force_refcounts,
+                                    Error **errp)
 {
-    int ret = qcow2_write_caches(bs);
+    int ret = qcow2_write_caches(bs, force_refcounts, errp);
     if (ret < 0) {
         return ret;
     }
 
-    return bdrv_flush(bs->file->bs);
+    ret = bdrv_flush(bs->file->bs);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "Failed to flush after writing caches");
+        return ret;
+    }
+
+    return 0;
 }
 
 /*********************************************************/
diff --git a/block/qcow2.c b/block/qcow2.c
index 614df0307f..04994df240 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -544,7 +544,7 @@ static int qcow2_mark_clean(BlockDriverState *bs)
 
         s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY;
 
-        ret = qcow2_flush_caches(bs);
+        ret = qcow2_flush_caches(bs, false, NULL);
         if (ret < 0) {
             return ret;
         }
@@ -574,7 +574,7 @@ int qcow2_mark_consistent(BlockDriverState *bs)
     BDRVQcow2State *s = bs->opaque;
 
     if (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT) {
-        int ret = qcow2_flush_caches(bs);
+        int ret = qcow2_flush_caches(bs, false, NULL);
         if (ret < 0) {
             return ret;
         }
@@ -2680,7 +2680,6 @@ fail_nometa:
 
 static int qcow2_inactivate(BlockDriverState *bs)
 {
-    BDRVQcow2State *s = bs->opaque;
     int ret, result = 0;
     Error *local_err = NULL;
 
@@ -2690,20 +2689,14 @@ static int qcow2_inactivate(BlockDriverState *bs)
         error_reportf_err(local_err, "Lost persistent bitmaps during "
                           "inactivation of node '%s': ",
                           bdrv_get_device_or_node_name(bs));
+        local_err = NULL;
     }
 
-    ret = qcow2_cache_flush(bs, s->l2_table_cache);
-    if (ret) {
+    ret = qcow2_flush_caches(bs, true, &local_err);
+    if (ret < 0) {
         result = ret;
-        error_report("Failed to flush the L2 table cache: %s",
-                     strerror(-ret));
-    }
-
-    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
-    if (ret) {
-        result = ret;
-        error_report("Failed to flush the refcount block cache: %s",
-                     strerror(-ret));
+        error_report_err(local_err);
+        local_err = NULL;
     }
 
     if (result == 0) {
@@ -4517,7 +4510,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 
     if (prealloc != PREALLOC_MODE_OFF) {
         /* Flush metadata before actually changing the image size */
-        ret = qcow2_write_caches(bs);
+        ret = qcow2_write_caches(bs, false, NULL);
         if (ret < 0) {
             error_setg_errno(errp, -ret,
                              "Failed to flush the preallocated area to disk");
@@ -4936,7 +4929,7 @@ static coroutine_fn int qcow2_co_flush_to_os(BlockDriverState *bs)
     int ret;
 
     qemu_co_mutex_lock(&s->lock);
-    ret = qcow2_write_caches(bs);
+    ret = qcow2_write_caches(bs, false, NULL);
     qemu_co_mutex_unlock(&s->lock);
 
     return ret;
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 83989996ff..cdc1ed2748 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -14,15 +14,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -38,15 +36,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_update; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -126,15 +122,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the L2 table cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the L2 table cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -150,15 +144,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_update; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -174,15 +166,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_alloc_write; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_alloc_write; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -198,15 +188,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_alloc_write; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l2_alloc_write; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -222,15 +210,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: write_aio; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: write_aio; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -246,15 +232,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: write_aio; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: write_aio; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -270,15 +254,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_load; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_load; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -294,15 +276,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_load; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_load; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -318,15 +298,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -342,15 +320,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_update_part; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -366,15 +342,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc; errno: 5; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc; errno: 5; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -390,15 +364,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -457,15 +429,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_hookup; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_hookup; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -481,15 +451,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_write; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_write; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -505,15 +473,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_write_blocks; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_write_blocks; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -529,15 +495,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_write_table; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_write_table; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -553,15 +517,13 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_switch_table; errno: 28; imm: off; once: off; write
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: refblock_alloc_switch_table; errno: 28; imm: off; once: off; write -b
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the L2 table cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 
@@ -595,8 +557,7 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_grow_write_table; errno: 5; imm: off; once: off
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -607,8 +568,7 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_grow_write_table; errno: 28; imm: off; once: off
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -619,8 +579,7 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_grow_activate_table; errno: 5; imm: off; once: off
-qemu-io: Failed to flush the L2 table cache: Input/output error
-qemu-io: Failed to flush the refcount block cache: Input/output error
+qemu-io: Failed to write the refcount block cache: Input/output error
 write failed: Input/output error
 No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
@@ -631,8 +590,7 @@ No errors were found on the image.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
 
 Event: l1_grow_activate_table; errno: 28; imm: off; once: off
-qemu-io: Failed to flush the L2 table cache: No space left on device
-qemu-io: Failed to flush the refcount block cache: No space left on device
+qemu-io: Failed to write the refcount block cache: No space left on device
 write failed: No space left on device
 No errors were found on the image.
 
diff --git a/tests/qemu-iotests/071.out b/tests/qemu-iotests/071.out
index bca0c02f5c..de1c59154d 100644
--- a/tests/qemu-iotests/071.out
+++ b/tests/qemu-iotests/071.out
@@ -86,7 +86,6 @@ read failed: Input/output error
 {"return": ""}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
-QEMU_PROG: Failed to flush the L2 table cache: Input/output error
-QEMU_PROG: Failed to flush the refcount block cache: Input/output error
+QEMU_PROG: Failed to flush after writing caches: Input/output error
 
 *** done
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 45ab01db8e..860228f93b 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -66,7 +66,7 @@ wrote 512/512 bytes at offset 0
 qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid
 qemu-img: Snapshot L1 table offset invalid
 qemu-img: Failed to turn zero into data clusters: Invalid argument
-qemu-io: Failed to flush the refcount block cache: Invalid argument
+qemu-io: Failed to write the refcount block cache: Invalid argument
 write failed: Invalid argument
 qemu-img: Snapshot L1 table offset invalid
 qemu-img: Could not apply snapshot 'test': Failed to load snapshot: Invalid argument
@@ -89,7 +89,7 @@ wrote 512/512 bytes at offset 0
 qemu-img: Failed to load snapshot: Snapshot L1 table too large
 qemu-img: Snapshot L1 table too large
 qemu-img: Failed to turn zero into data clusters: File too large
-qemu-io: Failed to flush the refcount block cache: File too large
+qemu-io: Failed to write the refcount block cache: File too large
 write failed: File too large
 qemu-img: Snapshot L1 table too large
 qemu-img: Could not apply snapshot 'test': Failed to load snapshot: File too large
-- 
2.31.1

[PATCH 10/14] qcow2: qcow2_inactivate(): don't call qcow2_mark_clean() when RO

[Vladimir Sementsov-Ogievskiy]

qcow2_inactivate() prints errors on different failures, let's not
exclude qcow2_mark_clean() call.

Still, if image is read-only, no reason to report failed write and no
reason even to try write. Write failure is possible when we open dirty
image for check in read-only mode. Let's not do it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 04994df240..ccfcd0db05 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2699,8 +2699,12 @@ static int qcow2_inactivate(BlockDriverState *bs)
         local_err = NULL;
     }
 
-    if (result == 0) {
-        qcow2_mark_clean(bs);
+    if (result == 0 && bdrv_is_writable(bs)) {
+        ret = qcow2_mark_clean(bs);
+        if (ret < 0) {
+            error_report("Failed to mark qcow2 node '%s' clean",
+                         bdrv_get_device_or_node_name(bs));
+        }
     }
 
     return result;
-- 
2.31.1

[PATCH 11/14] qcow2: refactor qcow2_inactivate

[Vladimir Sementsov-Ogievskiy]

Add a possibility for safe behavior: stop on first error end report in
in errp.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ccfcd0db05..8ad555feb7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2678,13 +2678,29 @@ fail_nometa:
     return ret;
 }
 
-static int qcow2_inactivate(BlockDriverState *bs)
+/*
+ * With force=true, ignore failures and just print them. Don't set errp, but
+ * still return a value < 0 in case when something failed.
+ *
+ * With force=false, return error < 0 and set errp on first failure. Nothing is
+ * printed.
+ */
+static int qcow2_do_inactivate(BlockDriverState *bs, bool force, Error **errp)
 {
     int ret, result = 0;
+    bool ok;
     Error *local_err = NULL;
 
-    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
-    if (local_err != NULL) {
+    if (force) {
+        errp = &local_err;
+    }
+
+    ok = qcow2_store_persistent_dirty_bitmaps(bs, true, errp);
+    if (!ok) {
+        if (!force) {
+            return -EINVAL;
+        }
+        assert(local_err);
         result = -EINVAL;
         error_reportf_err(local_err, "Lost persistent bitmaps during "
                           "inactivation of node '%s': ",
@@ -2692,8 +2708,11 @@ static int qcow2_inactivate(BlockDriverState *bs)
         local_err = NULL;
     }
 
-    ret = qcow2_flush_caches(bs, true, &local_err);
+    ret = qcow2_flush_caches(bs, true, errp);
     if (ret < 0) {
+        if (!force) {
+            return ret;
+        }
         result = ret;
         error_report_err(local_err);
         local_err = NULL;
@@ -2702,14 +2721,25 @@ static int qcow2_inactivate(BlockDriverState *bs)
     if (result == 0 && bdrv_is_writable(bs)) {
         ret = qcow2_mark_clean(bs);
         if (ret < 0) {
-            error_report("Failed to mark qcow2 node '%s' clean",
-                         bdrv_get_device_or_node_name(bs));
+            error_setg(errp, "Failed to mark qcow2 node '%s' clean",
+                       bdrv_get_device_or_node_name(bs));
+            if (!force) {
+                return ret;
+            }
+            result = ret;
+            error_report_err(local_err);
+            local_err = NULL;
         }
     }
 
     return result;
 }
 
+static int qcow2_inactivate(BlockDriverState *bs)
+{
+    return qcow2_do_inactivate(bs, true, &error_abort);
+}
+
 static void qcow2_close(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-- 
2.31.1

[PATCH 12/14] qcow2: implement .bdrv_close_safe

[Vladimir Sementsov-Ogievskiy]

Implement new API, so that qcow2 supports blockdev-del with
force=false.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h |  2 +
 block.c               |  4 +-
 block/qcow2.c         | 85 ++++++++++++++++++++++++++++++++++---------
 3 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 42d78a7a31..fbb5f3ff6d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -672,6 +672,8 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs);
 void bdrv_ref(BlockDriverState *bs);
 void bdrv_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
+int bdrv_unref_child_safe(BlockDriverState *parent, BdrvChild *child,
+                          Transaction *tran, Error **errp);
 int bdrv_try_unref(BlockDriverState *bs, Error **errp);
 BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              BlockDriverState *child_bs,
diff --git a/block.c b/block.c
index 187732c6f8..dd2ddedc86 100644
--- a/block.c
+++ b/block.c
@@ -3216,8 +3216,8 @@ static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child,
  * When @tran is not NULL, first failure is returned and the action may be
  * rolled back.
  */
-static int bdrv_unref_child_safe(BlockDriverState *parent, BdrvChild *child,
-                                 Transaction *tran, Error **errp)
+int bdrv_unref_child_safe(BlockDriverState *parent, BdrvChild *child,
+                          Transaction *tran, Error **errp)
 {
     if (child == NULL) {
         return 0;
diff --git a/block/qcow2.c b/block/qcow2.c
index 8ad555feb7..a8f891ee34 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2740,23 +2740,24 @@ static int qcow2_inactivate(BlockDriverState *bs)
     return qcow2_do_inactivate(bs, true, &error_abort);
 }
 
-static void qcow2_close(BlockDriverState *bs)
-{
-    BDRVQcow2State *s = bs->opaque;
-    qemu_vfree(s->l1_table);
-    /* else pre-write overlap checks in cache_destroy may crash */
-    s->l1_table = NULL;
+typedef struct Qcow2CloseState {
+    BlockDriverState *bs;
+    void *old_l1_table;
+} Qcow2CloseState;
 
-    if (!(s->flags & BDRV_O_INACTIVE)) {
-        qcow2_inactivate(bs);
-    }
+static void qcow2_close_commit(void *opaque)
+{
+    Qcow2CloseState *cs = opaque;
+    BlockDriverState *bs = cs->bs;
+    BDRVQcow2State *s = bs->opaque;
+
+    qemu_vfree(cs->old_l1_table);
 
     cache_clean_timer_del(bs);
     qcow2_cache_destroy(s->l2_table_cache);
     qcow2_cache_destroy(s->refcount_block_cache);
 
     qcrypto_block_free(s->crypto);
-    s->crypto = NULL;
     qapi_free_QCryptoBlockOpenOptions(s->crypto_opts);
 
     g_free(s->unknown_header_fields);
@@ -2766,15 +2767,65 @@ static void qcow2_close(BlockDriverState *bs)
     g_free(s->image_backing_file);
     g_free(s->image_backing_format);
 
-    if (has_data_file(bs)) {
-        bdrv_unref_child(bs, s->data_file);
-        s->data_file = NULL;
-    }
-
     qcow2_refcount_close(bs);
     qcow2_free_snapshots(bs);
 }
 
+static void qcow2_close_abort(void *opaque)
+{
+    Qcow2CloseState *cs = opaque;
+    BlockDriverState *bs = cs->bs;
+    BDRVQcow2State *s = bs->opaque;
+
+    s->l1_table = cs->old_l1_table;
+}
+
+static TransactionActionDrv qcow2_close_drv = {
+    .commit = qcow2_close_commit,
+    .abort = qcow2_close_abort,
+    .clean = g_free,
+};
+
+static int qcow2_close_safe(BlockDriverState *bs, Transaction *tran,
+                            Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret;
+    Qcow2CloseState *cs = g_new(Qcow2CloseState, 1);
+
+    *cs = (Qcow2CloseState) {
+        .bs = bs,
+        .old_l1_table = s->l1_table,
+    };
+
+    /* else pre-write overlap checks in cache_destroy may crash */
+    s->l1_table = NULL;
+    if (tran) {
+        tran_add(tran, &qcow2_close_drv, cs);
+    }
+
+    if (!(s->flags & BDRV_O_INACTIVE)) {
+        ret = qcow2_do_inactivate(bs, !tran, errp);
+        if (ret < 0 && tran) {
+            return ret;
+        }
+    }
+
+    if (has_data_file(bs)) {
+        ret = bdrv_unref_child_safe(bs, s->data_file, tran, errp);
+        if (ret < 0 && tran) {
+            return ret;
+        }
+    }
+
+    if (!tran) {
+        qcow2_close_commit(cs);
+        g_free(cs);
+    }
+
+    return 0;
+}
+
 static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
                                                    Error **errp)
 {
@@ -2793,7 +2844,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
     crypto = s->crypto;
     s->crypto = NULL;
 
-    qcow2_close(bs);
+    qcow2_close_safe(bs, NULL, &error_abort);
 
     memset(s, 0, sizeof(BDRVQcow2State));
     options = qdict_clone_shallow(bs->options);
@@ -6043,7 +6094,7 @@ BlockDriver bdrv_qcow2 = {
     .instance_size      = sizeof(BDRVQcow2State),
     .bdrv_probe         = qcow2_probe,
     .bdrv_open          = qcow2_open,
-    .bdrv_close         = qcow2_close,
+    .bdrv_close_safe    = qcow2_close_safe,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
     .bdrv_reopen_commit   = qcow2_reopen_commit,
     .bdrv_reopen_commit_post = qcow2_reopen_commit_post,
-- 
2.31.1

[PATCH 13/14] block/file-posix: implement .bdrv_close_safe

[Vladimir Sementsov-Ogievskiy]

Implement new close API for 'file' driver, so that we now have the
minimal set working: qcow2 + file.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/file-posix.c | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1f1756e192..90642d8185 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2195,14 +2195,38 @@ static void raw_aio_attach_aio_context(BlockDriverState *bs,
 #endif
 }
 
-static void raw_close(BlockDriverState *bs)
+static void raw_close_commit(void *opaque)
+{
+    BDRVRawState *s = opaque;
+
+    if (s->fd >= 0) {
+        /*
+         * Closing fd is unrecoverable action, that's why it is in .commit.
+         * So, yes, it may fail, but we ignore the failure.
+         */
+        qemu_close(s->fd);
+        s->fd = -1;
+    }
+}
+
+TransactionActionDrv raw_close_drv = {
+    .commit = raw_close_commit,
+};
+
+static int raw_close_safe(BlockDriverState *bs, Transaction *tran,
+                          Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
     if (s->fd >= 0) {
-        qemu_close(s->fd);
-        s->fd = -1;
+        if (tran) {
+            tran_add(tran, &raw_close_drv, s);
+        } else {
+            raw_close_commit(s);
+        }
     }
+
+    return 0;
 }
 
 /**
@@ -3278,7 +3302,7 @@ BlockDriver bdrv_file = {
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit = raw_reopen_commit,
     .bdrv_reopen_abort = raw_reopen_abort,
-    .bdrv_close = raw_close,
+    .bdrv_close_safe = raw_close_safe,
     .bdrv_co_create = raw_co_create,
     .bdrv_co_create_opts = raw_co_create_opts,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
@@ -3643,7 +3667,7 @@ static BlockDriver bdrv_host_device = {
     .bdrv_probe_device  = hdev_probe_device,
     .bdrv_parse_filename = hdev_parse_filename,
     .bdrv_file_open     = hdev_open,
-    .bdrv_close         = raw_close,
+    .bdrv_close_safe    = raw_close_safe,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
@@ -3771,7 +3795,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_probe_device	= cdrom_probe_device,
     .bdrv_parse_filename = cdrom_parse_filename,
     .bdrv_file_open     = cdrom_open,
-    .bdrv_close         = raw_close,
+    .bdrv_close_safe    = raw_close_safe,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
@@ -3902,7 +3926,7 @@ static BlockDriver bdrv_host_cdrom = {
     .bdrv_probe_device	= cdrom_probe_device,
     .bdrv_parse_filename = cdrom_parse_filename,
     .bdrv_file_open     = cdrom_open,
-    .bdrv_close         = raw_close,
+    .bdrv_close_safe    = raw_close_safe,
     .bdrv_reopen_prepare = raw_reopen_prepare,
     .bdrv_reopen_commit  = raw_reopen_commit,
     .bdrv_reopen_abort   = raw_reopen_abort,
-- 
2.31.1

[PATCH 14/14] iotests: add test for blockdev-del(force=false)

[Vladimir Sementsov-Ogievskiy]

Test for new option: use NBD server killing to simulate failure on file
close.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 .../tests/blockdev-del-close-failure          | 54 +++++++++++++++++++
 .../tests/blockdev-del-close-failure.out      |  4 ++
 2 files changed, 58 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/blockdev-del-close-failure
 create mode 100644 tests/qemu-iotests/tests/blockdev-del-close-failure.out

diff --git a/tests/qemu-iotests/tests/blockdev-del-close-failure b/tests/qemu-iotests/tests/blockdev-del-close-failure
new file mode 100755
index 0000000000..12b442f43f
--- /dev/null
+++ b/tests/qemu-iotests/tests/blockdev-del-close-failure
@@ -0,0 +1,54 @@
+#!/usr/bin/env python3
+#
+# Test blockdev-add force=false
+#
+# 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/>.
+#
+
+import iotests
+from iotests import qemu_img_create, qemu_img, qemu_nbd_popen, qemu_img_pipe
+
+iotests.script_initialize(supported_fmts=['qcow2'],
+                          unsupported_imgopts=['compat'])
+
+disk, nbd_sock = iotests.file_path('disk', 'nbd-sock')
+size = '1M'
+
+assert qemu_img_create('-f', iotests.imgfmt, disk, size) == 0
+assert qemu_img('bitmap', '--add', disk, 'bitmap0') == 0
+assert 'bitmaps' in qemu_img_pipe('info', disk)
+
+vm = iotests.VM()
+vm.launch()
+
+with qemu_nbd_popen('-k', nbd_sock, '-f', 'raw', disk):
+    result = vm.qmp('blockdev-add', {
+        'node-name': 'disk0',
+        'driver': 'qcow2',
+        'file': {
+            'driver': 'nbd',
+            'server': {
+                'type': 'unix',
+                'path': nbd_sock
+            }
+        }
+    })
+    assert result == {'return': {}}
+
+# Now bitmap is loaded and marked IN_USE in the image, but connection is lost
+# Bitmap can't be saved and blockdev-del(force=false) should fail
+vm.qmp_log('blockdev-del', node_name='disk0', force=False)
+vm.shutdown()
diff --git a/tests/qemu-iotests/tests/blockdev-del-close-failure.out b/tests/qemu-iotests/tests/blockdev-del-close-failure.out
new file mode 100644
index 0000000000..4a4776cc98
--- /dev/null
+++ b/tests/qemu-iotests/tests/blockdev-del-close-failure.out
@@ -0,0 +1,4 @@
+Start NBD server
+Kill NBD server
+{"execute": "blockdev-del", "arguments": {"force": false, "node-name": "disk0"}}
+{"error": {"class": "GenericError", "desc": "Failed to flush node 'disk0'"}}
-- 
2.31.1