From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60467) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtdXz-00021p-RZ for qemu-devel@nongnu.org; Tue, 03 Nov 2015 10:30:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtdXv-0007bN-DI for qemu-devel@nongnu.org; Tue, 03 Nov 2015 10:30:55 -0500 Date: Tue, 3 Nov 2015 15:30:47 +0000 From: Stefan Hajnoczi Message-ID: <20151103153047.GG15414@stefanha-x1.localdomain> References: <1446044465-19312-1-git-send-email-den@openvz.org> <1446044465-19312-5-git-send-email-den@openvz.org> <20151030155253.GD16864@stefanha-x1.localdomain> <87fv0nxhp4.fsf@emacs.mitica> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Z0mFw3+mXTC5ycVe" Content-Disposition: inline In-Reply-To: <87fv0nxhp4.fsf@emacs.mitica> Subject: Re: [Qemu-devel] [PATCH 4/4] migration: add missed aio_context_acquire into HMP snapshot code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: Amit Shah , "Denis V. Lunev" , Paolo Bonzini , qemu-devel@nongnu.org, qemu-stable@nongnu.org --Z0mFw3+mXTC5ycVe Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 03, 2015 at 03:48:07PM +0100, Juan Quintela wrote: > Stefan Hajnoczi wrote: > > On Wed, Oct 28, 2015 at 06:01:05PM +0300, Denis V. Lunev wrote: > >> diff --git a/block/snapshot.c b/block/snapshot.c > >> index 89500f2..f6fa17a 100644 > >> --- a/block/snapshot.c > >> +++ b/block/snapshot.c > >> @@ -259,6 +259,9 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDrive= rState *bs, > >> { > >> int ret; > >> Error *local_err =3D NULL; > >> + AioContext *aio_context =3D bdrv_get_aio_context(bs); > >> + > >> + aio_context_acquire(aio_context); > >> =20 > >> ret =3D bdrv_snapshot_delete(bs, id_or_name, NULL, &local_err); > >> if (ret =3D=3D -ENOENT || ret =3D=3D -EINVAL) { > >> @@ -267,6 +270,8 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDrive= rState *bs, > >> ret =3D bdrv_snapshot_delete(bs, NULL, id_or_name, &local_err= ); > >> } > >> =20 > >> + aio_context_release(aio_context); > >> + > >> if (ret < 0) { > >> error_propagate(errp, local_err); > >> } > > > > Please make the caller acquire the AioContext instead of modifying > > bdrv_snapshot_delete_id_or_name() because no other functions in this > > file acquire AioContext and the API should be consistent. >=20 > That is wrong (TM). No other functions in migration/* know what an > aiocontext is, and they are fine, thanks O:-) To clarify my comment: APIs should have a consistent locking strategy. Either all of the the block/snapshot.c public functions should take the lock or none of them should. With an inconsistent locking strategy it's really hard to review code and ensure it is correct because you need to look up for each function whether or not it takes the lock internally. > So, I guess we would have to get some other function exported from the > block layer, with the aiocontext taken? >=20 > Code ends being like this: >=20 >=20 > while ((bs =3D bdrv_next(bs))) { > if (bdrv_can_snapshot(bs) && > bdrv_snapshot_find(bs, snapshot, name) >=3D 0) { > AioContext *ctx =3D bdrv_get_aio_context(bs); >=20 > aio_context_acquire(ctx); > bdrv_snapshot_delete_by_id_or_name(bs, name, &err); > aio_context_release(ctx); > .... some error handling here ... > } >=20 >=20 > As discussed on irc, we need to get some function exported from the > block layer that does this. >=20 > I am sure that I don't understand the differences between hmp_devlvm() > and del_existing_snapshots(). On IRC I commented when you posted this code because there's a bug: bdrv_can_snapshot() and bdrv_snapshot_find() must be called with AioContext acquired. So the function should actually be: while ((bs =3D bdrv_next(bs))) { AioContext *ctx =3D bdrv_get_aio_context(ctx); if (bdrv_can_snapshot(bs) && ... aio_context_release(ctx); } Stefan --Z0mFw3+mXTC5ycVe Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJWONMnAAoJEJykq7OBq3PIUHcH+gJAZdo198coWF9SZytb5Rr9 B2Fz+JYmbknJoTOIJZnNWPIYHwHetVfsPBSAvOhBL/l1STh/OQB1LtvOOYNyq9h9 W7dYhmyXIJJHd55/4oNTJgxQxCEtHkYusAcyWkwzbI+DQimDoOQ9IfaeHTGf3/pK 5l7jm0dX+nItgHBTaSqGR4P0RotAfoqq/YBW5axgkXigvwKgaEE/Qiw+CSYZt1Gg YtnOCwXVKI/tiqZJl5WmTDiTWiydRQUXSMqe5GS3G5zdSByEDGx3A8XpjFtWRckS VCnALbRnWQCG3zPrHr5nJcMClusCztDRuUKpbIQvj84AeOGfTU9/KwGzELbLg6w= =Np13 -----END PGP SIGNATURE----- --Z0mFw3+mXTC5ycVe--