[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 1/3] block: Allow replacement of a BDS by its
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay |
Date: |
Wed, 8 Jun 2016 10:58:35 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 06.06.2016 um 16:42 hat Max Reitz geschrieben:
> change_parent_backing_link() asserts that the BDS to be replaced is not
> used as a backing file. However, we may want to replace a BDS by its
> overlay in which case that very link should not be redirected.
>
> Signed-off-by: Max Reitz <address@hidden>
So the scenario is like this?
+---- to
v
base <- from <- top
And we want to change it into this:
base <- from <- to <- top
Okay, makes sense.
(Hm, put ASCII art in the commit message? I'd be all for it.)
> block.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index f54bc25..16463aa 100644
> --- a/block.c
> +++ b/block.c
> @@ -2224,9 +2224,22 @@ void bdrv_close_all(void)
> static void change_parent_backing_link(BlockDriverState *from,
> BlockDriverState *to)
> {
> - BdrvChild *c, *next;
> + BdrvChild *c, *next, *to_c;
>
> QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
> + if (c->role == &child_backing) {
> + /* Allow @from to be in a backing chain, but only if it is @to's
> + * backing chain. Do not replace @from by @to there. */
The comment suggests bdrv_chain_contains(), but you only accept it as a
direct child of top and you accept non-backing-file children.
Intuitively I would say that anywhere in the backing chain would make
sense, but we can always allow that later when we actually need it.
Accepting all types of children sounds right when I think about
inserting a quorum node as 'to'.
So I guess the code is fine and the comment should be changed to
correctly reflect what the code does.
> + QLIST_FOREACH(to_c, &to->children, next) {
> + if (to_c == c) {
> + break;
> + }
> + }
> + if (to_c) {
> + continue;
> + }
> + }
> +
> assert(c->role != &child_backing);
> bdrv_ref(to);
> bdrv_replace_child(c, to);
The other thing is that I'm unsure whether this function makes any sense
at all. "Replace in all parents" is kind of arbitrary. In the long term,
we may want to allow the user to specify the exact graph modifications
on (block-)job-complete.
Kevin
- [Qemu-block] [PATCH v2 0/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/06
- [Qemu-block] [PATCH v2 3/3] iotests: Add test for post-mirror backing chains, Max Reitz, 2016/06/06
- [Qemu-block] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay, Max Reitz, 2016/06/06
- Re: [Qemu-block] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay,
Kevin Wolf <=
- [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/06
- Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/08
- Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/08
- Re: [Qemu-block] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Nir Soffer, 2016/06/08