Re: [PATCH v4 03/25] assertions for block global state API
On 15.11.21 13:27, Emanuele Giuseppe Esposito wrote: @@ -586,12 +589,14 @@ void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent) void coroutine_fn bdrv_co_drain(BlockDriverState *bs) { assert(qemu_in_coroutine()); + assert(qemu_in_main_thread()); bdrv_drained_begin(bs); bdrv_drained_end(bs); } void bdrv_drain(BlockDriverState *bs) { + assert(qemu_in_main_thread()); bdrv_drained_begin(bs); bdrv_drained_end(bs); } Why are these GS functions when both bdrv_drained_begin() and bdrv_drained_end() are I/O functions? I can understand making the drain_all functions GS functions, but it seems weird to say it’s an I/O function when a single BDS is drained via bdrv_drained_begin() and bdrv_drained_end(), but not via bdrv_drain(), which just does both. (I can see that there are no I/O path callers, but I still find it strange.) The problem as you saw is on the path callers: bdrv_drain seems to be called only in main thread, while others or similar drains are also coming from I/O. I would say maybe it's a better idea to just put everything I/O, maybe (probably) there are cases not caught by the tests. The only ones in global-state will be: - bdrv_apply_subtree_drain and bdrv_unapply_subtree_drain (called only in .attach and .detach callbacks of BdrvChildClass, run under BQL). - bdrv_drain_all_end_quiesce (called only by bdrv_close thus under BQL). - bdrv_drain_all_begin, bdrv_drain_all_end and bdrv_drain_all (have already BQL assertions) In general, this is the underlying problem with these assertions: if a test doesn't test a specific code path, an unneeded assertion might pass undetected. I believe the I/O vs. GS classification should not only be done based on functional concerns, i.e. who calls this function (is it called by an I/O function?) and whether it can be thread-safe or not (does it call a BQL function?), but also needs to be done semantically. I believe there are some functions that we consider thread-safe but are also only called by BQL functions, for example apparently bdrv_drain(), bdrv_apply_subtree_drain(), and bdrv_unapply_subtree_drain(). Such functions can functionally be considered both GS and I/O functions, so the decision should be done semantically. I believe that it makes sense to say all drain-related functions for a single subtree are I/O functions. OTOH, functions operating on multiple trees in the block graph (i.e. all functions that have some “_all_” in their name) should naturally be considered GS functions, regardless of whether their implementation is thread-safe or not, because they are by nature global functions. That’s why I’m wondering why some drain functions are I/O and others are GS; or why this block-status function is I/O and this one is GS. It may make sense functionally, but semantically it’s strange. I understand it may be difficult for you to know which functions relate to each other and thus know these semantic relationships, but in most of the series the split seems very reasonable to me, semantically. If it didn’t, I said so. :) Hanna
Re: [PATCH v4 03/25] assertions for block global state API
@@ -5958,6 +6043,7 @@ const char *bdrv_get_parent_name(const BlockDriverState *bs) /* TODO check what callers really want: bs->node_name or blk_name() */ const char *bdrv_get_device_name(const BlockDriverState *bs) { + assert(qemu_in_main_thread()); return bdrv_get_parent_name(bs) ?: ""; } This function is invoked from qcow2_signal_corruption(), which comes generally from an I/O path. Is it safe to assert that we’re in the main thread here? Well, the question is probably rather whether this needs really be a considered a global-state function, or whether putting it in common or I/O is fine. I believe you’re right given that it invokes bdrv_get_parent_name(), it cannot be thread-safe, but then we’ll have to change qcow2_signal_corruption() so it doesn’t invoke this function. I think that the assertion is wrong here. As long as a single caller is not under BQL, we cannot keep the function in global-state headers. It should go into block-io.h, together with bdrv_get_parent_name and bdrv_get_device_or_node_name (caller of bdrv_get_parent_name). Since bdrv_get_parent_name only scans through the list of bs->parents, as long as we can assert that the parent list is modified only under BQL + drain, it is safe to be read and can be I/O. [...] diff --git a/block/io.c b/block/io.c index bb0a254def..c5d7f8495e 100644 --- a/block/io.c +++ b/block/io.c [...] @@ -544,6 +546,7 @@ void bdrv_drained_end(BlockDriverState *bs) void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter) { + assert(qemu_in_main_thread()); bdrv_do_drained_end(bs, false, NULL, false, drained_end_counter); } Why is bdrv_drained_end an I/O function and this is a GS function, even though it does just a subset? Right this is clearly called in bdrv_drained_end. I don't know how is it possible that no test managed to trigger this assertion... This is actually invoked in both unit and qemu-iothread tests... @@ -586,12 +589,14 @@ void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent) void coroutine_fn bdrv_co_drain(BlockDriverState *bs) { assert(qemu_in_coroutine()); + assert(qemu_in_main_thread()); bdrv_drained_begin(bs); bdrv_drained_end(bs); } void bdrv_drain(BlockDriverState *bs) { + assert(qemu_in_main_thread()); bdrv_drained_begin(bs); bdrv_drained_end(bs); } Why are these GS functions when both bdrv_drained_begin() and bdrv_drained_end() are I/O functions? I can understand making the drain_all functions GS functions, but it seems weird to say it’s an I/O function when a single BDS is drained via bdrv_drained_begin() and bdrv_drained_end(), but not via bdrv_drain(), which just does both. (I can see that there are no I/O path callers, but I still find it strange.) The problem as you saw is on the path callers: bdrv_drain seems to be called only in main thread, while others or similar drains are also coming from I/O. I would say maybe it's a better idea to just put everything I/O, maybe (probably) there are cases not caught by the tests. The only ones in global-state will be: - bdrv_apply_subtree_drain and bdrv_unapply_subtree_drain (called only in .attach and .detach callbacks of BdrvChildClass, run under BQL). - bdrv_drain_all_end_quiesce (called only by bdrv_close thus under BQL). - bdrv_drain_all_begin, bdrv_drain_all_end and bdrv_drain_all (have already BQL assertions) In general, this is the underlying problem with these assertions: if a test doesn't test a specific code path, an unneeded assertion might pass undetected. [...] @@ -2731,6 +2742,7 @@ int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base, int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes, int64_t *pnum, int64_t *map, BlockDriverState **file) { + assert(qemu_in_main_thread()); return bdrv_block_status_above(bs, bdrv_filter_or_cow_bs(bs), offset, bytes, pnum, map, file); } Why is this a GS function as opposed to all other block-status functions? Because of the bdrv_filter_or_cow_bs() call? This function is in block.io but has the assertion. I think it's a proper I/O but I forgot to remove the assertion in my various trial&errors. Let me know if you disagree on anything of what I said. Thank you, Emanuele
Re: [PATCH v4 03/25] assertions for block global state API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block.c| 136 +++-- block/commit.c | 2 + block/io.c | 20 blockdev.c | 1 + 4 files changed, 156 insertions(+), 3 deletions(-) bdrv_make_zero() seems missing here – it can be considered an I/O or a GS function, but patch 2 classified it as GS. Hanna
Re: [PATCH v4 03/25] assertions for block global state API
On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote: All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block.c| 136 +++-- block/commit.c | 2 + block/io.c | 20 blockdev.c | 1 + 4 files changed, 156 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 6fdb4d7712..672f946065 100644 --- a/block.c +++ b/block.c [...] @@ -5606,7 +5678,6 @@ int64_t bdrv_getlength(BlockDriverState *bs) void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr) { int64_t nb_sectors = bdrv_nb_sectors(bs); - *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors; } This hunk seems at least unrelated. [...] @@ -5958,6 +6043,7 @@ const char *bdrv_get_parent_name(const BlockDriverState *bs) /* TODO check what callers really want: bs->node_name or blk_name() */ const char *bdrv_get_device_name(const BlockDriverState *bs) { +assert(qemu_in_main_thread()); return bdrv_get_parent_name(bs) ?: ""; } This function is invoked from qcow2_signal_corruption(), which comes generally from an I/O path. Is it safe to assert that we’re in the main thread here? Well, the question is probably rather whether this needs really be a considered a global-state function, or whether putting it in common or I/O is fine. I believe you’re right given that it invokes bdrv_get_parent_name(), it cannot be thread-safe, but then we’ll have to change qcow2_signal_corruption() so it doesn’t invoke this function. [...] diff --git a/block/io.c b/block/io.c index bb0a254def..c5d7f8495e 100644 --- a/block/io.c +++ b/block/io.c [...] @@ -544,6 +546,7 @@ void bdrv_drained_end(BlockDriverState *bs) void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter) { +assert(qemu_in_main_thread()); bdrv_do_drained_end(bs, false, NULL, false, drained_end_counter); } Why is bdrv_drained_end an I/O function and this is a GS function, even though it does just a subset? @@ -586,12 +589,14 @@ void bdrv_unapply_subtree_drain(BdrvChild *child, BlockDriverState *old_parent) void coroutine_fn bdrv_co_drain(BlockDriverState *bs) { assert(qemu_in_coroutine()); +assert(qemu_in_main_thread()); bdrv_drained_begin(bs); bdrv_drained_end(bs); } void bdrv_drain(BlockDriverState *bs) { +assert(qemu_in_main_thread()); bdrv_drained_begin(bs); bdrv_drained_end(bs); } Why are these GS functions when both bdrv_drained_begin() and bdrv_drained_end() are I/O functions? I can understand making the drain_all functions GS functions, but it seems weird to say it’s an I/O function when a single BDS is drained via bdrv_drained_begin() and bdrv_drained_end(), but not via bdrv_drain(), which just does both. (I can see that there are no I/O path callers, but I still find it strange.) [...] @@ -2731,6 +2742,7 @@ int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base, int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes, int64_t *pnum, int64_t *map, BlockDriverState **file) { +assert(qemu_in_main_thread()); return bdrv_block_status_above(bs, bdrv_filter_or_cow_bs(bs), offset, bytes, pnum, map, file); } Why is this a GS function as opposed to all other block-status functions? Because of the bdrv_filter_or_cow_bs() call? And isn’t the call from nvme_block_status_all() basically an I/O path? (Or is that always run in the main thread?) @@ -2800,6 +2812,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, int64_t bytes, int64_t *pnum) { int depth; + int ret = bdrv_common_block_status_above(top, base, include_base, false, offset, bytes, pnum, NULL, NULL, &depth); This hunk too seems unrelated. Hanna
[PATCH v4 03/25] assertions for block global state API
All the global state (GS) API functions will check that qemu_in_main_thread() returns true. If not, it means that the safety of BQL cannot be guaranteed, and they need to be moved to I/O. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- block.c| 136 +++-- block/commit.c | 2 + block/io.c | 20 blockdev.c | 1 + 4 files changed, 156 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 6fdb4d7712..672f946065 100644 --- a/block.c +++ b/block.c @@ -386,6 +386,7 @@ char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp) void bdrv_register(BlockDriver *bdrv) { assert(bdrv->format_name); +assert(qemu_in_main_thread()); QLIST_INSERT_HEAD(&bdrv_drivers, bdrv, list); } @@ -394,6 +395,8 @@ BlockDriverState *bdrv_new(void) BlockDriverState *bs; int i; +assert(qemu_in_main_thread()); + bs = g_new0(BlockDriverState, 1); QLIST_INIT(&bs->dirty_bitmaps); for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) { @@ -421,6 +424,7 @@ BlockDriverState *bdrv_new(void) static BlockDriver *bdrv_do_find_format(const char *format_name) { BlockDriver *drv1; +assert(qemu_in_main_thread()); QLIST_FOREACH(drv1, &bdrv_drivers, list) { if (!strcmp(drv1->format_name, format_name)) { @@ -436,6 +440,8 @@ BlockDriver *bdrv_find_format(const char *format_name) BlockDriver *drv1; int i; +assert(qemu_in_main_thread()); + drv1 = bdrv_do_find_format(format_name); if (drv1) { return drv1; @@ -485,11 +491,13 @@ static int bdrv_format_is_whitelisted(const char *format_name, bool read_only) int bdrv_is_whitelisted(BlockDriver *drv, bool read_only) { +assert(qemu_in_main_thread()); return bdrv_format_is_whitelisted(drv->format_name, read_only); } bool bdrv_uses_whitelist(void) { +assert(qemu_in_main_thread()); return use_bdrv_whitelist; } @@ -520,6 +528,8 @@ int bdrv_create(BlockDriver *drv, const char* filename, { int ret; +assert(qemu_in_main_thread()); + Coroutine *co; CreateCo cco = { .drv = drv, @@ -695,6 +705,8 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) QDict *qdict; int ret; +assert(qemu_in_main_thread()); + drv = bdrv_find_protocol(filename, true, errp); if (drv == NULL) { return -ENOENT; @@ -792,6 +804,7 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) { BlockDriver *drv = bs->drv; BlockDriverState *filtered = bdrv_filter_bs(bs); +assert(qemu_in_main_thread()); if (drv && drv->bdrv_probe_blocksizes) { return drv->bdrv_probe_blocksizes(bs, bsz); @@ -812,6 +825,7 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo) { BlockDriver *drv = bs->drv; BlockDriverState *filtered = bdrv_filter_bs(bs); +assert(qemu_in_main_thread()); if (drv && drv->bdrv_probe_geometry) { return drv->bdrv_probe_geometry(bs, geo); @@ -866,6 +880,7 @@ static BlockDriver *find_hdev_driver(const char *filename) { int score_max = 0, score; BlockDriver *drv = NULL, *d; +assert(qemu_in_main_thread()); QLIST_FOREACH(d, &bdrv_drivers, list) { if (d->bdrv_probe_device) { @@ -883,6 +898,7 @@ static BlockDriver *find_hdev_driver(const char *filename) static BlockDriver *bdrv_do_find_protocol(const char *protocol) { BlockDriver *drv1; +assert(qemu_in_main_thread()); QLIST_FOREACH(drv1, &bdrv_drivers, list) { if (drv1->protocol_name && !strcmp(drv1->protocol_name, protocol)) { @@ -903,6 +919,7 @@ BlockDriver *bdrv_find_protocol(const char *filename, const char *p; int i; +assert(qemu_in_main_thread()); /* TODO Drivers without bdrv_file_open must be specified explicitly */ /* @@ -968,6 +985,7 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, { int score_max = 0, score; BlockDriver *drv = NULL, *d; +assert(qemu_in_main_thread()); QLIST_FOREACH(d, &bdrv_drivers, list) { if (d->bdrv_probe) { @@ -1115,6 +1133,7 @@ int bdrv_parse_aio(const char *mode, int *flags) */ int bdrv_parse_discard_flags(const char *mode, int *flags) { +assert(qemu_in_main_thread()); *flags &= ~BDRV_O_UNMAP; if (!strcmp(mode, "off") || !strcmp(mode, "ignore")) { @@ -1135,6 +1154,7 @@ int bdrv_parse_discard_flags(const char *mode, int *flags) */ int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough) { +assert(qemu_in_main_thread()); *flags &= ~BDRV_O_CACHE_MASK; if (!strcmp(mode, "off") || !strcmp(mode, "none")) { @@ -1499,6 +1519,7 @@ static void bdrv_assign_node_name(BlockDriverState *bs, Error **errp) { char *gen_node_name = NULL; +assert(qemu_in_main_thread()); if (!node_name) { node_name = gen_node_name = id_ge