[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: |
Thu, 7 Apr 2016 13:29:47 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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)
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.
> /* 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.
Kevin
- [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