Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters

From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: qemu-block@nongnu.org, armbru@redhat.com, qemu-devel@nongnu.org,
	mreitz@redhat.com, den@openvz.org, jsnow@redhat.com
Subject: Re: [PATCH v2 23/36] block: adapt bdrv_append() for inserting filters
Date: Thu, 4 Feb 2021 10:05:47 +0100	[thread overview]
Message-ID: <20210204090547.GC6496@merkur.fritz.box> (raw)
In-Reply-To: <a68bc8ae-3cc5-5f9b-e3c0-6e39b23edc87@virtuozzo.com>

Am 04.02.2021 um 09:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.02.2021 00:33, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > >   int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> > >                   Error **errp)
> > >   {
> > > -    Error *local_err = NULL;
> > > +    int ret;
> > > +    GSList *tran = NULL;
> > > -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> > > -    if (local_err) {
> > > -        error_propagate(errp, local_err);
> > > -        return -EPERM;
> > > +    assert(!bs_new->backing);
> > > +
> > > +    ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
> > > +                                   &child_of_bds, bdrv_backing_role(bs_new),
> > > +                                   &bs_new->backing, &tran, errp);
> > > +    if (ret < 0) {
> > > +        goto out;
> > >       }
> > 
> > I don't think changing bs->backing without bdrv_set_backing_hd() is
> > correct at the moment. We lose a few things:
> > 
> > 1. The bdrv_is_backing_chain_frozen() check
> > 2. Updating backing_hd->inherits_from if necessary
> > 3. bdrv_refresh_limits()
> > 
> > If I'm not missing anything, all of these are needed in the context of
> > bdrv_append().
> 
> I decided that bdrv_append() is only for appending new nodes, so
> frozen and inherts_from checks are not needed. And I've added
> assert(!bs_new->backing)...
> 
> Checking this now:
> 
> - appending filters is obvious
> - bdrv_append_temp_snapshot() creates new qcow2 node based on tmp
>   file, don't see any backing initialization (and it would be rather
>   strange)

Yes, the internal uses are obviously unproblematic for the frozen check.

> - external_snapshot_prepare() do check if
>   (bdrv_cow_child(state->new_bs)) {  error-out }

Ok, the only thing bdrv_set_backing_hd() can and must check is whether
the link to the old backing file was frozen, and we know that we don't
have an old backing file. Makes sense.

Same thing for inherits_from, we only do this if the the new backing
file (i.e. the old active layer for bdrv_append) was already in the
backing chain of the new node.

> So everything is OK. I should describe it in commit message and add a
> comment to bdrv_append.

What about bdrv_refresh_limits()? The node gains a new backing file, so
I think the limits could change.

Ideally, bdrv_child_cb_attach/detach() would take care of this, but at
the moment they don't.

Kevin