Re: [Qemu-devel] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit'

[Kevin Wolf <kwolf@redhat.com>]

Am 27.10.2015 um 17:43 hat Alberto Garcia geschrieben:
> Hi, looks like we have a bug in the bdrv_reopen() code.
> 
> It turns out that 'block-commit' fails if the 'top' node is not the
> active layer or its immediate backing file, and none of our test cases
> has detected that. I'm attaching one that reproduces the problem.
> 
> What happens is that 'block-commit' reopens the overlay of the top
> node in read-write mode in order to update the backing file string. In
> addition to that, the 'base' image also needs to be reopened in r/w.
> 
> Here's the relevant code from commit_start():
> 
>     if (!(orig_base_flags & BDRV_O_RDWR)) {
>         reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
>                                          orig_base_flags | BDRV_O_RDWR);
>     }
>     if (!(orig_overlay_flags & BDRV_O_RDWR)) {
>         reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
>                                          orig_overlay_flags | BDRV_O_RDWR);
>     }
>     if (reopen_queue) {
>         bdrv_reopen_multiple(reopen_queue, &local_err);
>         /*...*/
>     }
> 
> 'base' is reopened first in r/w mode, then 'overlay_bs'. However it
> seems that the latter has the side effect or reopening 'base' again in
> read-only mode, therefore the job ends up failing with -EPERM.
> 
> Just swapping the order of the bdrv_reopen_queue() calls is enough to
> fix the problem, but I'm sure this needs deeper changes in the
> bdrv_reopen() code instead.

I think this might be fixed by applying the rest of my bdrv_reopen()
patches and then converting read-only from a flag to an option. The
reason is that we really have three values here: ro, rw and inherit from
parent. This can be represented by options (false, true, missing QDict
entry), but not by flags.

I've applied your test case to my working branch so I won't forget about
this. Maybe I should really try to get the series into 2.5 then.

All that being said, having the same BDS twice in a reopen queue is
calling for trouble. I guess currently the last one wins, with all
changes made in the first call reverted even if the respective options
aren't touched by the second one. I wonder if we need to merge multiple
entries for the same BDS.

Kevin