[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing Bdr
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links |
Date: |
Tue, 12 Mar 2019 16:12:53 +0000 |
06.03.2019 21:11, Alberto Garcia wrote:
> Our permission system is useful to define what operations are allowed
> on a certain block node and includes things like BLK_PERM_WRITE or
> BLK_PERM_RESIZE among others.
>
> One of the permissions is BLK_PERM_GRAPH_MOD which allows "changing
> the node that this BdrvChild points to". The exact meaning of this has
> never been very clear, but it can be understood as "change any of the
> links connected to the node". This can be used to prevent changing a
> backing link, but it's too coarse.
>
> This patch adds a new 'frozen' attribute to BdrvChild, which forbids
> detaching the link from the node it points to, and new API to freeze
> and unfreeze a backing chain.
>
> After this change a few functions can fail, so they need additional
> checks.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
> block.c | 76
> +++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 5 ++++
> include/block/block_int.h | 6 ++++
> 3 files changed, 87 insertions(+)
>
> diff --git a/block.c b/block.c
> index 35e78e2172..6e9c72f0cd 100644
> --- a/block.c
> +++ b/block.c
> @@ -2127,6 +2127,8 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
> BlockDriverState *old_bs = child->bs;
> int i;
>
> + assert(!child->frozen);
> +
> if (old_bs && new_bs) {
> assert(bdrv_get_aio_context(old_bs) ==
> bdrv_get_aio_context(new_bs));
> }
> @@ -2343,6 +2345,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
> BlockDriverState *backing_hd,
> bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
> bdrv_inherits_from_recursive(backing_hd, bs);
>
> + if (bdrv_is_backing_chain_frozen(bs, backing_bs(bs), errp)) {
> + return;
> + }
> +
> if (backing_hd) {
> bdrv_ref(backing_hd);
> }
> @@ -3712,6 +3718,11 @@ void bdrv_replace_node(BlockDriverState *from,
> BlockDriverState *to,
> if (!should_update_child(c, to)) {
> continue;
> }
> + if (c->frozen) {
> + error_setg(errp, "Cannot change '%s' link to '%s'",
> + c->name, from->node_name);
> + goto out;
> + }
> list = g_slist_prepend(list, c);
> perm |= c->perm;
> shared &= c->shared_perm;
> @@ -3924,6 +3935,63 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
> }
>
> /*
> + * Return true if at least one of the backing links between @bs and
> + * @base is frozen. @errp is set if that's the case.
> + */
> +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState
> *base,
> + Error **errp)
> +{
> + BlockDriverState *i;
> +
> + for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> + if (i->backing->frozen) {
> + error_setg(errp, "Cannot change '%s' link from '%s' to '%s'",
> + i->backing->name, i->node_name,
> + backing_bs(i)->node_name);
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Freeze all backing links between @bs and @base.
> + * If any of the links is already frozen the operation is aborted and
> + * none of the links are modified.
> + * Returns 0 on success. On failure returns < 0 and sets @errp.
> + */
> +int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> + Error **errp)
> +{
> + BlockDriverState *i;
> +
> + if (bdrv_is_backing_chain_frozen(bs, base, errp)) {
> + return -EPERM;
> + }
> +
> + for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> + i->backing->frozen = true;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Unfreeze all backing links between @bs and @base. The caller must
> + * ensure that all links are frozen before using this function.
> + */
> +void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState
> *base)
> +{
> + BlockDriverState *i;
> +
> + for (i = bs; i != base && i->backing; i = backing_bs(i)) {
> + assert(i->backing->frozen);
> + i->backing->frozen = false;
> + }
> +}
> +
> +/*
> * Drops images above 'base' up to and including 'top', and sets the image
> * above 'top' to have base as its backing file.
> *
> @@ -3972,6 +4040,14 @@ int bdrv_drop_intermediate(BlockDriverState *top,
> BlockDriverState *base,
> goto exit;
> }
>
> + /* This function changes all links that point to top and makes
> + * them point to base. Check that none of them is frozen. */
> + QLIST_FOREACH(c, &top->parents, next_parent) {
> + if (c->frozen) {
> + goto exit;
> + }
> + }
> +
> /* If 'base' recursively inherits from 'top' then we should set
> * base->inherits_from to top->inherits_from after 'top' and all
> * other intermediate nodes have been dropped.
Hmm, looking at this I have a bit unrelated questions about
bdrv_drop_intermediate:
1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish up with
partly updated graph??
I don't see any kind of roll-back in this case.. Why it don't operate like
bdrv_replace_node,
which firstly check all permissions, and then update all parents in
fail-free loop?
2. And therefore, why we just don't call bdrv_replace_node(top, base, errp)
from bdrv_drop_intermediate?
3. About updating inherits_from: is it possible for some reason that
base->inherits_from points to some
node inside removed backing chain, but bdrv_inherits_from_recursive(base,
explicit_top) is false? In
this case we'll finish with dead-pointer in base->inherits_from.
4. Should bdrv_replace_node do something around inherits_from?
> diff --git a/include/block/block.h b/include/block/block.h
> index 5b5cf868df..e4a25674b1 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -353,6 +353,11 @@ int bdrv_drop_intermediate(BlockDriverState *top,
> BlockDriverState *base,
> BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
> BlockDriverState *bs);
> BlockDriverState *bdrv_find_base(BlockDriverState *bs);
> +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState
> *base,
> + Error **errp);
> +int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
> + Error **errp);
> +void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState
> *base);
>
>
> typedef struct BdrvCheckResult {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 836d67c1ae..c8a83c3eea 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -709,6 +709,12 @@ struct BdrvChild {
> uint64_t backup_perm;
> uint64_t backup_shared_perm;
>
> + /*
> + * This link is frozen: the child can neither be replaced nor
> + * detached from the parent.
> + */
> + bool frozen;
> +
> QLIST_ENTRY(BdrvChild) next;
> QLIST_ENTRY(BdrvChild) next_parent;
> };
>
Hmm, also, I have a related reproducer, which lead to crash, when use stream
base as commit top:
#3 __GI___assert_fail (assertion=0x55fbd2218818
"bdrv_op_blocker_is_empty(bs)", file=0x55fbd22175e0 "block.c", line=3795,
function=0x55fbd2219422 <__PRETTY_FUNCTION__.31496> "bdrv_delete") at
assert.c:101
#4 bdrv_delete (bs=0x55fbd4593cf0) at block.c:3795
#5 bdrv_unref (bs=0x55fbd4593cf0) at block.c:5051
#6 bdrv_drop_intermediate (top=0x55fbd4593cf0, base=0x55fbd3b36260,
backing_file_str=0x55fbd3b36291 "img0") at block.c:4034
#7 commit_prepare (job=0x55fbd4612000) at block/commit.c:78
#8 job_prepare (job=0x55fbd4612000) at job.c:771
#9 job_txn_apply (txn=0x55fbd4359a50, fn=0x55fbd1eae5c7 <job_prepare>) at
job.c:146
#10 job_do_finalize (job=0x55fbd4612000) at job.c:788
to reproduce:
qemu-img create -f qcow2 img0 $size; for i in {1..4}; do qemu-img create -f
qcow2 -b img$((i-1)) img$i $size; done
for i in {1,2}; do qemu-io -f qcow2 -c "write 0 $size" img$i; done
./x86_64-softmmu/qemu-system-x86_64 -nographic -qmp-pretty stdio -nodefaults
-serial none -drive
file=img4,node-name=img4,backing.node-name=img3,backing.backing.node-name=img2,backing.backing.backing.node-name=img1,backing.backing.backing.backing.node-name=img0
and copy the following input:
{ "execute": "qmp_capabilities" }
{"execute": "block-commit", "arguments": {"device": "img4", "top-node": "img1",
"job-id": "commit0", "speed": 1000000, "base-node": "img0"}}
{"execute": "block-stream", "arguments": {"device": "img3", "job-id":
"stream0", "speed": 500000, "base-node": "img1"}}
after a bit waiting, I get
qemu-system-x86_64: block.c:3795: bdrv_delete: Assertion
`bdrv_op_blocker_is_empty(bs)' failed.
Aborted (core dumped)
But with your patches 01-04 applied, this reproducer gives
{
"error": {
"class": "GenericError",
"desc": "Cannot change 'backing' link from '#block507' to 'img1'"
}
}
somewhere, and don't crash!!
Could we consider it as a reason to apply 01-04 (may be with some updates, I
didn't review them carefully yet) to v4.0 as a bug fix?
--------
Also, related. We've faced a lot of related problems, trying to implement
top-filter for stream job. It lead to failures in 30 iotest which
want use parallel stream jobs, sharing a node to be base for one stream and top
for another. It also touches the fact that stream stores
pointer to base, but don't increase its refcount, so it's illegal pointer. And
with your series we at least freeze backing child which
points to base.
But copy-on-read filter which is going to be used as stream top is .file child
based, not .backing. So, it will not work. May be
freeze_backing_chain should be (not now, but may be later) updated to handle
such filters, may be we should support backing child
in copy-on-read...
--
Best regards,
Vladimir
- Re: [Qemu-block] [PATCH v2 08/13] block: Allow changing the backing file on reopen, (continued)
- [Qemu-block] [PATCH v2 09/13] block: Add a 'mutable_opts' field to BlockDriver, Alberto Garcia, 2019/03/06
- [Qemu-block] [PATCH v2 06/13] block: Handle child references in bdrv_reopen_queue(), Alberto Garcia, 2019/03/06
- [Qemu-block] [PATCH v2 10/13] block: Add bdrv_reset_options_allowed(), Alberto Garcia, 2019/03/06
- [Qemu-block] [PATCH v2 03/13] block: Freeze the backing chain for the duration of the mirror job, Alberto Garcia, 2019/03/06
- [Qemu-block] [PATCH v2 01/13] block: Allow freezing BdrvChild links, Alberto Garcia, 2019/03/06
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links,
Vladimir Sementsov-Ogievskiy <=
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links, Alberto Garcia, 2019/03/13
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links, Vladimir Sementsov-Ogievskiy, 2019/03/13
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links, Kevin Wolf, 2019/03/13
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links, Alberto Garcia, 2019/03/13
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 01/13] block: Allow freezing BdrvChild links, Alberto Garcia, 2019/03/13
[Qemu-block] [PATCH v2 13/13] qemu-iotests: Test the x-blockdev-reopen QMP command, Alberto Garcia, 2019/03/06
[Qemu-block] [PATCH v2 12/13] block: Add an 'x-blockdev-reopen' QMP command, Alberto Garcia, 2019/03/06
[Qemu-block] [PATCH v2 02/13] block: Freeze the backing chain for the duration of the commit job, Alberto Garcia, 2019/03/06
[Qemu-block] [PATCH v2 07/13] block: Allow omitting the 'backing' option in certain cases, Alberto Garcia, 2019/03/06