mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Revert "block: Let replace_child_noperm free children"

Browse Source 
We are going to reimplement this behavior (clear bs->file / bs->backing
pointers automatically when child->bs is cleared) in a nicer way, see
further commit
"block: Manipulate bs->file / bs->backing pointers in .attach/.detach".

With this revert we bring back a problem that was fixed by b0a9f6fed3.
Still the problem was mostly theoretical, we don't have concrete bugs
fixed by b0a9f6fed3, we don't have a specific test. Probably some
accidental failures of iotests are related.

Alternatively, we may merge this and following three reverts into final
"block: Manipulate ..." to avoid any kind of regression. But seems that
in this case having separate clear revert commits is better.

This reverts commit b0a9f6fed3.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
Message-Id: <20220726201134.924743-10-vsementsov@yandex-team.ru>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
This commit is contained in:
Vladimir Sementsov-Ogievskiy 2022-07-26 23:11:28 +03:00 committed by Kevin Wolf
parent 0c6100a7ff
commit 4eba825a82
1 changed files with 23 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

102
block.c
View File

@ -90,10 +90,8 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
static bool bdrv_recurse_has_child(BlockDriverState *bs,
BlockDriverState *child);
static void bdrv_child_free(BdrvChild *child);
static void bdrv_replace_child_noperm(BdrvChild **child,
BlockDriverState *new_bs,
bool free_empty_child);
BlockDriverState *new_bs);
static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
BdrvChild *child,
Transaction *tran);
@ -2345,7 +2343,6 @@ typedef struct BdrvReplaceChildState {
BdrvChild *child;
BdrvChild **childp;
BlockDriverState *old_bs;
bool free_empty_child;
} BdrvReplaceChildState;
static void bdrv_replace_child_commit(void *opaque)
@ -2353,9 +2350,6 @@ static void bdrv_replace_child_commit(void *opaque)
BdrvReplaceChildState *s = opaque;
GLOBAL_STATE_CODE();
if (s->free_empty_child && !s->child->bs) {
bdrv_child_free(s->child);
}
bdrv_unref(s->old_bs);
}
@ -2373,26 +2367,22 @@ static void bdrv_replace_child_abort(void *opaque)
* modify the BdrvChild * pointer we indirectly pass to it, i.e. it
* will not modify s->child. From that perspective, it does not matter
* whether we pass s->childp or &s->child.
* (TODO: Right now, bdrv_replace_child_noperm() never modifies that
* pointer anyway (though it will in the future), so at this point it
* absolutely does not matter whether we pass s->childp or &s->child.)
* (2) If new_bs is not NULL, s->childp will be NULL. We then cannot use
* it here.
* (3) If new_bs is NULL, *s->childp will have been NULLed by
* bdrv_replace_child_tran()'s bdrv_replace_child_noperm() call, and we
* must not pass a NULL *s->childp here.
* (TODO: In its current state, bdrv_replace_child_noperm() will not
* have NULLed *s->childp, so this does not apply yet. It will in the
* future.)
*
* So whether new_bs was NULL or not, we cannot pass s->childp here; and in
* any case, there is no reason to pass it anyway.
*/
bdrv_replace_child_noperm(&s->child, s->old_bs, true);
/*
* The child was pre-existing, so s->old_bs must be non-NULL, and
* s->child thus must not have been freed
*/
assert(s->child != NULL);
if (!new_bs) {
/* As described above, *s->childp was cleared, so restore it */
assert(s->childp != NULL);
*s->childp = s->child;
}
bdrv_replace_child_noperm(&s->child, s->old_bs);
bdrv_unref(new_bs);
}
@ -2409,44 +2399,30 @@ static TransactionActionDrv bdrv_replace_child_drv = {
*
* The function doesn't update permissions, caller is responsible for this.
*
* (*childp)->bs must not be NULL.
*
* Note that if new_bs == NULL, @childp is stored in a state object attached
* to @tran, so that the old child can be reinstated in the abort handler.
* Therefore, if @new_bs can be NULL, @childp must stay valid until the
* transaction is committed or aborted.
*
* If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
* freed (on commit). @free_empty_child should only be false if the
* caller will free the BDrvChild themselves (which may be important
* if this is in turn called in another transactional context).
* (TODO: The reinstating does not happen yet, but it will once
* bdrv_replace_child_noperm() NULLs *childp when new_bs is NULL.)
*/
static void bdrv_replace_child_tran(BdrvChild **childp,
BlockDriverState *new_bs,
Transaction *tran,
bool free_empty_child)
Transaction *tran)
{
BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
*s = (BdrvReplaceChildState) {
.child = *childp,
.childp = new_bs == NULL ? childp : NULL,
.old_bs = (*childp)->bs,
.free_empty_child = free_empty_child,
};
tran_add(tran, &bdrv_replace_child_drv, s);
/* The abort handler relies on this */
assert(s->old_bs != NULL);
if (new_bs) {
bdrv_ref(new_bs);
}
/*
* Pass free_empty_child=false, we will free the child (if
* necessary) in bdrv_replace_child_commit() (if our
* @free_empty_child parameter was true).
*/
bdrv_replace_child_noperm(childp, new_bs, false);
bdrv_replace_child_noperm(childp, new_bs);
/* old_bs reference is transparently moved from *childp to @s */
}
@ -2828,22 +2804,8 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)
return permissions[qapi_perm];
}
/**
* Replace (*childp)->bs by @new_bs.
*
* If @new_bs is NULL, *childp will be set to NULL, too: BDS parents
* generally cannot handle a BdrvChild with .bs == NULL, so clearing
* BdrvChild.bs should generally immediately be followed by the
* BdrvChild pointer being cleared as well.
*
* If @free_empty_child is true and @new_bs is NULL, the BdrvChild is
* freed. @free_empty_child should only be false if the caller will
* free the BdrvChild themselves (this may be important in a
* transactional context, where it may only be freed on commit).
*/
static void bdrv_replace_child_noperm(BdrvChild **childp,
BlockDriverState *new_bs,
bool free_empty_child)
BlockDriverState *new_bs)
{
BdrvChild *child = *childp;
BlockDriverState *old_bs = child->bs;
@ -2882,9 +2844,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
}
child->bs = new_bs;
if (!new_bs) {
*childp = NULL;
}
if (new_bs) {
assert_bdrv_graph_writable(new_bs);
@ -2915,10 +2874,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp,
bdrv_parent_drained_end_single(child);
drain_saldo++;
}
if (free_empty_child && !child->bs) {
bdrv_child_free(child);
}
}
/**
@ -2950,14 +2905,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
BlockDriverState *bs = child->bs;
GLOBAL_STATE_CODE();
/*
* Pass free_empty_child=false, because we still need the child
* for the AioContext operations on the parent below; those
* BdrvChildClass methods all work on a BdrvChild object, so we
* need to keep it as an empty shell (after this function, it will
* not be attached to any parent, and it will not have a .bs).
*/
bdrv_replace_child_noperm(s->child, NULL, false);
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);
@ -2979,6 +2927,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
bdrv_unref(bs);
bdrv_child_free(child);
*s->child = NULL;
}
static TransactionActionDrv bdrv_attach_child_common_drv = {
@ -3057,9 +3006,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,
}
bdrv_ref(child_bs);
bdrv_replace_child_noperm(&new_child, child_bs, true);
/* child_bs was non-NULL, so new_child must not have been freed */
assert(new_child != NULL);
bdrv_replace_child_noperm(&new_child, child_bs);
*child = new_child;
@ -3120,7 +3067,8 @@ static void bdrv_detach_child(BdrvChild **childp)
BlockDriverState *old_bs = (*childp)->bs;
GLOBAL_STATE_CODE();
bdrv_replace_child_noperm(childp, NULL, true);
bdrv_replace_child_noperm(childp, NULL);
bdrv_child_free(*childp);
if (old_bs) {
/*
@ -5171,11 +5119,7 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
}
if (child->bs) {
/*
* Pass free_empty_child=false, we will free the child in
* bdrv_remove_filter_or_cow_child_commit()
*/
bdrv_replace_child_tran(childp, NULL, tran, false);
bdrv_replace_child_tran(childp, NULL, tran);
}
s = g_new(BdrvRemoveFilterOrCowChild, 1);
@ -5185,6 +5129,8 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
.is_backing = (childp == &bs->backing),
};
tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s);
*childp = NULL;
}
/*
@ -5228,7 +5174,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from,
* Passing a pointer to the local variable @c is fine here, because
* @to is not NULL, and so &c will not be attached to the transaction.
*/
bdrv_replace_child_tran(&c, to, tran, true);
bdrv_replace_child_tran(&c, to, tran);
}
return 0;
@ -5393,9 +5339,7 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
bdrv_drained_begin(old_bs);
bdrv_drained_begin(new_bs);
bdrv_replace_child_tran(&child, new_bs, tran, true);
/* @new_bs must have been non-NULL, so @child must not have been freed */
assert(child != NULL);
bdrv_replace_child_tran(&child, new_bs, tran);
found = g_hash_table_new(NULL, NULL);
refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);