On 9/24/19 6:12 AM, Max Reitz wrote:
> On 07.08.19 16:12, Vladimir Sementsov-Ogievskiy wrote:
>> It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
>> can't work, as qcow2 needs write access to file child, to mark bitmaps
>> in-image with IN_USE flag. But usually children goes after parents in
>> reopen queue and file child is still RO on qcow2 reopen commit. Reverse
>> reopen order to fix it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
>> ---
>> block.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 696162cd7a..d59f9f97cb 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3476,10 +3476,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
>> Error **errp)
>> bs_entry->perms_checked = true;
>> }
>>
>> - /* If we reach this point, we have success and just need to apply the
>> - * changes
>> + /*
>> + * If we reach this point, we have success and just need to apply the
>> + * changes.
>> + *
>> + * Reverse order is used to comfort qcow2 driver: on commit it need to
>> write
>> + * IN_USE flag to the image, to mark bitmaps in the image as invalid.
>> But
>> + * children are usually goes after parents in reopen-queue, so go from
>> last
>> + * to first element.
>> */
>> - QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
>> + QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
>> bdrv_reopen_commit(&bs_entry->state);
>> }
>
> I suppose this works, but only because everything but the IN_USE thing
> actually behaves correctly. In theory, all the work is done by the time
> .prepare is through, so we can call commit in any order anyway.
>
> So I’m still of the opinion that writing IN_USE in commit is just plain
> wrong.
>
> It feels like you’re trying to work around wrongs in reopen by piling
> more wrongs on top. I don’t like reopen already, and I don’t think this
> makes it any better.
>
> I don’t like how the comment implies that everything is just as it
> should be, but that isn’t the real problem here, so whatever.
>
>
> Well, at least the change is simple, and it doesn’t make things worse
> than they actually are already (that is, wrong), so
>
> Acked-by: Max Reitz <mre...@redhat.com>
>
Thanks, Max!
Unfortunate, but I agree.
Acked-by: John Snow <js...@redhat.com>
