Re: [PATCH v4 02/10] block: reverse order for reopen commits

[John Snow]

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 <address@hidden>
>> ---
>>  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 <address@hidden>
> 

Thanks, Max!

Unfortunate, but I agree.

Acked-by: John Snow <address@hidden>