Re: [PATCH] block/snapshot: Clarify goto fallback behavior

[Vladimir Sementsov-Ogievskiy]

05.05.2021 19:25, Max Reitz wrote:
> On 05.05.21 17:05, Vladimir Sementsov-Ogievskiy wrote:
> > 03.05.2021 12:54, Max Reitz wrote:
> > > In the bdrv_snapshot_goto() fallback code, we work with a pointer to
> > > either bs->file or bs->backing. 
> >
> > > We close that child,
> >
> > Do we?
>
> We *detach it.
>
> > > close the node
> > > (with .bdrv_close()), apply the snapshot on the child node, and then
> > > re-open the node (with .bdrv_open()).
> > >
> > > In order for .bdrv_open() to attach the same child node that we had
> > > before, we pass "file={child-node}" or "backing={child-node}" to it.
> > > Therefore, when .bdrv_open() has returned success, we can assume that
> > > bs->file or bs->backing (respectively) points to our original child
> > > again.  This is verified by an assertion.
> > >
> > > All of this is not immediately clear from a quick glance at the code,
> > > so add a comment to the assertion what it is for, and why it is valid.
> > > It certainly confused Coverity.
> > >
> > > Reported-by: Coverity (CID 1452774)
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >   block/snapshot.c | 14 +++++++++++++-
> > >   1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/block/snapshot.c b/block/snapshot.c
> > > index e8ae9a28c1..cce5e35b35 100644
> > > --- a/block/snapshot.c
> > > +++ b/block/snapshot.c
> > > @@ -275,13 +275,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > >           qobject_unref(file_options);
> > >           g_free(subqdict_prefix);
> > > +        /* Force .bdrv_open() below to re-attach fallback_bs on *fallback_ptr 
> > > */
> > >           qdict_put_str(options, (*fallback_ptr)->name,
> > >                         bdrv_get_node_name(fallback_bs));
> > > +        /* Now close bs, apply the snapshot on fallback_bs, and re-open bs */
> > >           if (drv->bdrv_close) {
> > >               drv->bdrv_close(bs);
> > >           }
> > > +        /* .bdrv_open() will re-attach it */
> > >           bdrv_unref_child(bs, *fallback_ptr);
> > >           *fallback_ptr = NULL;
> > > @@ -296,7 +299,16 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
> > >               return ret < 0 ? ret : open_ret;
> > >           }
> > > -        assert(fallback_bs == (*fallback_ptr)->bs);
> > > +        /*
> > > +         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
> > > +         * was closed above and set to NULL, but the .bdrv_open() call
> > > +         * has opened it again, because we set the respective option
> > > +         * (with the qdict_put_str() call above).
> > > +         * Assert that .bdrv_open() has attached some child on
> > > +         * *fallback_ptr, and that it has attached the one we wanted
> > > +         * it to (i.e., fallback_bs).
> > > +         */
> > > +        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
> > >           bdrv_unref(fallback_bs);
> > >           return ret;
> > >       }
> >
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >
> > ===
> >
> > I still think that this fallback has more problems.. Nothing guarantee that all 
> > format drivers (even restricted to those who have only one child) support such 
> > logic.
> >
> > For example, .bdrv_open() may rely on state structure were zeroed and not 
> > initialize some fields. And .bdrv_close() may just g_free() some things, not 
> > setting them to zero.. So we probably should call bdrv_open()/bdrv_close() 
> > generic functions. But we can't: at least bdrv_close() requires that node has 
> > no parents.
> >
> > Not saying about that we lose everything on failure (when actually, it's better 
> > to restore original state on failure).
> >
> > This feature should instead be done with help of bdrv_reopen_multiple(), and 
> > even with it it's not obvious how to do it properly.
> >
> > The feature were done long ago in 2010 with commit 
> > 7cdb1f6d305e1000b5f882257cbee71b8bb08ef5 by Morita Kazutaka.
> >
> > Note also, that the only protocol driver that support snapshots is rbd, and 
> > snapshot support was added to it in 2012 with commit 
> > bd6032470631d8d5de6db84ecb55398b70d9d2f3 by Gregory Farnum.
> >
> > Other two drivers with support are sheepdog (which is deprecated) and qcow2 (I 
> > doubt that it should be used as protocol driver for some other format).
> >
> >
> > Do we really need this fallback? Is it used somewhere? May be, we can just 
> > deprecate it, and look will someone complain?
>
> Maybe? :)

:) I'll send a patch later.


--
Best regards,
Vladimir