From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48499) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1cXk-0006sc-0M for qemu-devel@nongnu.org; Wed, 25 Nov 2015 11:03:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1cXi-0001to-Pi for qemu-devel@nongnu.org; Wed, 25 Nov 2015 11:03:39 -0500 References: <1447108773-6836-1-git-send-email-mreitz@redhat.com> <1447108773-6836-13-git-send-email-mreitz@redhat.com> <20151125155706.GE12581@noname.str.redhat.com> From: Max Reitz Message-ID: <5655DBD3.2020203@redhat.com> Date: Wed, 25 Nov 2015 17:03:31 +0100 MIME-Version: 1.0 In-Reply-To: <20151125155706.GE12581@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fgcXqiGPal9EQVfbgDpnAI5NcapHmmcV0" Subject: Re: [Qemu-devel] [PATCH v7 12/24] virtio-blk: Functions for op blocker management List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Alberto Garcia , qemu-block@nongnu.org, John Snow , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --fgcXqiGPal9EQVfbgDpnAI5NcapHmmcV0 Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 25.11.2015 16:57, Kevin Wolf wrote: > Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: >> Put the code for setting up and removing op blockers into an own >> function, respectively. Then, we can invoke those functions whenever a= >> BDS is removed from an virtio-blk BB or inserted into it. >> >> Signed-off-by: Max Reitz >=20 > Do you know of a case where this is observable? Actually, no. > I don't think you can > change the medium of a virtio-blk device, and blk_set_bs() isn't > converted to a wrapper around blk_remove/insert_bs() yet. If this patch= > is necessary to fix something observable, the latter is probably a bug.= So I guess that implies "Otherwise, this patch should be dropped"? >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virt= io-blk.c >> index c42ddeb..4c95d5b 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -39,6 +39,8 @@ struct VirtIOBlockDataPlane { >> EventNotifier *guest_notifier; /* irq */ >> QEMUBH *bh; /* bh for guest notification */ >> =20 >> + Notifier insert_notifier, remove_notifier; >> + >> /* Note that these EventNotifiers are assigned by value. This is= >> * fine as long as you do not call event_notifier_cleanup on them= >> * (because you don't own the file descriptor or handle; you just= >> @@ -137,6 +139,54 @@ static void handle_notify(EventNotifier *e) >> blk_io_unplug(s->conf->conf.blk); >> } >> =20 >> +static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s) >> +{ >> + assert(!s->blocker); >> + error_setg(&s->blocker, "block device is in use by data plane"); >> + blk_op_block_all(s->conf->conf.blk, s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_RESIZE, s->blocke= r); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_DRIVE_DEL, s->blo= cker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_BACKUP_SOURCE, s-= >blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_CHANGE, s->blocke= r); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_SOURCE, s-= >blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_COMMIT_TARGET, s-= >blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EJECT, s->blocker= ); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT= , >> + s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT= , >> + s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT= _DELETE, >> + s->blocker); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_MIRROR, s->blocke= r); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_STREAM, s->blocke= r); >> + blk_op_unblock(s->conf->conf.blk, BLOCK_OP_TYPE_REPLACE, s->block= er); >> +} >=20 > This makes me wonder: What do we even block here any more? If I didn't > miss anything, it's only BLOCK_OP_TYPE_BACKUP_TARGET, and I'm not sure > why this needs to be blocked, or if we simply forgot to enable it. Well, even though in practice this wall of code doesn't make much sense, of course it will be safe for potential additions of new op blockers. And of course we actually don't want these blockers at all anymore... Max --fgcXqiGPal9EQVfbgDpnAI5NcapHmmcV0 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 iQEcBAEBCAAGBQJWVdvTAAoJEDuxQgLoOKytQwEH/3Y5004CslAXHVf/yu/rhl5n ZQAbkpK//pMq2vBVFqvapC0JHua3/AaZUXmea+8Sp+4qOdc1F4dfz+zC2vKaGZxJ UrM/C8wqLvQF09wrwth+IlPmEckRO8Sy76HcgfPDqoPw/O/2H8ZZU9pQDxNeQrNA wPnXA0DdvFWEqSlSVKA3jQX2lVMANUZfrx72a0rcBY2ZTu+AG/p2SZPA2Vx4uFzo qgXQk98HptFMU96kubpuee5Wat22bFiJQQ9dn2wZH8mwtNgahxCbahaH+tqMitcQ o3I/9zlQa+fn74q0mNyiUXUd9XP6KIQLUwAnbkD+7GQ2x9wmeuYmTliH7hiTfbM= =Vpjs -----END PGP SIGNATURE----- --fgcXqiGPal9EQVfbgDpnAI5NcapHmmcV0--