[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 13/14] block: Manipulate bs->file / bs->backing pointers in .atta
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
[PATCH 13/14] block: Manipulate bs->file / bs->backing pointers in .attach/.detach |
Date: |
Fri, 3 Dec 2021 21:25:52 +0100 |
bs->file and bs->backing are a kind of duplication of part of
bs->children. But very useful diplication, so let's not drop them at
all:)
We should manage bs->file and bs->backing in same place, where we
manage bs->children, to keep them in sync.
Moreover, generic io paths are unprepared to BdrvChild without a bs, so
it's double good to clear bs->file / bs->backing when we detach the
child.
Detach is simple: if we detach bs->file or bs->backing child, just
set corresponding field to NULL.
Attach is a bit more complicated. But we still can precisely detect
should we set one of bs->file / bs->backing or not:
- if role is BDRV_CHILD_COW, we definitely deal with bs->backing
- else, if role is BDRV_CHILD_FILTERED (it must be also
BDRV_CHILD_PRIMARY), it's a filtered child. Use
bs->drv->filtered_child_is_backing to chose the pointer field to
modify.
- else, if role is BDRV_CHILD_PRIMARY, we deal with bs->file
- in all other cases, it's neither bs->backing nor bs->file. It's some
other child and we shouldn't care
OK. This change brings one more good thing: we can (and should) get rid
of all indirect pointers in the block-graph-change transactions:
bdrv_attach_child_common() stores BdrvChild** into transaction to clear
it on abort.
bdrv_attach_child_common() has two callers: bdrv_attach_child_noperm()
just pass-through this feature, bdrv_root_attach_child() doesn't need
the feature.
Look at bdrv_attach_child_noperm() callers:
- bdrv_attach_child() doesn't need the feature
- bdrv_set_file_or_backing_noperm() uses the feature to manage
bs->file and bs->backing, we don't want it anymore
- bdrv_append() uses the feature to manage bs->backing, again we
don't want it anymore
So, we should drop this stuff! Great!
We still keep BdrvChild** argument to return the child and int return
value, and not move to simply returning BdrvChild*, as we don't want to
lose int return values.
However we don't require *@child to be NULL anymore, and even allow
@child to be NULL, if caller don't need the new child pointer.
Finally, we now set .file / .backing automatically in generic code and
want to restring setting them by hand outside of .attach/.detach.
So, this patch cleanups all remaining places where they were set.
To find such places I use:
git grep '\->file ='
git grep '\->backing ='
git grep '&.*\<backing\>'
git grep '&.*\<file\>'
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/block_int.h | 15 +++-
block.c | 155 ++++++++++++++++-------------------
block/raw-format.c | 4 +-
block/snapshot.c | 1 -
tests/unit/test-bdrv-drain.c | 10 +--
5 files changed, 88 insertions(+), 97 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 919e33de5f..4ea800e589 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -937,9 +937,6 @@ struct BlockDriverState {
QDict *full_open_options;
char exact_filename[PATH_MAX];
- BdrvChild *backing;
- BdrvChild *file;
-
/* I/O Limits */
BlockLimits bl;
@@ -992,7 +989,19 @@ struct BlockDriverState {
* which can affect this node by changing these defaults). This is always a
* parent node of this node. */
BlockDriverState *inherits_from;
+
+ /*
+ * @backing and @file are some of @children or NULL. All these three fields
+ * (@file, @backing and @children) are modified only in
+ * bdrv_child_cb_attach() and bdrv_child_cb_detach().
+ *
+ * See also comment in include/block/block.h, to learn how backing and file
+ * are connected with BdrvChildRole.
+ */
QLIST_HEAD(, BdrvChild) children;
+ BdrvChild *backing;
+ BdrvChild *file;
+
QLIST_HEAD(, BdrvChild) parents;
QDict *options;
diff --git a/block.c b/block.c
index d57d7a80ab..0c6bbc9b0b 100644
--- a/block.c
+++ b/block.c
@@ -1388,9 +1388,33 @@ static void bdrv_child_cb_attach(BdrvChild *child)
BlockDriverState *bs = child->opaque;
QLIST_INSERT_HEAD(&bs->children, child, next);
+ if (bs->drv->is_filter | (child->role & BDRV_CHILD_FILTERED)) {
+ /*
+ * Here we handle filters and block/raw-format.c when it behave like
+ * filter.
+ */
+ assert(!(child->role & BDRV_CHILD_COW));
+ if (child->role & (BDRV_CHILD_PRIMARY | BDRV_CHILD_FILTERED)) {
+ assert(child->role & BDRV_CHILD_PRIMARY);
+ assert(child->role & BDRV_CHILD_FILTERED);
+ assert(!bs->backing);
+ assert(!bs->file);
- if (child->role & BDRV_CHILD_COW) {
+ if (bs->drv->filtered_child_is_backing) {
+ bs->backing = child;
+ } else {
+ bs->file = child;
+ }
+ }
+ } else if (child->role & BDRV_CHILD_COW) {
+ assert(bs->drv->supports_backing);
+ assert(!(child->role & BDRV_CHILD_PRIMARY));
+ assert(!bs->backing);
+ bs->backing = child;
bdrv_backing_attach(child);
+ } else if (child->role & BDRV_CHILD_PRIMARY) {
+ assert(!bs->file);
+ bs->file = child;
}
bdrv_apply_subtree_drain(child, bs);
@@ -1407,6 +1431,12 @@ static void bdrv_child_cb_detach(BdrvChild *child)
bdrv_unapply_subtree_drain(child, bs);
QLIST_REMOVE(child, next);
+ if (child == bs->backing) {
+ assert(child != bs->file);
+ bs->backing = NULL;
+ } else if (child == bs->file) {
+ bs->file = NULL;
+ }
}
static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
@@ -1604,7 +1634,7 @@ open_failed:
bs->drv = NULL;
if (bs->file != NULL) {
bdrv_unref_child(bs, bs->file);
- bs->file = NULL;
+ assert(!bs->file);
}
g_free(bs->opaque);
bs->opaque = NULL;
@@ -2756,7 +2786,7 @@ static void bdrv_child_free(BdrvChild *child)
}
typedef struct BdrvAttachChildCommonState {
- BdrvChild **child;
+ BdrvChild *child;
AioContext *old_parent_ctx;
AioContext *old_child_ctx;
} BdrvAttachChildCommonState;
@@ -2764,32 +2794,30 @@ typedef struct BdrvAttachChildCommonState {
static void bdrv_attach_child_common_abort(void *opaque)
{
BdrvAttachChildCommonState *s = opaque;
- BdrvChild *child = *s->child;
- BlockDriverState *bs = child->bs;
+ BlockDriverState *bs = s->child->bs;
- bdrv_replace_child_noperm(child, NULL);
+ bdrv_replace_child_noperm(s->child, NULL);
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort);
}
- if (bdrv_child_get_parent_aio_context(child) != s->old_parent_ctx) {
+ if (bdrv_child_get_parent_aio_context(s->child) != s->old_parent_ctx) {
GSList *ignore;
/* No need to ignore `child`, because it has been detached already */
ignore = NULL;
- child->klass->can_set_aio_ctx(child, s->old_parent_ctx, &ignore,
- &error_abort);
+ s->child->klass->can_set_aio_ctx(s->child, s->old_parent_ctx, &ignore,
+ &error_abort);
g_slist_free(ignore);
ignore = NULL;
- child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
+ s->child->klass->set_aio_ctx(s->child, s->old_parent_ctx, &ignore);
g_slist_free(ignore);
}
bdrv_unref(bs);
- bdrv_child_free(child);
- *s->child = NULL;
+ bdrv_child_free(s->child);
}
static TransactionActionDrv bdrv_attach_child_common_drv = {
@@ -2800,11 +2828,11 @@ static TransactionActionDrv
bdrv_attach_child_common_drv = {
/*
* Common part of attaching bdrv child to bs or to blk or to job
*
- * Resulting new child is returned through @child.
- * At start *@child must be NULL.
- * @child is saved to a new entry of @tran, so that *@child could be reverted
to
- * NULL on abort(). So referenced variable must live at least until transaction
- * end.
+ * If @child is not NULL, it's set to new created child. Note, that @child
+ * pointer is stored in the transaction and therefore not cleared on abort.
+ * Consider @child as part of return value: we may change the return value of
+ * the function to BdrvChild* and return child directly, but this way we lose
+ * different return codes.
*
* Function doesn't update permissions, caller is responsible for this.
*/
@@ -2820,8 +2848,6 @@ static int bdrv_attach_child_common(BlockDriverState
*child_bs,
AioContext *parent_ctx;
AioContext *child_ctx = bdrv_get_aio_context(child_bs);
- assert(child);
- assert(*child == NULL);
assert(child_class->get_parent_desc);
new_child = g_new(BdrvChild, 1);
@@ -2869,22 +2895,25 @@ static int bdrv_attach_child_common(BlockDriverState
*child_bs,
bdrv_ref(child_bs);
bdrv_replace_child_noperm(new_child, child_bs);
- *child = new_child;
-
BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1);
*s = (BdrvAttachChildCommonState) {
- .child = child,
+ .child = new_child,
.old_parent_ctx = parent_ctx,
.old_child_ctx = child_ctx,
};
tran_add(tran, &bdrv_attach_child_common_drv, s);
+ if (child) {
+ *child = new_child;
+ }
+
return 0;
}
/*
- * Variable referenced by @child must live at least until transaction end.
- * (see bdrv_attach_child_common() doc for details)
+ * If @child is not NULL, it's set to new created child. @child is a part of
+ * return value, not a part of transaction (see bdrv_attach_child_common() doc
+ * for details).
*
* Function doesn't update permissions, caller is responsible for this.
*/
@@ -2963,7 +2992,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
void *opaque, Error **errp)
{
int ret;
- BdrvChild *child = NULL;
+ BdrvChild *child;
Transaction *tran = tran_new();
ret = bdrv_attach_child_common(child_bs, child_name, child_class,
@@ -2977,11 +3006,10 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
out:
tran_finalize(tran, ret);
- /* child is unset on failure by bdrv_attach_child_common_abort() */
- assert((ret < 0) == !child);
bdrv_unref(child_bs);
- return child;
+
+ return ret < 0 ? NULL : child;
}
/*
@@ -3003,7 +3031,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
Error **errp)
{
int ret;
- BdrvChild *child = NULL;
+ BdrvChild *child;
Transaction *tran = tran_new();
ret = bdrv_attach_child_noperm(parent_bs, child_bs, child_name,
child_class,
@@ -3019,12 +3047,10 @@ BdrvChild *bdrv_attach_child(BlockDriverState
*parent_bs,
out:
tran_finalize(tran, ret);
- /* child is unset on failure by bdrv_attach_child_common_abort() */
- assert((ret < 0) == !child);
bdrv_unref(child_bs);
- return child;
+ return ret < 0 ? NULL : child;
}
/* Callers must ensure that child->frozen is false. */
@@ -3221,9 +3247,7 @@ static int
bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
ret = bdrv_attach_child_noperm(parent_bs, child_bs,
is_backing ? "backing" : "file",
&child_of_bds, role,
- is_backing ? &parent_bs->backing :
- &parent_bs->file,
- tran, errp);
+ NULL, tran, errp);
if (ret < 0) {
return ret;
}
@@ -3471,14 +3495,16 @@ int bdrv_open_file_child(const char *filename,
/* commit_top and mirror_top don't use this function */
assert(!parent->drv->filtered_child_is_backing);
-
role = parent->drv->is_filter ?
(BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY) : BDRV_CHILD_IMAGE;
- parent->file = bdrv_open_child(filename, options, bdref_key, parent,
- &child_of_bds, role, false, errp);
+ if (!bdrv_open_child(filename, options, bdref_key, parent,
+ &child_of_bds, role, false, errp))
+ {
+ return -EINVAL;
+ }
- return parent->file ? 0 : -EINVAL;
+ return 0;
}
/*
@@ -4731,8 +4757,8 @@ static void bdrv_close(BlockDriverState *bs)
bdrv_unref_child(bs, child);
}
- bs->backing = NULL;
- bs->file = NULL;
+ assert(!bs->backing);
+ assert(!bs->file);
g_free(bs->opaque);
bs->opaque = NULL;
qatomic_set(&bs->copy_on_read, 0);
@@ -4862,41 +4888,13 @@ static bool should_update_child(BdrvChild *c,
BlockDriverState *to)
return ret;
}
-typedef struct BdrvRemoveFilterOrCowChild {
- BdrvChild *child;
- bool is_backing;
-} BdrvRemoveFilterOrCowChild;
-
-static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
-{
- BdrvRemoveFilterOrCowChild *s = opaque;
- BlockDriverState *parent_bs = s->child->opaque;
-
- if (s->is_backing) {
- parent_bs->backing = s->child;
- } else {
- parent_bs->file = s->child;
- }
-
- /*
- * We don't have to restore child->bs here to undo
bdrv_replace_child_tran()
- * because that function is transactionable and it registered own
completion
- * entries in @tran, so .abort() for bdrv_replace_child_safe() will be
- * called automatically.
- */
-}
-
static void bdrv_remove_filter_or_cow_child_commit(void *opaque)
{
- BdrvRemoveFilterOrCowChild *s = opaque;
-
- bdrv_child_free(s->child);
+ bdrv_child_free(opaque);
}
static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = {
- .abort = bdrv_remove_filter_or_cow_child_abort,
.commit = bdrv_remove_filter_or_cow_child_commit,
- .clean = g_free,
};
/*
@@ -4907,8 +4905,6 @@ static void
bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
Transaction *tran)
{
- BdrvRemoveFilterOrCowChild *s;
-
assert(child == bs->backing || child == bs->file);
if (!child) {
@@ -4919,18 +4915,7 @@ static void
bdrv_remove_file_or_backing_child(BlockDriverState *bs,
bdrv_replace_child_tran(child, NULL, tran);
}
- s = g_new(BdrvRemoveFilterOrCowChild, 1);
- *s = (BdrvRemoveFilterOrCowChild) {
- .child = child,
- .is_backing = (child == bs->backing),
- };
- tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
-
- if (s->is_backing) {
- bs->backing = NULL;
- } else {
- bs->file = NULL;
- }
+ tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, child);
}
/*
@@ -5082,7 +5067,7 @@ int bdrv_append(BlockDriverState *bs_new,
BlockDriverState *bs_top,
ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
&child_of_bds, bdrv_backing_role(bs_new),
- &bs_new->backing, tran, errp);
+ NULL, tran, errp);
if (ret < 0) {
goto out;
}
diff --git a/block/raw-format.c b/block/raw-format.c
index bda757fd19..29435d3ac4 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -456,8 +456,8 @@ static int raw_open(BlockDriverState *bs, QDict *options,
int flags,
file_role = BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY;
}
- bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
- file_role, false, errp);
+ bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
+ file_role, false, errp);
if (!bs->file) {
return -EINVAL;
}
diff --git a/block/snapshot.c b/block/snapshot.c
index 12fa0e3904..cb184d70b4 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -279,7 +279,6 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
/* .bdrv_open() will re-attach it */
bdrv_unref_child(bs, *fallback_ptr);
- *fallback_ptr = NULL;
ret = bdrv_snapshot_goto(fallback_bs, snapshot_id, errp);
open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 45edbd867f..cb0eac5a1e 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1804,9 +1804,8 @@ static void test_drop_intermediate_poll(void)
for (i = 0; i < 3; i++) {
if (i) {
/* Takes the reference to chain[i - 1] */
- chain[i]->backing = bdrv_attach_child(chain[i], chain[i - 1],
- "chain", &chain_child_class,
- BDRV_CHILD_COW,
&error_abort);
+ bdrv_attach_child(chain[i], chain[i - 1], "chain",
+ &chain_child_class, BDRV_CHILD_COW,
&error_abort);
}
}
@@ -2024,9 +2023,8 @@ static void do_test_replace_child_mid_drain(int
old_drain_count,
new_child_bs->total_sectors = 1;
bdrv_ref(old_child_bs);
- parent_bs->backing = bdrv_attach_child(parent_bs, old_child_bs, "child",
- &child_of_bds, BDRV_CHILD_COW,
- &error_abort);
+ bdrv_attach_child(parent_bs, old_child_bs, "child", &child_of_bds,
+ BDRV_CHILD_COW, &error_abort);
for (i = 0; i < old_drain_count; i++) {
bdrv_drained_begin(old_child_bs);
--
2.31.1
- [PATCH 00/14] block: cleanup backing and file handling, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 01/14] block: BlockDriver: add .filtered_child_is_backing field, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 02/14] block: introduce bdrv_open_file_child() helper, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 09/14] Revert "block: Let replace_child_noperm free children", Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 10/14] Revert "block: Let replace_child_tran keep indirect pointer", Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 03/14] block/blklogwrites: don't care to remove bs->file child on failure, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 11/14] Revert "block: Restructure remove_file_or_backing_child()", Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 12/14] Revert "block: Pass BdrvChild ** to replace_child_noperm", Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 04/14] test-bdrv-graph-mod: update test_parallel_perm_update test case, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 05/14] tests-bdrv-drain: bdrv_replace_test driver: declare supports_backing, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 13/14] block: Manipulate bs->file / bs->backing pointers in .attach/.detach,
Vladimir Sementsov-Ogievskiy <=
- [PATCH 14/14] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 06/14] test-bdrv-graph-mod: fix filters to be filters, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 07/14] block: document connection between child roles and bs->backing/bs->file, Vladimir Sementsov-Ogievskiy, 2021/12/03
- [PATCH 08/14] block/snapshot: stress that we fallback to primary child, Vladimir Sementsov-Ogievskiy, 2021/12/03