Make a version of bdrv_unref() that honestly report any failure. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@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