From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54882) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a49gj-0000g3-TH for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:51:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a49gi-0003B9-NK for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:51:25 -0500 References: <1447108773-6836-1-git-send-email-mreitz@redhat.com> <1447108773-6836-15-git-send-email-mreitz@redhat.com> <20151130153621.GF4494@noname.str.redhat.com> <565C85EB.7030504@redhat.com> <20151201131649.GE6527@noname.str.redhat.com> From: Max Reitz Message-ID: <565F1373.7000008@redhat.com> Date: Wed, 2 Dec 2015 16:51:15 +0100 MIME-Version: 1.0 In-Reply-To: <20151201131649.GE6527@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FV7P5NQfjqQvW0FUSGftxfnjJod2hR6wn" Subject: Re: [Qemu-devel] [PATCH v7 14/24] nbd: Switch from close to eject notifier 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) --FV7P5NQfjqQvW0FUSGftxfnjJod2hR6wn Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 01.12.2015 14:16, Kevin Wolf wrote: > Am 30.11.2015 um 18:22 hat Max Reitz geschrieben: >> On 30.11.2015 16:36, Kevin Wolf wrote: >>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben: >>>> The NBD code uses the BDS close notifier to determine when a medium = is >>>> ejected. However, now it should use the BB's BDS removal notifier fo= r >>>> that instead of the BDS's close notifier. >>>> >>>> Signed-off-by: Max Reitz >>>> --- >>>> blockdev-nbd.c | 37 +------------------------------------ >>>> nbd.c | 13 +++++++++++++ >>>> 2 files changed, 14 insertions(+), 36 deletions(-) >>>> >>>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c >>>> index bcdd18b..b28a55b 100644 >>>> --- a/blockdev-nbd.c >>>> +++ b/blockdev-nbd.c >>>> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, E= rror **errp) >>>> } >>>> } >>>> =20 >>>> -/* >>>> - * Hook into the BlockBackend notifiers to close the export when th= e >>>> - * backend is closed. >>>> - */ >>>> -typedef struct NBDCloseNotifier { >>>> - Notifier n; >>>> - NBDExport *exp; >>>> - QTAILQ_ENTRY(NBDCloseNotifier) next; >>>> -} NBDCloseNotifier; >>>> - >>>> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =3D >>>> - QTAILQ_HEAD_INITIALIZER(close_notifiers); >>>> - >>>> -static void nbd_close_notifier(Notifier *n, void *data) >>>> -{ >>>> - NBDCloseNotifier *cn =3D DO_UPCAST(NBDCloseNotifier, n, n); >>>> - >>>> - notifier_remove(&cn->n); >>>> - QTAILQ_REMOVE(&close_notifiers, cn, next); >>>> - >>>> - nbd_export_close(cn->exp); >>>> - nbd_export_put(cn->exp); >>> >>> Here you remove a close/put pair, but in the new code you only add th= e >>> close call. Why is this correct? >> >> Thanks for putting so much unfounded faith in me. :-) >> >> After having put quite some time into looking into it, first I have to= >> say that it's a very good question. >> >> First off, it's wrong. This is because I thought every export would ha= ve >> a refcount of one. Turns out this is wrong, they have a refcount of tw= o: >> It is created with a refcount of one, and then it gains another by >> giving it a name and entering it into the export list. >> >> I did know about the second but I completely forgot about the former. >> And indeed I do think it is wrong. qmp_nbd_server_add() does not keep >> the reference to the export, once the function returns, it is gone. >> Therefore, it should call nbd_export_put() before returning. >> >> So in my opinion the current code is wrong and I didn't fix it enough.= >> *cough* >> >> So, with the nbd_export_put() added to qmp_nbd_server_add(), every >> export will have a refcount of 1 + |clients|. The eject notifier will >> call nbd_close_notifier(), which first disconnects the clients, thus >> reducing the refcount by |clients|, and then sets the name to NULL, th= us >> dropping the final refcount and destroying the export. >> >> In the old code, we needed another nbd_export_put() because of the >> superfluous reference from nbd_export_new(), which doesn't actually >> exist for blockdev-nbd (it does for qemu-nbd, though). >=20 > Okay, that makes sense to me. So first a patch that moves the existing > nbd_export_put() call from nbd_close_notifier() to qmp_nbd_server_add()= > and then this one on top? I would have put both in the same patch, but this does make more sense. I'll do so. Max --FV7P5NQfjqQvW0FUSGftxfnjJod2hR6wn 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 iQEcBAEBCAAGBQJWXxNzAAoJEDuxQgLoOKytN44H/RTE7ex9hlZgdJTBPfxlWASj C/tUuKS89il3E7jHwntwgDjIgxzdux8Vtu7IHPZbrMXBGIXdD59XHrbmDfoeNadM vixwACp43Nr7pZmACXggOs6TZBuWf+F8TxZ2DL2dVR7VPbBhn0jZcortYMnfnMw8 Mf0znlwNXlqT2/wfl2eH7nI0hYENV+rGvb+4nMDZOD1vr2WdqKMrVfzcP1NReADc J5b9jiXDikOpu2l+Wu3I/lsL5MMxnp6Byys5Tglvljni7wefWe/DRRzMhNeM2Zb0 QauT+I4GDBgSOAHJ89BTvoC7k76IkDVVgZe1q9FdeRa6gimWQ5LGvG9gMO9bfEw= =jn3B -----END PGP SIGNATURE----- --FV7P5NQfjqQvW0FUSGftxfnjJod2hR6wn--