From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46631) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gRG5g-000240-Lw for qemu-devel@nongnu.org; Mon, 26 Nov 2018 07:34:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gRG5a-0007KF-Qb for qemu-devel@nongnu.org; Mon, 26 Nov 2018 07:34:16 -0500 Date: Mon, 26 Nov 2018 13:33:59 +0100 From: Kevin Wolf Message-ID: <20181126123359.GC6612@localhost.localdomain> References: <20181126112828.23047-1-kwolf@redhat.com> <20181126112828.23047-2-kwolf@redhat.com> <7b3288b3-f7f8-b6cf-d4b1-b0540205e3df@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+QahgC5+KEYLbs62" Content-Disposition: inline In-Reply-To: <7b3288b3-f7f8-b6cf-d4b1-b0540205e3df@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-3.1 1/2] block: Don't inactivate children before parents List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org --+QahgC5+KEYLbs62 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 26.11.2018 um 13:05 hat Max Reitz geschrieben: > On 26.11.18 12:28, Kevin Wolf wrote: > > bdrv_child_cb_inactivate() asserts that parents are already inactive > > when children get inactivated. This precondition is necessary because > > parents could still issue requests in their inactivation code. > >=20 > > When block nodes are created individually with -blockdev, all of them > > are monitor owned and will be returned by bdrv_next() in an undefined > > order (in practice, in the order of their creation, which is usually > > children before parents), which obviously fails the assertion. > >=20 > > This patch fixes the ordering by skipping nodes with still active > > parents in bdrv_inactivate_recurse() because we know that they will be > > covered by recursion when the last active parent becomes inactive. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > >=20 > > diff --git a/block.c b/block.c > > index 5ba3435f8f..0569275e31 100644 > > --- a/block.c > > +++ b/block.c > > @@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp) > > } > > } > > =20 > > +static bool bdrv_has_active_bds_parent(BlockDriverState *bs) > > +{ > > + BdrvChild *parent; > > + > > + QLIST_FOREACH(parent, &bs->parents, next_parent) { > > + if (parent->role->parent_is_bds) { > > + BlockDriverState *parent_bs =3D parent->opaque; > > + if (!(parent_bs->open_flags & BDRV_O_INACTIVE)) { > > + return true; > > + } > > + } > > + } >=20 > Now I see why you say this might make sense as a permission. You do? To be honest, now that I found this solution, I don't think a permission makes much sense any more, because you would have the same loop, and you would only be checking a different flag in the end. > > + > > + return false; > > +} > > + > > static int bdrv_inactivate_recurse(BlockDriverState *bs, > > bool setting_flag) > > { > > @@ -4622,6 +4638,12 @@ static int bdrv_inactivate_recurse(BlockDriverSt= ate *bs, > > return -ENOMEDIUM; > > } > > =20 > > + /* Make sure that we don't inactivate a child before its parent. > > + * It will be covered by recursion from the yet active parent. */ > > + if (bdrv_has_active_bds_parent(bs)) { > > + return 0; > > + } > > + >=20 > Hm. Wouldn't it make more sense to always return early when there are > any BDS parents? Because if there are any BDS parents and none of them > are active (anymore), then this child will have been inactivated > already, and we can save ourselves the trouble of going through the rest > of the function again. I don't quite follow... If we always return early no matter whether there is an active parent, who will have inactivated the child? After trying to write up why you're wrong, I think there are two cases and both of us have a point: 1. bdrv_next() enumerates the child node first and then the last BDS parent. This is what this patch fixes. It will inactivate the child exactly once, at the time that the last parent has become inactive (and recursively calls this function for each of its children). If you remove that one inactivation, too, children won't be inactivated at all. 2. bdrv_next() enumerates the last BDS parent first and then the child. This is unlikely and might even be impossible today, but once we expose bdrv_reopen() in QMP and let the user reconfigure the edges, it probably becomes possible. In this case, even after my patch we inactivate drivers twice. Maybe we should just return early if BDRV_O_INACTIVE is already set. What makes me kind of unsure is that we already test for this flag, but only for part of the function. Any ideas how to test this? Can we create such a situation today? > Do drivers support multiple calls to .bdrv_in_activate() at all? They were probably not designed for that... Not sure if there was ever a commit where we used to call them multiple times without failing the assertion first. Kevin --+QahgC5+KEYLbs62 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJb++g3AAoJEH8JsnLIjy/WgYwQALsHGMFK7j8vakxa2CrtwoNq qFhGpm6qpiXUuUIIraflz0YJIzZ2xGSrgqwpr8bpRbmk6u+x//hyOTobrVCqyJBA f6io1pD4L/fulwpXby2gWvloPI8m3uzAdQb+HQRZ4SV+46QDNo6gCCBm4/E7Dsns tIZVBEoVSf7S5824WA0E77732lss+UlLUqL08gG3AGuQ74pFTnayusHckn/bxSI3 C+23iM5wSfvBEn20ynTZuprKuUJ12CpXIHYYxQc0wdj8W8SiP94yNf6w8ZaZ2+fQ /8FP6ziSX9EgzXxq9DMlRefF08b+CEUxo6g/4HuCzEjBUSsZ8IdtUqw+nHeJP/ud vWgfWvwUrwP61BjnOASgipnPc/ROKZ2t7of0aVpqV97daJ+SBITuJ6TXWt8YK3D6 imjhoOqKPMSzqHesrMGV2wWUHKhk0E99Vref6+w/50kYrygbvjMPh/P/TN+BjxHb R78lX1JyW1hKDvau2b9dOERuxc4lpSnu+qbhfj67AsKW6sEHp1p7hPcXG0LlGtY3 sxoCmIY4xOoZ7UsXNAvldr4ruRaeIlJ25ZPGZj81p2ug7Y/EVDf3TdZmHiNl6Tbd QXM3X4I3aIPaJY09YMrrP6b1qTuAr3GGyC61dlFWAhSrf2MrC67YVC/8fj5ChUTR rjyGKmQgX4yyd/ko6pNP =rjMt -----END PGP SIGNATURE----- --+QahgC5+KEYLbs62--