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 <mre...@redhat.com> > > 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. > 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. >> /* 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. ;-) 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. Max
signature.asc
Description: OpenPGP digital signature