| Message ID | 20200813162935.210070-20-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
On 13.08.20 18:29, Kevin Wolf wrote: > The reference owned by the user/monitor that is created when adding the > export and dropped when removing it was tied to the 'exports' list in > nbd/server.c. Every block export will have a user reference, so move it > to the block export level and tie it to the 'block_exports' list in > block/export/export.c instead. This is necessary for introducing a QMP > command for removing exports. > > Note that exports are present in block_exports even after the user has > requested shutdown. This is different from NBD's exports where exports > are immediately removed on a shutdown request, even if they are still in > the process of shutting down. In order to avoid that the user still > interacts with an export that is shutting down (and possibly removes it > a second time), we need to remember if the user actually still owns it. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/export.h | 8 ++++++++ > block/export/export.c | 4 ++++ > blockdev-nbd.c | 5 ----- > nbd/server.c | 2 -- > 4 files changed, 12 insertions(+), 7 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 13.08.20 18:29, Kevin Wolf wrote: > The reference owned by the user/monitor that is created when adding the > export and dropped when removing it was tied to the 'exports' list in > nbd/server.c. Every block export will have a user reference, so move it > to the block export level and tie it to the 'block_exports' list in > block/export/export.c instead. This is necessary for introducing a QMP > command for removing exports. > > Note that exports are present in block_exports even after the user has > requested shutdown. This is different from NBD's exports where exports > are immediately removed on a shutdown request, even if they are still in > the process of shutting down. In order to avoid that the user still > interacts with an export that is shutting down (and possibly removes it > a second time), we need to remember if the user actually still owns it. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/export.h | 8 ++++++++ > block/export/export.c | 4 ++++ > blockdev-nbd.c | 5 ----- > nbd/server.c | 2 -- > 4 files changed, 12 insertions(+), 7 deletions(-) With this patch, there’s an abort in iotest 281. Perhaps because blk_exp_unref() is now done by blk_exp_request_shutdown() outside of where the AIO context is locked? Max
Am 19.08.2020 um 13:56 hat Max Reitz geschrieben: > On 13.08.20 18:29, Kevin Wolf wrote: > > The reference owned by the user/monitor that is created when adding the > > export and dropped when removing it was tied to the 'exports' list in > > nbd/server.c. Every block export will have a user reference, so move it > > to the block export level and tie it to the 'block_exports' list in > > block/export/export.c instead. This is necessary for introducing a QMP > > command for removing exports. > > > > Note that exports are present in block_exports even after the user has > > requested shutdown. This is different from NBD's exports where exports > > are immediately removed on a shutdown request, even if they are still in > > the process of shutting down. In order to avoid that the user still > > interacts with an export that is shutting down (and possibly removes it > > a second time), we need to remember if the user actually still owns it. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > include/block/export.h | 8 ++++++++ > > block/export/export.c | 4 ++++ > > blockdev-nbd.c | 5 ----- > > nbd/server.c | 2 -- > > 4 files changed, 12 insertions(+), 7 deletions(-) > > With this patch, there’s an abort in iotest 281. Perhaps because > blk_exp_unref() is now done by blk_exp_request_shutdown() outside of > where the AIO context is locked? I have two fixes locally that were related to failing qemu-iotests. I guess the first one might be for what you're seeing? Kevin diff --git a/block/export/export.c b/block/export/export.c index 71d17bd440..d021b98b74 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -105,9 +105,14 @@ void blk_exp_unref(BlockExport *exp) { assert(exp->refcount > 0); if (--exp->refcount == 0) { + AioContext *aio_context = exp->ctx; + + aio_context_acquire(aio_context); QLIST_REMOVE(exp, next); exp->drv->delete(exp); blk_unref(exp->blk); + aio_context_release(aio_context); + g_free(exp->id); g_free(exp); aio_wait_kick(); diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index e1eaaedb55..31ce9e6fe0 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -45,7 +45,7 @@ exports available: 0 {"execute":"nbd-server-add", "arguments":{"device":"nosuch"}} {"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}} {"execute":"nbd-server-add", "arguments":{"device":"n"}} -{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}} +{"error": {"class": "GenericError", "desc": "Block export id 'n' is already in use"}} {"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b2"}} {"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}} {"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b3"}} @@ -126,7 +126,7 @@ exports available: 0 {"execute":"nbd-server-add", "arguments":{"device":"nosuch"}} {"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}} {"execute":"nbd-server-add", "arguments":{"device":"n"}} -{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}} +{"error": {"class": "GenericError", "desc": "Block export id 'n' is already in use"}} {"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b2"}} {"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}} {"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2", "bitmap":"b3"}}
On 19.08.20 16:23, Kevin Wolf wrote: > Am 19.08.2020 um 13:56 hat Max Reitz geschrieben: >> On 13.08.20 18:29, Kevin Wolf wrote: >>> The reference owned by the user/monitor that is created when adding the >>> export and dropped when removing it was tied to the 'exports' list in >>> nbd/server.c. Every block export will have a user reference, so move it >>> to the block export level and tie it to the 'block_exports' list in >>> block/export/export.c instead. This is necessary for introducing a QMP >>> command for removing exports. >>> >>> Note that exports are present in block_exports even after the user has >>> requested shutdown. This is different from NBD's exports where exports >>> are immediately removed on a shutdown request, even if they are still in >>> the process of shutting down. In order to avoid that the user still >>> interacts with an export that is shutting down (and possibly removes it >>> a second time), we need to remember if the user actually still owns it. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> include/block/export.h | 8 ++++++++ >>> block/export/export.c | 4 ++++ >>> blockdev-nbd.c | 5 ----- >>> nbd/server.c | 2 -- >>> 4 files changed, 12 insertions(+), 7 deletions(-) >> >> With this patch, there’s an abort in iotest 281. Perhaps because >> blk_exp_unref() is now done by blk_exp_request_shutdown() outside of >> where the AIO context is locked? > > I have two fixes locally that were related to failing qemu-iotests. I > guess the first one might be for what you're seeing? > > Kevin > > diff --git a/block/export/export.c b/block/export/export.c > index 71d17bd440..d021b98b74 100644 > --- a/block/export/export.c > +++ b/block/export/export.c > @@ -105,9 +105,14 @@ void blk_exp_unref(BlockExport *exp) > { > assert(exp->refcount > 0); > if (--exp->refcount == 0) { If this is done without locking the context, should this be an atomic operation? > + AioContext *aio_context = exp->ctx; > + > + aio_context_acquire(aio_context); > QLIST_REMOVE(exp, next); > exp->drv->delete(exp); > blk_unref(exp->blk); > + aio_context_release(aio_context); > + But for the crash I was seeing, this should be sufficient, yes. Max
diff --git a/include/block/export.h b/include/block/export.h index 43229857b0..83f554b745 100644 --- a/include/block/export.h +++ b/include/block/export.h @@ -40,6 +40,14 @@ struct BlockExport { */ int refcount; + /* + * True if one of the references in refcount belongs to the user. After the + * user has dropped their reference, they may not e.g. remove the same + * export a second time (which would decrease the refcount without having + * it incremented first). + */ + bool user_owned; + /* * The AioContex whose lock needs to be held while calling * BlockExportDriver callbacks. diff --git a/block/export/export.c b/block/export/export.c index 72f1fab975..f94a81258a 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -78,6 +78,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) *exp = (BlockExport) { .drv = &blk_exp_nbd, .refcount = 1, + .user_owned = true, .id = g_strdup(export->id), }; @@ -117,6 +118,9 @@ void blk_exp_request_shutdown(BlockExport *exp) aio_context_acquire(aio_context); exp->drv->request_shutdown(exp); aio_context_release(aio_context); + + exp->user_owned = false; + blk_exp_unref(exp); } static bool blk_exp_has_type(BlockExportType type) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 1c7aa874ee..40013b7d64 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -231,11 +231,6 @@ int nbd_export_create(BlockExport *exp, BlockExportOptions *exp_args, goto out; } - /* The list of named exports has a strong reference to this export now and - * our only way of accessing it is through nbd_export_find(), so we can drop - * the strong reference that is @exp. */ - blk_exp_unref((BlockExport*) exp); - ret = 0; out: aio_context_release(aio_context); diff --git a/nbd/server.c b/nbd/server.c index 7e2976b81d..e3ac7f548b 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1616,7 +1616,6 @@ int nbd_export_new(BlockExport *blk_exp, BlockDriverState *bs, blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); - blk_exp_ref(&exp->common); QTAILQ_INSERT_TAIL(&exports, exp, next); return 0; @@ -1663,7 +1662,6 @@ static void nbd_export_request_shutdown(BlockExport *blk_exp) client_close(client, true); } if (exp->name) { - blk_exp_unref(&exp->common); g_free(exp->name); exp->name = NULL; QTAILQ_REMOVE(&exports, exp, next);
The reference owned by the user/monitor that is created when adding the export and dropped when removing it was tied to the 'exports' list in nbd/server.c. Every block export will have a user reference, so move it to the block export level and tie it to the 'block_exports' list in block/export/export.c instead. This is necessary for introducing a QMP command for removing exports. Note that exports are present in block_exports even after the user has requested shutdown. This is different from NBD's exports where exports are immediately removed on a shutdown request, even if they are still in the process of shutting down. In order to avoid that the user still interacts with an export that is shutting down (and possibly removes it a second time), we need to remember if the user actually still owns it. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/export.h | 8 ++++++++ block/export/export.c | 4 ++++ blockdev-nbd.c | 5 ----- nbd/server.c | 2 -- 4 files changed, 12 insertions(+), 7 deletions(-)