From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52321) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYbJl-0006cx-Lh for qemu-devel@nongnu.org; Wed, 24 Feb 2016 10:25:34 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYbJj-0004yD-RD for qemu-devel@nongnu.org; Wed, 24 Feb 2016 10:25:33 -0500 References: <1455646106-2047-1-git-send-email-mreitz@redhat.com> <1455646106-2047-8-git-send-email-mreitz@redhat.com> <20160217105345.GE29494@noname.str.redhat.com> <56C494B4.8020707@redhat.com> <20160217162053.GK29494@noname.str.redhat.com> <56C86B70.8090102@redhat.com> <87oab9i24b.fsf@blackfin.pond.sub.org> <56CB3773.1030107@redhat.com> <874mcz92q1.fsf@blackfin.pond.sub.org> <56CC6429.3050008@redhat.com> <87si0izccy.fsf@blackfin.pond.sub.org> From: Max Reitz Message-ID: <56CDCB61.9050703@redhat.com> Date: Wed, 24 Feb 2016 16:25:21 +0100 MIME-Version: 1.0 In-Reply-To: <87si0izccy.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gC9hiAbSkuOkEAPiMiv2UUMmf2UdOXBTI" Subject: Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , qemu-block@nongnu.org, qemu-devel@nongnu.org, Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gC9hiAbSkuOkEAPiMiv2UUMmf2UdOXBTI Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 24.02.2016 10:28, Markus Armbruster wrote: > Max Reitz writes: >=20 >> On 23.02.2016 10:48, Markus Armbruster wrote: [...] >>> Can you explain the *actual* difference between blk_backends and >>> monitor_block_backends? "Actual" in the sense of difference in conte= nts >>> over time. >> >> First difference: The name. That's enough reason for me. >> >> Second difference: blk_new() adds any BB to blk_backends automatically= =2E >> It doesn't do so for monitor_block_backends. >> >> Third difference: Often the monitor doesn't take care of signalling th= at >> it's releasing its reference, only sometimes (where "sometimes" means >> every time blk_hide...() is called). blk_delete() will instead >> automatically remove it from blk_backends, because it's assuming that >> the last reference had been held by the monitor. >=20 > The reference held in trust for lookup by name exists as long as lookup= > by name is permitted. >=20 > Therefore, it goes away when the BB dies or when it loses its name. And I'd like it to be more explicit than this. A BB should simply never have a name anymore when it's deleted. The current life cycle has a short time where a named BB can have a refcount of 0. While I know that this time span is considered to be "atomic", it still seems off. > The only way for a BB to lose its name is (was?) the messed up HMP > drive_del: it wants to kill the BB right away, but can't, so it needs t= o > hide it instead until it can. >=20 > New functionality may lead to a more complex live cycle where BBs may > acquire and lose their name in new ways. >=20 > Note that I carefully avoid calling the reference the monitor's > reference. Because you made me realize it's not the monitor's alone! > Lookup by name has more customers than just the monitor. Maybe, but I'd personally consider this a service offered by the monitor, and thus a reference by the monitor. >> The second and third difference are what I referred to as "magic". You= >> could also call them "convenience", if you prefer, but in any case as = we >> can see by the existence blk_hide...(), removing the monitor reference= >> in blk_delete() appears to be wrong. Both should be separate operation= s, >> and this is what this patch does. >=20 > I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del() > shows that the name going away in blk_delete() is wrong. It must go > away there, because the thing it names goes away. Yes, it must, but I believe it shouldn't have a name at that point at all= =2E > Additional operations to add / remove a BBs name may make sense; I'd > have to see their users. See "more complex live cycle" above. They do make sense, if for nothing else, then because blk_hide...() is replaced by a function that actually does something that makes sense in the general life cycle. >> Assuming that any blk_new() is ultimately done by the monitor, we mayb= e >> actually do not need an own monitor_add_blk() function; except that >> Kevin stated that he does deem it useful when I proposed inlining it >> (back) into blk_new(). >=20 > As Kevin noted, that's not a good assumption. Which is a reason why we should have said explicit functions. >> All in all, you have convinced me that the commit message is incorrect= =2E >> It should state that blk_backends is effectively repurposed to contain= >> the list of all BBs, and that a more explicit monitor_block_backends >> list will take its place, with explicit operations for the monitor to >> signal when it takes or releases the reference to a BB. >=20 > A member of monitor_block_backends is always a member of blk_backends. > Correct? I sure hope so. > Is a member of blk_backends with a name always a member of > monitor_block_backends? No. Right now, after blk_new() it isn't; but Kevin proposed moving the name setting to monitor_add_blk(), then it will be. Apart from that, a BB that's passed to blk_delete() is currently generally still a member of blk_backends (with a name). As said above, I don't think it should be, and consequently it will not be a member of monitor_block_backends. So I guess you could say that I believe that being a member of blk_backends hand having a name before this patch should be equivalent to being a member of monitor_block_backends after this patch. It isn't, because blk_backends contained some BBs which shouldn't be there, in my opinion. Max --gC9hiAbSkuOkEAPiMiv2UUMmf2UdOXBTI 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 iQEcBAEBCAAGBQJWzcthAAoJEDuxQgLoOKytQKEIAKTuhQmNdaysdRt4t5jqwmJT /8APb18G5ys8U2AoDtNgHgPXwmt9kcOHYNBQlc0Qr+fJzOgR5F9ZQSHoESkn51hU KQXqjgHS+wA9955ChTmi+GNpcEGFWMacAHgby1bi0K4Xhr73WqnaiGxicj6ie8j9 5GSHYJ81wCwr+igDh+yJPr3c101YNoSaO+28HD+rYxMSu+TngTboa6LLJUR3fIcN jc4+1iOHSgsWfcF3nAdjxURAjr/AMEreO/9H1gY4dOgnkhNQcbrwnpT02TpWqpXm ejuY20He80kdO8mR1leHmDtQ1O7gVGxqzWBqpE7Fh5On4ToGUtO9J4nz0PoXONg= =pp5H -----END PGP SIGNATURE----- --gC9hiAbSkuOkEAPiMiv2UUMmf2UdOXBTI--