| Message ID | 1447108773-6836-15-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mreitz@redhat.com> > --- > 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, Error **errp) > } > } > > -/* > - * 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 = > - QTAILQ_HEAD_INITIALIZER(close_notifiers); > - > -static void nbd_close_notifier(Notifier *n, void *data) > -{ > - NBDCloseNotifier *cn = 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 the close call. Why is this correct? Kevin
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 <mreitz@redhat.com> >> --- >> 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, Error **errp) >> } >> } >> >> -/* >> - * 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 = >> - QTAILQ_HEAD_INITIALIZER(close_notifiers); >> - >> -static void nbd_close_notifier(Notifier *n, void *data) >> -{ >> - NBDCloseNotifier *cn = 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 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
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 for > >> that instead of the BDS's close notifier. > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > >> --- > >> 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, Error **errp) > >> } > >> } > >> > >> -/* > >> - * 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 = > >> - QTAILQ_HEAD_INITIALIZER(close_notifiers); > >> - > >> -static void nbd_close_notifier(Notifier *n, void *data) > >> -{ > >> - NBDCloseNotifier *cn = 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 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). 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? Kevin
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 for >>>> that instead of the BDS's close notifier. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> 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, Error **errp) >>>> } >>>> } >>>> >>>> -/* >>>> - * 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 = >>>> - QTAILQ_HEAD_INITIALIZER(close_notifiers); >>>> - >>>> -static void nbd_close_notifier(Notifier *n, void *data) >>>> -{ >>>> - NBDCloseNotifier *cn = 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 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). > > 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
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, Error **errp) } } -/* - * 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 = - QTAILQ_HEAD_INITIALIZER(close_notifiers); - -static void nbd_close_notifier(Notifier *n, void *data) -{ - NBDCloseNotifier *cn = 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); - g_free(cn); -} - void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, Error **errp) { BlockBackend *blk; NBDExport *exp; - NBDCloseNotifier *n; if (server_fd == -1) { error_setg(errp, "NBD server not running"); @@ -113,20 +87,11 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable, } nbd_export_set_name(exp, device); - - n = g_new0(NBDCloseNotifier, 1); - n->n.notify = nbd_close_notifier; - n->exp = exp; - blk_add_close_notifier(blk, &n->n); - QTAILQ_INSERT_TAIL(&close_notifiers, n, next); } void qmp_nbd_server_stop(Error **errp) { - while (!QTAILQ_EMPTY(&close_notifiers)) { - NBDCloseNotifier *cn = QTAILQ_FIRST(&close_notifiers); - nbd_close_notifier(&cn->n, nbd_export_get_blockdev(cn->exp)); - } + nbd_export_close_all(); if (server_fd != -1) { qemu_set_fd_handler(server_fd, NULL, NULL, NULL); diff --git a/nbd.c b/nbd.c index b3d9654..ab2e725 100644 --- a/nbd.c +++ b/nbd.c @@ -162,6 +162,8 @@ struct NBDExport { QTAILQ_ENTRY(NBDExport) next; AioContext *ctx; + + Notifier eject_notifier; }; static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports); @@ -1053,6 +1055,12 @@ static void blk_aio_detach(void *opaque) exp->ctx = NULL; } +static void nbd_eject_notifier(Notifier *n, void *data) +{ + NBDExport *exp = container_of(n, NBDExport, eject_notifier); + nbd_export_close(exp); +} + NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, uint32_t nbdflags, void (*close)(NBDExport *), Error **errp) @@ -1075,6 +1083,10 @@ NBDExport *nbd_export_new(BlockBackend *blk, off_t dev_offset, off_t size, exp->ctx = blk_get_aio_context(blk); blk_ref(blk); blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); + + exp->eject_notifier.notify = nbd_eject_notifier; + blk_add_remove_bs_notifier(blk, &exp->eject_notifier); + /* * NBD exports are used for non-shared storage migration. Make sure * that BDRV_O_INCOMING is cleared and the image is ready for write @@ -1154,6 +1166,7 @@ void nbd_export_put(NBDExport *exp) } if (exp->blk) { + notifier_remove(&exp->eject_notifier); blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach, exp); blk_unref(exp->blk);
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 <mreitz@redhat.com> --- blockdev-nbd.c | 37 +------------------------------------ nbd.c | 13 +++++++++++++ 2 files changed, 14 insertions(+), 36 deletions(-)