From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43382) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ao88N-00072k-GF for qemu-devel@nongnu.org; Thu, 07 Apr 2016 07:30:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ao88M-0005nj-LU for qemu-devel@nongnu.org; Thu, 07 Apr 2016 07:29:59 -0400 Date: Thu, 7 Apr 2016 13:29:47 +0200 From: Kevin Wolf Message-ID: <20160407112947.GB4509@noname.redhat.com> References: <1459965434-21531-1-git-send-email-mreitz@redhat.com> <1459965434-21531-3-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1459965434-21531-3-git-send-email-mreitz@redhat.com> 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: Max Reitz Cc: Alberto Garcia , qemu-devel@nongnu.org, qemu-block@nongnu.org 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 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