[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