[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/3] block: Allow replacement of a BDS by its
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay |
Date: |
Wed, 8 Jun 2016 16:21:32 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 |
On 08.06.2016 10:58, Kevin Wolf wrote:
> 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.
Doesn't the assert(c->role != &child_backing) prevent this scenario?
(which is the reason for the first sentence in my commit message.)
So the scenario is rather:
+----- target
v
base <- source <- BB
to:
base <- source <- target <- BB
> (Hm, put ASCII art in the commit message? I'd be all for it.)
Well, depending on whether a v3 will come or not, I can put it there, of
course.
>
>> 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.
OK, will do.
>> + 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.
Well, as a replacement of bdrv_swap(), this function definitely does
make sense. Whether we can do even better in the long run... Is a
question for the long run, I think.
Max
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 0/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/06
- [Qemu-devel] [PATCH v2 3/3] iotests: Add test for post-mirror backing chains, Max Reitz, 2016/06/06
- [Qemu-devel] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay, Max Reitz, 2016/06/06
- [Qemu-devel] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/06
- Re: [Qemu-devel] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/08
- Re: [Qemu-devel] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Max Reitz, 2016/06/08
- Re: [Qemu-devel] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Nir Soffer, 2016/06/08
- Re: [Qemu-devel] [PATCH v2 2/3] block/mirror: Fix target backing BDS, Kevin Wolf, 2016/06/09