Re: [Qemu-block] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

20.03.2018 16:44, Max Reitz wrote:
> On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> > Consider migration with shared storage. Persistent bitmaps are stored
> > on bdrv_inactivate. Then, on destination
> > process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
> > leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
> > are already loaded on destination start. In this case we should call
> > qcow2_reopen_bitmaps_rw instead.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Reviewed-by: John Snow <address@hidden>
> > ---
> >   block/qcow2.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 7472af6931..6219666d4a 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState 
> > *bs, QDict *options,
> >           s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
> >       }
> >   
> > -    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
> > +    if (bdrv_has_readonly_bitmaps(bs)) {
> So if there are any read-only bitmaps, we'll skip
> qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
> qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.
>
> But if that's the case, shouldn't we skip that function if any bitmap
> has been loaded already, RO or R/W?  (And we can call
> qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
> just won't do anything then.)
>
> This doesn't make this patch really wrong, I'm just wondering whether it
> can do better.  (To add to that, I don't even know whether there is a
> case where qcow2_do_open() would be called with any R/W bitmaps already
> present.)
>
> Max

hm reasonable. So, we don't know about such cases, it's better to don't 
allow them.

> > +        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) 
> > {
> > +            bool header_updated = false;
> > +            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
> > +            update_header = update_header && !header_updated;
> > +        }
> > +    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
> >           update_header = false;
> >       }
> >       if (local_err != NULL) {


--
Best regards,
Vladimir