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

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 <vsementsov@virtuozzo.com>
---
 block.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

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 <vsementsov@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 <mreitz@redhat.com>

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 <vsementsov@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 <mreitz@redhat.com>
> 

Thanks, Max!

Unfortunate, but I agree.

Acked-by: John Snow <jsnow@redhat.com>