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

[Max Reitz]

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>

signature.asc
Description: OpenPGP digital signature