| Message ID | 20200813162935.210070-14-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
On 13.08.20 18:29, Kevin Wolf wrote: > Having a refcount makes sense for all types of block exports. It is also > a prerequisite for keeping a list of all exports at the BlockExport > level. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/export.h | 10 ++++++ > include/block/nbd.h | 2 -- > block/export/export.c | 14 ++++++++ > blockdev-nbd.c | 2 +- > nbd/server.c | 72 +++++++++++++++++++----------------------- > 5 files changed, 58 insertions(+), 42 deletions(-) [...] > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 8dd127af52..a8b7b785e7 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) > /* 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. */ > - nbd_export_put(exp); > + blk_exp_unref((BlockExport*) exp); :/ Less so because of the asterisk, but more so because of “another instance of a cast because we can’t access a BlockExport’s fields. > out: > aio_context_release(aio_context); > diff --git a/nbd/server.c b/nbd/server.c > index 4c594e6558..2bf30bb731 100644 > --- a/nbd/server.c > +++ b/nbd/server.c [...] > @@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > > exp = g_new0(NBDExport, 1); > exp->common = (BlockExport) { > - .drv = &blk_exp_nbd, > + .drv = &blk_exp_nbd, > + .refcount = 1, > }; This makes me wish... Ah, for patch 16, I see. :) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 8/13/20 11:29 AM, Kevin Wolf wrote: > Having a refcount makes sense for all types of block exports. It is also > a prerequisite for keeping a list of all exports at the BlockExport > level. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > +++ b/include/block/export.h > @@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport; > typedef struct BlockExportDriver { > BlockExportType type; > BlockExport *(*create)(BlockExportOptions *, Error **); > + void (*delete)(BlockExport *); > } BlockExportDriver; > > struct BlockExport { > const BlockExportDriver *drv; > + > + /* > + * Reference count for this block export. This includes strong references > + * both from the owner (qemu-nbd or the monitor) and clients connected to > + * the export. I guess 'the monitor' includes qemu-storage-daemon. > + */ > + int refcount; > }; > > extern const BlockExportDriver blk_exp_nbd; > > BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp); > +void blk_exp_ref(BlockExport *exp); > +void blk_exp_unref(BlockExport *exp); Yay, I think this naming is more consistent with the rest of qemu... > > #endif > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 23030db3f1..af8509ab70 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk); > void nbd_export_close(NBDExport *exp); > void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp); > -void nbd_export_get(NBDExport *exp); > -void nbd_export_put(NBDExport *exp); ...as opposed to this which is common in kernel but less so in this project. No hard feelings from me :) > +++ b/blockdev-nbd.c > @@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) > /* 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. */ > - nbd_export_put(exp); > + blk_exp_unref((BlockExport*) exp); Even a helper function that converts NBDBlockExport* to BlockExport* rather than a cast might be nicer, but then again, I see from Max's review that this may be a temporary state of things. (The QAPI contains such type-safe container casts, such as qapi_DriveBackup_base(), if that helps...) > @@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > > exp = g_new0(NBDExport, 1); > exp->common = (BlockExport) { > - .drv = &blk_exp_nbd, > + .drv = &blk_exp_nbd, > + .refcount = 1, I'm not sure whether trying to align the '=' is good, because the moment you add a longer field name, every other line has to be touched. I'm fine with just one space on both side of =, even if it is more ragged to read. But you're the author, so you get to pick. > @@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > exp->ctx = ctx; > blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); > > + blk_exp_ref(&exp->common); > QTAILQ_INSERT_TAIL(&exports, exp, next); > - nbd_export_get(exp); > + Is there any consequence to this changed ordering in grabbing the reference vs. updating the list? Overall looks nice.
Am 19.08.2020 um 22:58 hat Eric Blake geschrieben: > On 8/13/20 11:29 AM, Kevin Wolf wrote: > > Having a refcount makes sense for all types of block exports. It is also > > a prerequisite for keeping a list of all exports at the BlockExport > > level. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > > +++ b/include/block/export.h > > @@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport; > > typedef struct BlockExportDriver { > > BlockExportType type; > > BlockExport *(*create)(BlockExportOptions *, Error **); > > + void (*delete)(BlockExport *); > > } BlockExportDriver; > > struct BlockExport { > > const BlockExportDriver *drv; > > + > > + /* > > + * Reference count for this block export. This includes strong references > > + * both from the owner (qemu-nbd or the monitor) and clients connected to > > + * the export. > > I guess 'the monitor' includes qemu-storage-daemon. Yes, qemu-storage-daemon has a QMP monitor, so I would count it there. Even the --export command line option only calls the QMP command internally. > > + */ > > + int refcount; > > }; > > extern const BlockExportDriver blk_exp_nbd; > > BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp); > > +void blk_exp_ref(BlockExport *exp); > > +void blk_exp_unref(BlockExport *exp); > > Yay, I think this naming is more consistent with the rest of qemu... > > > #endif > > diff --git a/include/block/nbd.h b/include/block/nbd.h > > index 23030db3f1..af8509ab70 100644 > > --- a/include/block/nbd.h > > +++ b/include/block/nbd.h > > @@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > > void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk); > > void nbd_export_close(NBDExport *exp); > > void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp); > > -void nbd_export_get(NBDExport *exp); > > -void nbd_export_put(NBDExport *exp); > > ...as opposed to this which is common in kernel but less so in this project. > No hard feelings from me :) > > > +++ b/blockdev-nbd.c > > @@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) > > /* 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. */ > > - nbd_export_put(exp); > > + blk_exp_unref((BlockExport*) exp); > > Even a helper function that converts NBDBlockExport* to BlockExport* rather > than a cast might be nicer, but then again, I see from Max's review that > this may be a temporary state of things. > (The QAPI contains such type-safe container casts, such as > qapi_DriveBackup_base(), if that helps...) Yes, this goes away before the end of the series. > > @@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > > exp = g_new0(NBDExport, 1); > > exp->common = (BlockExport) { > > - .drv = &blk_exp_nbd, > > + .drv = &blk_exp_nbd, > > + .refcount = 1, > > I'm not sure whether trying to align the '=' is good, because the moment you > add a longer field name, every other line has to be touched. I'm fine with > just one space on both side of =, even if it is more ragged to read. But > you're the author, so you get to pick. I generally prefer aligned '=' because the code is read much more often than it is written or modified, so being friendly for readers is important. > > @@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs, > > exp->ctx = ctx; > > blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); > > + blk_exp_ref(&exp->common); > > QTAILQ_INSERT_TAIL(&exports, exp, next); > > - nbd_export_get(exp); > > + > > Is there any consequence to this changed ordering in grabbing the reference > vs. updating the list? No intended consequences, but if Max is right that the code (before and after this series) lacks some locking, it might make a theoretical difference. If it does, the new code is safer than the old one. If it doesn't, it's just more consistent with the order I'm used to see in other places: First take the reference, then use it. Kevin
diff --git a/include/block/export.h b/include/block/export.h index 5424bdc85d..f44290a4a2 100644 --- a/include/block/export.h +++ b/include/block/export.h @@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport; typedef struct BlockExportDriver { BlockExportType type; BlockExport *(*create)(BlockExportOptions *, Error **); + void (*delete)(BlockExport *); } BlockExportDriver; struct BlockExport { const BlockExportDriver *drv; + + /* + * Reference count for this block export. This includes strong references + * both from the owner (qemu-nbd or the monitor) and clients connected to + * the export. + */ + int refcount; }; extern const BlockExportDriver blk_exp_nbd; BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp); +void blk_exp_ref(BlockExport *exp); +void blk_exp_unref(BlockExport *exp); #endif diff --git a/include/block/nbd.h b/include/block/nbd.h index 23030db3f1..af8509ab70 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk); void nbd_export_close(NBDExport *exp); void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp); -void nbd_export_get(NBDExport *exp); -void nbd_export_put(NBDExport *exp); AioContext *nbd_export_aio_context(NBDExport *exp); NBDExport *nbd_export_find(const char *name); diff --git a/block/export/export.c b/block/export/export.c index 12672228c7..1d5de564c7 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -49,6 +49,20 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) return drv->create(export, errp); } +void blk_exp_ref(BlockExport *exp) +{ + assert(exp->refcount > 0); + exp->refcount++; +} + +void blk_exp_unref(BlockExport *exp) +{ + assert(exp->refcount > 0); + if (--exp->refcount == 0) { + exp->drv->delete(exp); + } +} + void qmp_block_export_add(BlockExportOptions *export, Error **errp) { blk_exp_add(export, errp); diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 8dd127af52..a8b7b785e7 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) /* 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. */ - nbd_export_put(exp); + blk_exp_unref((BlockExport*) exp); out: aio_context_release(aio_context); diff --git a/nbd/server.c b/nbd/server.c index 4c594e6558..2bf30bb731 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -83,7 +83,6 @@ struct NBDRequestData { struct NBDExport { BlockExport common; - int refcount; BlockBackend *blk; char *name; @@ -499,7 +498,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, } QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); - nbd_export_get(client->exp); + blk_exp_ref(&client->exp->common); nbd_check_meta_export(client); return 0; @@ -707,7 +706,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) client->exp = exp; client->check_align = check_align; QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); - nbd_export_get(client->exp); + blk_exp_ref(&client->exp->common); nbd_check_meta_export(client); rc = 1; } @@ -1406,7 +1405,7 @@ void nbd_client_put(NBDClient *client) g_free(client->tlsauthz); if (client->exp) { QTAILQ_REMOVE(&client->exp->clients, client, next); - nbd_export_put(client->exp); + blk_exp_unref(&client->exp->common); } g_free(client); } @@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, exp = g_new0(NBDExport, 1); exp->common = (BlockExport) { - .drv = &blk_exp_nbd, + .drv = &blk_exp_nbd, + .refcount = 1, }; /* @@ -1566,7 +1566,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, blk_set_enable_write_cache(blk, !writethrough); blk_set_allow_aio_context_change(blk, true); - exp->refcount = 1; QTAILQ_INIT(&exp->clients); exp->blk = blk; exp->name = g_strdup(name); @@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs, exp->ctx = ctx; blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); + blk_exp_ref(&exp->common); QTAILQ_INSERT_TAIL(&exports, exp, next); - nbd_export_get(exp); + return exp; fail: @@ -1660,7 +1660,7 @@ void nbd_export_close(NBDExport *exp) { NBDClient *client, *next; - nbd_export_get(exp); + blk_exp_ref(&exp->common); /* * TODO: Should we expand QMP NbdServerRemoveNode enum to allow a * close mode that stops advertising the export to new clients but @@ -1672,13 +1672,13 @@ void nbd_export_close(NBDExport *exp) client_close(client, true); } if (exp->name) { - nbd_export_put(exp); + blk_exp_unref(&exp->common); g_free(exp->name); exp->name = NULL; QTAILQ_REMOVE(&exports, exp, next); QTAILQ_INSERT_TAIL(&closed_exports, exp, next); } - nbd_export_put(exp); + blk_exp_unref(&exp->common); } void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp) @@ -1695,47 +1695,41 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp) error_append_hint(errp, "Use mode='hard' to force client disconnect\n"); } -void nbd_export_get(NBDExport *exp) -{ - assert(exp->refcount > 0); - exp->refcount++; -} - -void nbd_export_put(NBDExport *exp) +static void nbd_export_delete(BlockExport *blk_exp) { - assert(exp->refcount > 0); - if (--exp->refcount == 0) { - assert(exp->name == NULL); - assert(QTAILQ_EMPTY(&exp->clients)); + NBDExport *exp = container_of(blk_exp, NBDExport, common); - g_free(exp->description); - exp->description = NULL; + assert(exp->name == NULL); + assert(QTAILQ_EMPTY(&exp->clients)); - if (exp->blk) { - if (exp->eject_notifier_blk) { - notifier_remove(&exp->eject_notifier); - blk_unref(exp->eject_notifier_blk); - } - blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, - blk_aio_detach, exp); - blk_unref(exp->blk); - exp->blk = NULL; - } + g_free(exp->description); + exp->description = NULL; - if (exp->export_bitmap) { - bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false); - g_free(exp->export_bitmap_context); + if (exp->blk) { + if (exp->eject_notifier_blk) { + notifier_remove(&exp->eject_notifier); + blk_unref(exp->eject_notifier_blk); } + blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, + blk_aio_detach, exp); + blk_unref(exp->blk); + exp->blk = NULL; + } - QTAILQ_REMOVE(&closed_exports, exp, next); - g_free(exp); - aio_wait_kick(); + if (exp->export_bitmap) { + bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false); + g_free(exp->export_bitmap_context); } + + QTAILQ_REMOVE(&closed_exports, exp, next); + g_free(exp); + aio_wait_kick(); } const BlockExportDriver blk_exp_nbd = { .type = BLOCK_EXPORT_TYPE_NBD, .create = nbd_export_create, + .delete = nbd_export_delete, }; void nbd_export_close_all(void)
Having a refcount makes sense for all types of block exports. It is also a prerequisite for keeping a list of all exports at the BlockExport level. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/export.h | 10 ++++++ include/block/nbd.h | 2 -- block/export/export.c | 14 ++++++++ blockdev-nbd.c | 2 +- nbd/server.c | 72 +++++++++++++++++++----------------------- 5 files changed, 58 insertions(+), 42 deletions(-)