| Message ID | 20220314213226.362217-3-v.sementsov-og@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: nbd-export: select bitmap by node/name pair | expand |
On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru> > > Hi all! Current logic of relying on search through backing chain is not > safe neither convenient. > > Sometimes it leads to necessity of extra bitmap copying. Also, we are > going to add "snapshot-access" driver, to access some snapshot state > through NBD. And this driver is not formally a filter, and of course > it's not a COW format driver. So, searching through backing chain will > not work. Instead of widening the workaround of bitmap searching, let's > extend the interface so that user can select bitmap precisely. > > Note, that checking for bitmap active status is not copied to the new > API, I don't see a reason for it, user should understand the risks. And > anyway, bitmap from other node is unrelated to this export being > read-only or read-write. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru> > --- > blockdev-nbd.c | 8 +++++- > nbd/server.c | 63 +++++++++++++++++++++++++++--------------- > qapi/block-export.json | 5 +++- > qemu-nbd.c | 11 ++++++-- > 4 files changed, 61 insertions(+), 26 deletions(-) > > @@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, > } > exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps); > for (i = 0, bitmaps = arg->bitmaps; bitmaps; > - i++, bitmaps = bitmaps->next) { > - const char *bitmap = bitmaps->value; > + i++, bitmaps = bitmaps->next) > + { > + const char *bitmap; I'm not sure if our prevailing style splits { to its own line on a multi-line 'for'. But this is a cosmetic question, not one of correctness. > + case QTYPE_QDICT: > + bitmap = bitmaps->value->u.external.name; > + bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node, > + bitmap, NULL, errp); > + if (!bm) { > + ret = -ENOENT; > + goto fail; > + } > + break; > + default: > + abort(); Not sure if g_assert_not_reached() or __builtin_unreachable() would be any better here. I'm fine with the abort() for now. > +++ b/qapi/block-export.json > @@ -6,6 +6,7 @@ > ## > > { 'include': 'sockets.json' } > +{ 'include': 'block-core.json' } Hmm. Does this extra inclusion negatively impact qemu-storage-daemon, since that is why we created block-export.json in the first place (to minimize the stuff that qsd pulled in without needing all of block-core.json)? In other words, would it be better to move BlockDirtyBitmapOrStr to this file? Everything else looks okay with this patch.
17.03.2022 00:28, Eric Blake wrote: > On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote: >> From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru> >> >> Hi all! Current logic of relying on search through backing chain is not >> safe neither convenient. >> >> Sometimes it leads to necessity of extra bitmap copying. Also, we are >> going to add "snapshot-access" driver, to access some snapshot state >> through NBD. And this driver is not formally a filter, and of course >> it's not a COW format driver. So, searching through backing chain will >> not work. Instead of widening the workaround of bitmap searching, let's >> extend the interface so that user can select bitmap precisely. >> >> Note, that checking for bitmap active status is not copied to the new >> API, I don't see a reason for it, user should understand the risks. And >> anyway, bitmap from other node is unrelated to this export being >> read-only or read-write. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov-og@ya.ru> >> --- >> blockdev-nbd.c | 8 +++++- >> nbd/server.c | 63 +++++++++++++++++++++++++++--------------- >> qapi/block-export.json | 5 +++- >> qemu-nbd.c | 11 ++++++-- >> 4 files changed, 61 insertions(+), 26 deletions(-) >> > >> @@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, >> } >> exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps); >> for (i = 0, bitmaps = arg->bitmaps; bitmaps; >> - i++, bitmaps = bitmaps->next) { >> - const char *bitmap = bitmaps->value; >> + i++, bitmaps = bitmaps->next) >> + { >> + const char *bitmap; > > I'm not sure if our prevailing style splits { to its own line on a > multi-line 'for'. But this is a cosmetic question, not one of > correctness. > >> + case QTYPE_QDICT: >> + bitmap = bitmaps->value->u.external.name; >> + bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node, >> + bitmap, NULL, errp); >> + if (!bm) { >> + ret = -ENOENT; >> + goto fail; >> + } >> + break; >> + default: >> + abort(); > > Not sure if g_assert_not_reached() or __builtin_unreachable() would be > any better here. I'm fine with the abort() for now. > >> +++ b/qapi/block-export.json >> @@ -6,6 +6,7 @@ >> ## >> >> { 'include': 'sockets.json' } >> +{ 'include': 'block-core.json' } > > Hmm. Does this extra inclusion negatively impact qemu-storage-daemon, > since that is why we created block-export.json in the first place (to > minimize the stuff that qsd pulled in without needing all of > block-core.json)? In other words, would it be better to move > BlockDirtyBitmapOrStr to this file? And include block-export in block-core? Another alternative is to move BlockDirtyBitmapOrStr to a separate file included from both block-export and block-core but that seems to be too much. > > Everything else looks okay with this patch. >
On Mon, Mar 21, 2022 at 02:50:25PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > +++ b/qapi/block-export.json > > > @@ -6,6 +6,7 @@ > > > ## > > > { 'include': 'sockets.json' } > > > +{ 'include': 'block-core.json' } > > > > Hmm. Does this extra inclusion negatively impact qemu-storage-daemon, > > since that is why we created block-export.json in the first place (to > > minimize the stuff that qsd pulled in without needing all of > > block-core.json)? In other words, would it be better to move > > BlockDirtyBitmapOrStr to this file? > > And include block-export in block-core? Right now, we have: qapi/block-core.json "Block core (VM unrelated)" - includes {common,crypto,job,sockets}.json qapi/block-export.json "Block device exports" - includes sockets.json qapi/block.json "Additional block stuff (VM related)" - includes block-core.json Kevin, you forked off qapi/block-export.json. What do you propose here? > > Another alternative is to move BlockDirtyBitmapOrStr to a separate file included from both block-export and block-core but that seems to be too much. Indeed, that feels like a step too far; we already have confusion on which file to stick new stuff in, and adding another file won't help that.
17.03.2022 00:28, Eric Blake wrote: >> +++ b/qapi/block-export.json >> @@ -6,6 +6,7 @@ >> ## >> >> { 'include': 'sockets.json' } >> +{ 'include': 'block-core.json' } > Hmm. Does this extra inclusion negatively impact qemu-storage-daemon, > since that is why we created block-export.json in the first place (to > minimize the stuff that qsd pulled in without needing all of > block-core.json)? In other words, would it be better to move > BlockDirtyBitmapOrStr to this file? Actually, looking at storage-daemon/qapi/qapi-schema.json I see block-cores.json. That's block.json which is not mentioned in storage-daemon/qapi/qapi-schema.json. So, I think it's OK to keep simple include for now.
On Fri, Apr 08, 2022 at 11:27:42PM +0300, Vladimir Sementsov-Ogievskiy wrote: > 17.03.2022 00:28, Eric Blake wrote: > > > +++ b/qapi/block-export.json > > > @@ -6,6 +6,7 @@ > > > ## > > > { 'include': 'sockets.json' } > > > +{ 'include': 'block-core.json' } > > Hmm. Does this extra inclusion negatively impact qemu-storage-daemon, > > since that is why we created block-export.json in the first place (to > > minimize the stuff that qsd pulled in without needing all of > > block-core.json)? In other words, would it be better to move > > BlockDirtyBitmapOrStr to this file? > > Actually, looking at storage-daemon/qapi/qapi-schema.json I see block-cores.json. > > That's block.json which is not mentioned in storage-daemon/qapi/qapi-schema.json. > > So, I think it's OK to keep simple include for now. We're early enough in the 7.1 cycle that if someone proposes a reason why this would need to change, then we can adjust it. So for now, I'm adding Reviewed-by: Eric Blake <eblake@redhat.com> and queuing this series through my NBD tree.
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 9840d25a82..7f6531cba0 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -211,8 +211,14 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp) QAPI_CLONE_MEMBERS(BlockExportOptionsNbdBase, &export_opts->u.nbd, qapi_NbdServerAddOptions_base(arg)); if (arg->has_bitmap) { + BlockDirtyBitmapOrStr *el = g_new(BlockDirtyBitmapOrStr, 1); + + *el = (BlockDirtyBitmapOrStr) { + .type = QTYPE_QSTRING, + .u.local = g_strdup(arg->bitmap), + }; export_opts->u.nbd.has_bitmaps = true; - QAPI_LIST_PREPEND(export_opts->u.nbd.bitmaps, g_strdup(arg->bitmap)); + QAPI_LIST_PREPEND(export_opts->u.nbd.bitmaps, el); } /* diff --git a/nbd/server.c b/nbd/server.c index 5da884c2fc..3956602c0a 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1643,7 +1643,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, uint64_t perm, shared_perm; bool readonly = !exp_args->writable; bool shared = !exp_args->writable; - strList *bitmaps; + BlockDirtyBitmapOrStrList *bitmaps; size_t i; int ret; @@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, } exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps); for (i = 0, bitmaps = arg->bitmaps; bitmaps; - i++, bitmaps = bitmaps->next) { - const char *bitmap = bitmaps->value; + i++, bitmaps = bitmaps->next) + { + const char *bitmap; BlockDriverState *bs = blk_bs(blk); BdrvDirtyBitmap *bm = NULL; - while (bs) { - bm = bdrv_find_dirty_bitmap(bs, bitmap); - if (bm != NULL) { - break; + switch (bitmaps->value->type) { + case QTYPE_QSTRING: + bitmap = bitmaps->value->u.local; + while (bs) { + bm = bdrv_find_dirty_bitmap(bs, bitmap); + if (bm != NULL) { + break; + } + + bs = bdrv_filter_or_cow_bs(bs); } - bs = bdrv_filter_or_cow_bs(bs); - } + if (bm == NULL) { + ret = -ENOENT; + error_setg(errp, "Bitmap '%s' is not found", + bitmaps->value->u.local); + goto fail; + } - if (bm == NULL) { - ret = -ENOENT; - error_setg(errp, "Bitmap '%s' is not found", bitmap); - goto fail; + if (readonly && bdrv_is_writable(bs) && + bdrv_dirty_bitmap_enabled(bm)) { + ret = -EINVAL; + error_setg(errp, "Enabled bitmap '%s' incompatible with " + "readonly export", bitmap); + goto fail; + } + break; + case QTYPE_QDICT: + bitmap = bitmaps->value->u.external.name; + bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node, + bitmap, NULL, errp); + if (!bm) { + ret = -ENOENT; + goto fail; + } + break; + default: + abort(); } - if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) { - ret = -EINVAL; - goto fail; - } + assert(bm); - if (readonly && bdrv_is_writable(bs) && - bdrv_dirty_bitmap_enabled(bm)) { + if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) { ret = -EINVAL; - error_setg(errp, - "Enabled bitmap '%s' incompatible with readonly export", - bitmap); goto fail; } diff --git a/qapi/block-export.json b/qapi/block-export.json index f183522d0d..6afed472ff 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -6,6 +6,7 @@ ## { 'include': 'sockets.json' } +{ 'include': 'block-core.json' } ## # @NbdServerOptions: @@ -89,6 +90,7 @@ # @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with # the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect # each bitmap. +# Since 7.1 bitmap may be specified by node/name pair. # # @allocation-depth: Also export the allocation depth map for @device, so # the NBD client can use NBD_OPT_SET_META_CONTEXT with @@ -99,7 +101,8 @@ ## { 'struct': 'BlockExportOptionsNbd', 'base': 'BlockExportOptionsNbdBase', - 'data': { '*bitmaps': ['str'], '*allocation-depth': 'bool' } } + 'data': { '*bitmaps': ['BlockDirtyBitmapOrStr'], + '*allocation-depth': 'bool' } } ## # @BlockExportOptionsVhostUserBlk: diff --git a/qemu-nbd.c b/qemu-nbd.c index 713e7557a9..72138f703e 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -567,7 +567,7 @@ int main(int argc, char **argv) QDict *options = NULL; const char *export_name = NULL; /* defaults to "" later for server mode */ const char *export_description = NULL; - strList *bitmaps = NULL; + BlockDirtyBitmapOrStrList *bitmaps = NULL; bool alloc_depth = false; const char *tlscredsid = NULL; const char *tlshostname = NULL; @@ -687,7 +687,14 @@ int main(int argc, char **argv) alloc_depth = true; break; case 'B': - QAPI_LIST_PREPEND(bitmaps, g_strdup(optarg)); + { + BlockDirtyBitmapOrStr *el = g_new(BlockDirtyBitmapOrStr, 1); + *el = (BlockDirtyBitmapOrStr) { + .type = QTYPE_QSTRING, + .u.local = g_strdup(optarg), + }; + QAPI_LIST_PREPEND(bitmaps, el); + } break; case 'k': sockpath = optarg;