From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aoYf7-0002sS-K6 for qemu-devel@nongnu.org; Fri, 08 Apr 2016 11:49:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aoYf3-00049F-Kj for qemu-devel@nongnu.org; Fri, 08 Apr 2016 11:49:33 -0400 References: <1459965434-21531-1-git-send-email-mreitz@redhat.com> <1459965434-21531-3-git-send-email-mreitz@redhat.com> <20160407112947.GB4509@noname.redhat.com> From: Max Reitz Message-ID: <5707D301.90009@redhat.com> Date: Fri, 8 Apr 2016 17:49:21 +0200 MIME-Version: 1.0 In-Reply-To: <20160407112947.GB4509@noname.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QQHApRKCXhHt7HTF5fUehPg5nBMKDN1LK" Subject: Re: [Qemu-devel] [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Alberto Garcia This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --QQHApRKCXhHt7HTF5fUehPg5nBMKDN1LK Content-Type: multipart/mixed; boundary="lqu3qdx5TmCmfTNXqFBKFsrBEkd5Ks1oJ" From: Max Reitz To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Alberto Garcia Message-ID: <5707D301.90009@redhat.com> Subject: Re: [PATCH v2 for-2.7 2/8] block: Let bdrv_open_inherit() return the snapshot References: <1459965434-21531-1-git-send-email-mreitz@redhat.com> <1459965434-21531-3-git-send-email-mreitz@redhat.com> <20160407112947.GB4509@noname.redhat.com> In-Reply-To: <20160407112947.GB4509@noname.redhat.com> --lqu3qdx5TmCmfTNXqFBKFsrBEkd5Ks1oJ Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 t= o >> undo if not returning the overlay). >> >> Signed-off-by: Max Reitz >=20 > This is a tricky patch, but after all it looks correct to me. I think w= e > could improve a bit on the documentation, though: >=20 > 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: >=20 >> @@ -1481,12 +1482,16 @@ static int bdrv_append_temp_snapshot(BlockDriv= erState *bs, int flags, >> goto out; >> } >> =20 >> + bdrv_ref(bs_snapshot); >> bdrv_append(bs_snapshot, bs); >=20 > This is because bdrv_append() drops the reference, but we want to retur= n > 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=3Don, create a temporary qcow2 overlay. bs points= to the >> * temporary snapshot afterwards. */ >> if (snapshot_flags) { >> - ret =3D bdrv_append_temp_snapshot(bs, snapshot_flags, snapsho= t_options, >> - &local_err); >> + BlockDriverState *snapshot_bs; >> + snapshot_bs =3D bdrv_append_temp_snapshot(bs, snapshot_flags,= >> + snapshot_options, &lo= cal_err); >> snapshot_options =3D NULL; >> if (local_err) { >> + ret =3D -EINVAL; >> goto close_and_fail; >> } >> + if (!*pbs) { >> + /* The reference is now held by the overlay BDS */ >> + bdrv_unref(bs); >=20 > 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.) >=20 >> + bs =3D snapshot_bs; >> + } else { >> + /* It is still referenced in the same way that *pbs was r= eferenced, >> + * however that may be */ >> + bdrv_unref(snapshot_bs); >=20 > In this case we don't in fact return the reference for bs_snapshot, so > drop it. >=20 > 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 --lqu3qdx5TmCmfTNXqFBKFsrBEkd5Ks1oJ-- --QQHApRKCXhHt7HTF5fUehPg5nBMKDN1LK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXB9MBAAoJEDuxQgLoOKytfWgIAIpiVnH+57qzPHRpzE7jahp8 AMVdJxci2OwMVdUp4op4woPm3n9jAYKStuT7FH1bqgvcQCLE25CJftpec1xvH8Ks KF1Tp2gIUtHlesEPKn0Zv/wZcBuGsnTaqHNO4LViqj195xw1JXFZyAADqIBW+v7G tbP3VrIfx8eSideWow0BZNOKjnzE8bg0ZlMD1p1xzny1CJFiA57d3bwRN2W3YKTP C03IV0hulloGI+xiSk7EcTwNz5Bi1K54se85XVDEzTYO5TtSqzmz0vt0dcYGrWmd B3q1EiVQ2F33XVdqPVFEVpPtDovyvAOXTrdKVTTdf8TsbY3nAHyraJnw6bP1oGA= =GTb3 -----END PGP SIGNATURE----- --QQHApRKCXhHt7HTF5fUehPg5nBMKDN1LK--