[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() r
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot |
Date: |
Fri, 8 Apr 2016 18:26:07 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 08.04.2016 um 17:49 hat Max Reitz geschrieben:
> On 07.04.2016 13:29, Kevin Wolf wrote:
> > Am 06.04.2016 um 19:57 hat Max Reitz geschrieben:
> >> If bdrv_open_inherit() creates a snapshot BDS and *pbs is NULL, that
> >> snapshot BDS should be returned instead of the BDS under it.
> >>
> >> To this end, bdrv_append_temp_snapshot() now returns the snapshot BDS
> >> instead of just appending it on top of the snapshotted BDS. Also, it
> >> calls bdrv_ref() before bdrv_append() (which bdrv_open_inherit() has to
> >> undo if not returning the overlay).
> >>
> >> Signed-off-by: Max Reitz <address@hidden>
> >
> > This is a tricky patch, but after all it looks correct to me. I think we
> > could improve a bit on the documentation, though:
> >
> > 1. The commit message suggests that by returning the wrong BDS we may
> > have an observable bug. It would be good to add details on why this
> > used to be harmless (IIUC, all users of BDRV_O_SNAPSHOT go through
> > blk_new_open(), and there first setting *pbs (which is blk->root->bs)
> > and then doing bdrv_append() does the right thing)
>
> To be honest, I'd rather not wrap my head around why it worked, but I'll
> try to.
I hope you just need to verify my theory stated above. ;-)
> > 2. The refcounting stuff isn't obvious either:
> >
> >> @@ -1481,12 +1482,16 @@ static int
> >> bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
> >> goto out;
> >> }
> >>
> >> + bdrv_ref(bs_snapshot);
> >> bdrv_append(bs_snapshot, bs);
> >
> > This is because bdrv_append() drops the reference, but we want to return
> > a strong reference.
>
> Well, it's mostly because now we do return a (strong) reference. Before,
> bs_snapshot simply was not returned at all.
>
> I'll add a comment, though.
Yes, that's the additional reference you need. I just kept all existing
references in mind:
We already get a strong reference from bdrv_new(). bdrv_append() takes
ownership of a strong reference, though, and this is why we need a
second one to return.
> >> /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> >> * temporary snapshot afterwards. */
> >> if (snapshot_flags) {
> >> - ret = bdrv_append_temp_snapshot(bs, snapshot_flags,
> >> snapshot_options,
> >> - &local_err);
> >> + BlockDriverState *snapshot_bs;
> >> + snapshot_bs = bdrv_append_temp_snapshot(bs, snapshot_flags,
> >> + snapshot_options,
> >> &local_err);
> >> snapshot_options = NULL;
> >> if (local_err) {
> >> + ret = -EINVAL;
> >> goto close_and_fail;
> >> }
> >> + if (!*pbs) {
> >> + /* The reference is now held by the overlay BDS */
> >> + bdrv_unref(bs);
> >
> > We still hold a strong reference to the newly created bs that we wanted
> > to return, but now we're returning a different BDS, so we must drop the
> > reference. (The overlay BDS doesn't hold "the" same reference as the
> > comment suggests, but an additional one.)
> >
> >> + bs = snapshot_bs;
> >> + } else {
> >> + /* It is still referenced in the same way that *pbs was
> >> referenced,
> >> + * however that may be */
> >> + bdrv_unref(snapshot_bs);
> >
> > In this case we don't in fact return the reference for bs_snapshot, so
> > drop it.
> >
> > So I think what I would like here is comments that explain where the
> > ownership of the individual strong references goes, not who else may or
> > may not hold additional references to a BDS.
>
> Well, the ownership goes away. ;-)
Still good to know that this function owns the reference before it drops
it. ;-)
> I've fattened the comments so they explain exactly why the refcount is
> decremented, respectively, and why this will most likely not result in
> the deletion of the BDS.
Sounds good.
Kevin
pgpyhKuxuw8xD.pgp
Description: PGP signature
- [Qemu-devel] [PATCH v2 for-2.7 0/8] blockdev: (Nearly) free clean-up work, Max Reitz, 2016/04/06
- [Qemu-devel] [PATCH v2 for-2.7 3/8] tests: Drop BDS from test-throttle.c, Max Reitz, 2016/04/06
- [Qemu-devel] [PATCH v2 for-2.7 4/8] block: Drop blk_new_with_bs(), Max Reitz, 2016/04/06
- [Qemu-devel] [PATCH v2 for-2.7 7/8] block: Assert !bs->refcnt in bdrv_close(), Max Reitz, 2016/04/06
- [Qemu-devel] [PATCH v2 for-2.7 5/8] block: Drop bdrv_new_root(), Max Reitz, 2016/04/06
- [Qemu-devel] [PATCH v2 for-2.7 6/8] block: Make bdrv_open() return a BDS, Max Reitz, 2016/04/06