From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3SAI-000329-JK for qemu-devel@nongnu.org; Mon, 30 Nov 2015 12:23:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3SAH-0003Ct-ID for qemu-devel@nongnu.org; Mon, 30 Nov 2015 12:23:02 -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> From: Max Reitz Message-ID: <565C85EB.7030504@redhat.com> Date: Mon, 30 Nov 2015 18:22:51 +0100 MIME-Version: 1.0 In-Reply-To: <20151130153621.GF4494@noname.str.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3chH3t021hVCEp1rHnAK7uUpMGvdTVoeP" 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) --3chH3t021hVCEp1rHnAK7uUpMGvdTVoeP Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 for >> 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, Err= or **errp) >> } >> } >> =20 >> -/* >> - * Hook into the BlockBackend notifiers to close the export when the >> - * 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); >=20 > Here you remove a close/put pair, but in the new code you only add the > 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 have a refcount of one. Turns out this is wrong, they have a refcount of two: 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, thus 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). Max --3chH3t021hVCEp1rHnAK7uUpMGvdTVoeP 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 iQEcBAEBCAAGBQJWXIXrAAoJEDuxQgLoOKyt3R0H/20m68Gej/5+1Q1O0aSq92U3 5upV4QEIti0ie/tkf1jRqHdCmwchYGi8sl7dsGFuaBzgdRIgVn/9KP4YCTfc3xsZ IpH/hl9G16JmvIUwvE/QMpF9LZbwVvDuUSl7BoHBUXFNDObLRIviVYKQlRBgYVbE PP9etk4Ee3u/uIARh7rYgWyZeJBmC1jGEiOOx8kN0XaT2tv33XaljsvcFB2gAV35 2QVj2hS7hetKB56VJAQeTmFLRfSX3gHdb2CaZ8zaAnJvRR18K86CD3vYiTHrnJUC 6DqvWZ++hYPuE/KO4xGm7dQPaPzUVCN+swTJflv538e0zEm2WGXix/b8n02Xkrs= =8JM3 -----END PGP SIGNATURE----- --3chH3t021hVCEp1rHnAK7uUpMGvdTVoeP--