* [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" @ 2022-04-27 11:40 Hanna Reitz 2022-04-27 11:40 ` [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function Hanna Reitz ` (4 more replies) 0 siblings, 5 replies; 11+ messages in thread From: Hanna Reitz @ 2022-04-27 11:40 UTC (permalink / raw) To: qemu-block Cc: Kevin Wolf, Emanuele Giuseppe Esposito, Hanna Reitz, qemu-devel, Paolo Bonzini Hi, This assertion was disabled in commit b1c073490553f80594b903ceedfc7c1aef6b1b19. We wanted to re-enable it once the 7.1 tree has opened (which is now), but to do so, we should also fix the bug reported in https://gitlab.com/qemu-project/qemu/-/issues/945 . Patches 1 and 2 fix that problem (I hope), patch 3 re-enables the assertion, and patch 4 adds a regression test for issue 945. Hanna Reitz (4): block: Classify bdrv_get_flags() as I/O function qcow2: Do not reopen data_file in invalidate_cache Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" iotests: Add regression test for issue 945 include/block/block-global-state.h | 1 - include/block/block-io.h | 1 + include/qemu/main-loop.h | 3 +- block.c | 2 +- block/qcow2.c | 104 +++++++++++------- .../tests/export-incoming-iothread | 81 ++++++++++++++ .../tests/export-incoming-iothread.out | 5 + 7 files changed, 151 insertions(+), 46 deletions(-) create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out -- 2.35.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function 2022-04-27 11:40 [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz @ 2022-04-27 11:40 ` Hanna Reitz 2022-04-27 13:14 ` Eric Blake 2022-04-27 11:40 ` [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache Hanna Reitz ` (3 subsequent siblings) 4 siblings, 1 reply; 11+ messages in thread From: Hanna Reitz @ 2022-04-27 11:40 UTC (permalink / raw) To: qemu-block Cc: Kevin Wolf, Emanuele Giuseppe Esposito, Hanna Reitz, qemu-devel, Paolo Bonzini This function is safe to call in an I/O context, and qcow2_do_open() does so (invoked in an I/O context by qcow2_co_invalidate_cache()). Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/block/block-global-state.h | 1 - include/block/block-io.h | 1 + block.c | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 25bb69bbef..21265e3966 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -172,7 +172,6 @@ void bdrv_next_cleanup(BdrvNextIterator *it); BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs); void bdrv_iterate_format(void (*it)(void *opaque, const char *name), void *opaque, bool read_only); -int bdrv_get_flags(BlockDriverState *bs); char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp); char *bdrv_dirname(BlockDriverState *bs, Error **errp); diff --git a/include/block/block-io.h b/include/block/block-io.h index 5e3f346806..62c84f0519 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -103,6 +103,7 @@ int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg, bool bdrv_is_read_only(BlockDriverState *bs); bool bdrv_is_writable(BlockDriverState *bs); bool bdrv_is_sg(BlockDriverState *bs); +int bdrv_get_flags(BlockDriverState *bs); bool bdrv_is_inserted(BlockDriverState *bs); void bdrv_lock_medium(BlockDriverState *bs, bool locked); void bdrv_eject(BlockDriverState *bs, bool eject_flag); diff --git a/block.c b/block.c index 8cd16e757e..2c00dddd80 100644 --- a/block.c +++ b/block.c @@ -6298,7 +6298,7 @@ const char *bdrv_get_device_or_node_name(const BlockDriverState *bs) int bdrv_get_flags(BlockDriverState *bs) { - GLOBAL_STATE_CODE(); + IO_CODE(); return bs->open_flags; } -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function 2022-04-27 11:40 ` [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function Hanna Reitz @ 2022-04-27 13:14 ` Eric Blake 0 siblings, 0 replies; 11+ messages in thread From: Eric Blake @ 2022-04-27 13:14 UTC (permalink / raw) To: Hanna Reitz Cc: Kevin Wolf, Emanuele Giuseppe Esposito, qemu-devel, qemu-block, Paolo Bonzini On Wed, Apr 27, 2022 at 01:40:54PM +0200, Hanna Reitz wrote: > This function is safe to call in an I/O context, and qcow2_do_open() > does so (invoked in an I/O context by qcow2_co_invalidate_cache()). > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > include/block/block-global-state.h | 1 - > include/block/block-io.h | 1 + > block.c | 2 +- > 3 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache 2022-04-27 11:40 [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz 2022-04-27 11:40 ` [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function Hanna Reitz @ 2022-04-27 11:40 ` Hanna Reitz 2022-04-27 13:20 ` Eric Blake 2022-04-27 16:05 ` Kevin Wolf 2022-04-27 11:40 ` [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz ` (2 subsequent siblings) 4 siblings, 2 replies; 11+ messages in thread From: Hanna Reitz @ 2022-04-27 11:40 UTC (permalink / raw) To: qemu-block Cc: Kevin Wolf, Emanuele Giuseppe Esposito, Hanna Reitz, qemu-devel, Paolo Bonzini qcow2_co_invalidate_cache() closes and opens the qcow2 file, by calling qcow2_close() and qcow2_do_open(). These two functions must thus be usable from both a global-state and an I/O context. As they are, they are not safe to call in an I/O context, because they use bdrv_unref_child() and bdrv_open_child() to close/open the data_file child, respectively, both of which are global-state functions. When used from qcow2_co_invalidate_cache(), we do not need to close/open the data_file child, though (we do not do this for bs->file or bs->backing either), and so we should skip it in the qcow2_co_invalidate_cache() path. To do so, add a parameter to qcow2_do_open() and qcow2_close() to make them skip handling s->data_file, and have qcow2_co_invalidate_cache() exempt it from the memset() on the BDRVQcow2State. (Note that the QED driver similarly closes/opens the QED image by invoking bdrv_qed_close()+bdrv_qed_do_open(), but both functions seem safe to use in an I/O context.) Fixes: https://gitlab.com/qemu-project/qemu/-/issues/945 Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block/qcow2.c | 104 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 62 insertions(+), 42 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index b5c47931ef..4f5e6440fb 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1296,7 +1296,8 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp) /* Called with s->lock held. */ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, - int flags, Error **errp) + int flags, bool open_data_file, + Error **errp) { ERRP_GUARD(); BDRVQcow2State *s = bs->opaque; @@ -1614,50 +1615,52 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, goto fail; } - /* Open external data file */ - s->data_file = bdrv_open_child(NULL, options, "data-file", bs, - &child_of_bds, BDRV_CHILD_DATA, - true, errp); - if (*errp) { - ret = -EINVAL; - goto fail; - } + if (open_data_file) { + /* Open external data file */ + s->data_file = bdrv_open_child(NULL, options, "data-file", bs, + &child_of_bds, BDRV_CHILD_DATA, + true, errp); + if (*errp) { + ret = -EINVAL; + goto fail; + } - if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) { - if (!s->data_file && s->image_data_file) { - s->data_file = bdrv_open_child(s->image_data_file, options, - "data-file", bs, &child_of_bds, - BDRV_CHILD_DATA, false, errp); + if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) { + if (!s->data_file && s->image_data_file) { + s->data_file = bdrv_open_child(s->image_data_file, options, + "data-file", bs, &child_of_bds, + BDRV_CHILD_DATA, false, errp); + if (!s->data_file) { + ret = -EINVAL; + goto fail; + } + } if (!s->data_file) { + error_setg(errp, "'data-file' is required for this image"); ret = -EINVAL; goto fail; } - } - if (!s->data_file) { - error_setg(errp, "'data-file' is required for this image"); - ret = -EINVAL; - goto fail; - } - /* No data here */ - bs->file->role &= ~BDRV_CHILD_DATA; + /* No data here */ + bs->file->role &= ~BDRV_CHILD_DATA; - /* Must succeed because we have given up permissions if anything */ - bdrv_child_refresh_perms(bs, bs->file, &error_abort); - } else { - if (s->data_file) { - error_setg(errp, "'data-file' can only be set for images with an " - "external data file"); - ret = -EINVAL; - goto fail; - } + /* Must succeed because we have given up permissions if anything */ + bdrv_child_refresh_perms(bs, bs->file, &error_abort); + } else { + if (s->data_file) { + error_setg(errp, "'data-file' can only be set for images with " + "an external data file"); + ret = -EINVAL; + goto fail; + } - s->data_file = bs->file; + s->data_file = bs->file; - if (data_file_is_raw(bs)) { - error_setg(errp, "data-file-raw requires a data file"); - ret = -EINVAL; - goto fail; + if (data_file_is_raw(bs)) { + error_setg(errp, "data-file-raw requires a data file"); + ret = -EINVAL; + goto fail; + } } } @@ -1839,7 +1842,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, fail: g_free(s->image_data_file); - if (has_data_file(bs)) { + if (open_data_file && has_data_file(bs)) { bdrv_unref_child(bs, s->data_file); s->data_file = NULL; } @@ -1876,7 +1879,8 @@ static void coroutine_fn qcow2_open_entry(void *opaque) BDRVQcow2State *s = qoc->bs->opaque; qemu_co_mutex_lock(&s->lock); - qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp); + qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, true, + qoc->errp); qemu_co_mutex_unlock(&s->lock); } @@ -2714,7 +2718,7 @@ static int qcow2_inactivate(BlockDriverState *bs) return result; } -static void qcow2_close(BlockDriverState *bs) +static void qcow2_do_close(BlockDriverState *bs, bool close_data_file) { BDRVQcow2State *s = bs->opaque; qemu_vfree(s->l1_table); @@ -2740,7 +2744,7 @@ static void qcow2_close(BlockDriverState *bs) g_free(s->image_backing_file); g_free(s->image_backing_format); - if (has_data_file(bs)) { + if (close_data_file && has_data_file(bs)) { bdrv_unref_child(bs, s->data_file); s->data_file = NULL; } @@ -2749,11 +2753,17 @@ static void qcow2_close(BlockDriverState *bs) qcow2_free_snapshots(bs); } +static void qcow2_close(BlockDriverState *bs) +{ + qcow2_do_close(bs, true); +} + static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, Error **errp) { ERRP_GUARD(); BDRVQcow2State *s = bs->opaque; + BdrvChild *data_file; int flags = s->flags; QCryptoBlock *crypto = NULL; QDict *options; @@ -2767,14 +2777,24 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs, crypto = s->crypto; s->crypto = NULL; - qcow2_close(bs); + /* + * Do not reopen s->data_file (i.e., have qcow2_do_close() not close it, + * and then prevent qcow2_do_open() from opening it), because this function + * runs in the I/O path and as such we must not invoke global-state + * functions like bdrv_unref_child() and bdrv_open_child(). + */ + qcow2_do_close(bs, false); + + data_file = s->data_file; memset(s, 0, sizeof(BDRVQcow2State)); + s->data_file = data_file; + options = qdict_clone_shallow(bs->options); flags &= ~BDRV_O_INACTIVE; qemu_co_mutex_lock(&s->lock); - ret = qcow2_do_open(bs, options, flags, errp); + ret = qcow2_do_open(bs, options, flags, false, errp); qemu_co_mutex_unlock(&s->lock); qobject_unref(options); if (ret < 0) { -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache 2022-04-27 11:40 ` [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache Hanna Reitz @ 2022-04-27 13:20 ` Eric Blake 2022-04-27 16:05 ` Kevin Wolf 1 sibling, 0 replies; 11+ messages in thread From: Eric Blake @ 2022-04-27 13:20 UTC (permalink / raw) To: Hanna Reitz Cc: Kevin Wolf, Emanuele Giuseppe Esposito, qemu-devel, qemu-block, Paolo Bonzini On Wed, Apr 27, 2022 at 01:40:55PM +0200, Hanna Reitz wrote: > qcow2_co_invalidate_cache() closes and opens the qcow2 file, by calling > qcow2_close() and qcow2_do_open(). These two functions must thus be > usable from both a global-state and an I/O context. > > As they are, they are not safe to call in an I/O context, because they > use bdrv_unref_child() and bdrv_open_child() to close/open the data_file > child, respectively, both of which are global-state functions. When > used from qcow2_co_invalidate_cache(), we do not need to close/open the > data_file child, though (we do not do this for bs->file or bs->backing > either), and so we should skip it in the qcow2_co_invalidate_cache() > path. > > To do so, add a parameter to qcow2_do_open() and qcow2_close() to make > them skip handling s->data_file, and have qcow2_co_invalidate_cache() > exempt it from the memset() on the BDRVQcow2State. > > (Note that the QED driver similarly closes/opens the QED image by > invoking bdrv_qed_close()+bdrv_qed_do_open(), but both functions seem > safe to use in an I/O context.) > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/945 > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > block/qcow2.c | 104 ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 62 insertions(+), 42 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache 2022-04-27 11:40 ` [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache Hanna Reitz 2022-04-27 13:20 ` Eric Blake @ 2022-04-27 16:05 ` Kevin Wolf 1 sibling, 0 replies; 11+ messages in thread From: Kevin Wolf @ 2022-04-27 16:05 UTC (permalink / raw) To: Hanna Reitz Cc: Emanuele Giuseppe Esposito, Paolo Bonzini, qemu-devel, qemu-block Am 27.04.2022 um 13:40 hat Hanna Reitz geschrieben: > qcow2_co_invalidate_cache() closes and opens the qcow2 file, by calling > qcow2_close() and qcow2_do_open(). These two functions must thus be > usable from both a global-state and an I/O context. > > As they are, they are not safe to call in an I/O context, because they > use bdrv_unref_child() and bdrv_open_child() to close/open the data_file > child, respectively, both of which are global-state functions. When > used from qcow2_co_invalidate_cache(), we do not need to close/open the > data_file child, though (we do not do this for bs->file or bs->backing > either), and so we should skip it in the qcow2_co_invalidate_cache() > path. > > To do so, add a parameter to qcow2_do_open() and qcow2_close() to make > them skip handling s->data_file, and have qcow2_co_invalidate_cache() > exempt it from the memset() on the BDRVQcow2State. > > (Note that the QED driver similarly closes/opens the QED image by > invoking bdrv_qed_close()+bdrv_qed_do_open(), but both functions seem > safe to use in an I/O context.) > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/945 > Signed-off-by: Hanna Reitz <hreitz@redhat.com> This feels a bit like a hack, and we'll have to change it again if we ever want to allow changing the data_file with reopen. But it should do the job for now. Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" 2022-04-27 11:40 [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz 2022-04-27 11:40 ` [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function Hanna Reitz 2022-04-27 11:40 ` [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache Hanna Reitz @ 2022-04-27 11:40 ` Hanna Reitz 2022-04-27 13:22 ` Eric Blake 2022-04-27 11:40 ` [PATCH 4/4] iotests: Add regression test for issue 945 Hanna Reitz 2022-04-27 16:13 ` [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Kevin Wolf 4 siblings, 1 reply; 11+ messages in thread From: Hanna Reitz @ 2022-04-27 11:40 UTC (permalink / raw) To: qemu-block Cc: Kevin Wolf, Emanuele Giuseppe Esposito, Hanna Reitz, qemu-devel, Paolo Bonzini This reverts commit b1c073490553f80594b903ceedfc7c1aef6b1b19. (We wanted to do so once the 7.1 tree opens, which has happened. The issue reported in https://gitlab.com/qemu-project/qemu/-/issues/945 should be fixed by the preceding patches.) Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- include/qemu/main-loop.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index d3750c8e76..89bd9edefb 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -284,8 +284,7 @@ bool qemu_in_main_thread(void); #else #define GLOBAL_STATE_CODE() \ do { \ - /* FIXME: Re-enable after 7.0 release */ \ - /* assert(qemu_in_main_thread()); */ \ + assert(qemu_in_main_thread()); \ } while (0) #endif /* CONFIG_COCOA */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" 2022-04-27 11:40 ` [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz @ 2022-04-27 13:22 ` Eric Blake 0 siblings, 0 replies; 11+ messages in thread From: Eric Blake @ 2022-04-27 13:22 UTC (permalink / raw) To: Hanna Reitz Cc: Kevin Wolf, Emanuele Giuseppe Esposito, qemu-devel, qemu-block, Paolo Bonzini On Wed, Apr 27, 2022 at 01:40:56PM +0200, Hanna Reitz wrote: > This reverts commit b1c073490553f80594b903ceedfc7c1aef6b1b19. (We > wanted to do so once the 7.1 tree opens, which has happened. The issue > reported in https://gitlab.com/qemu-project/qemu/-/issues/945 should be > fixed by the preceding patches.) > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > include/qemu/main-loop.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> [Here's hoping we don't have to re-disable it because it finds more bugs - but maximizing the soak time during development with it enabled is good] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] iotests: Add regression test for issue 945 2022-04-27 11:40 [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz ` (2 preceding siblings ...) 2022-04-27 11:40 ` [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz @ 2022-04-27 11:40 ` Hanna Reitz 2022-04-27 13:52 ` Eric Blake 2022-04-27 16:13 ` [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Kevin Wolf 4 siblings, 1 reply; 11+ messages in thread From: Hanna Reitz @ 2022-04-27 11:40 UTC (permalink / raw) To: qemu-block Cc: Kevin Wolf, Emanuele Giuseppe Esposito, Hanna Reitz, qemu-devel, Paolo Bonzini Create a VM with a BDS in an iothread, add -incoming defer to the command line, and then export this BDS via NBD. Doing so should not fail an assertion. Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- .../tests/export-incoming-iothread | 81 +++++++++++++++++++ .../tests/export-incoming-iothread.out | 5 ++ 2 files changed, 86 insertions(+) create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out diff --git a/tests/qemu-iotests/tests/export-incoming-iothread b/tests/qemu-iotests/tests/export-incoming-iothread new file mode 100755 index 0000000000..7679e49103 --- /dev/null +++ b/tests/qemu-iotests/tests/export-incoming-iothread @@ -0,0 +1,81 @@ +#!/usr/bin/env python3 +# group: rw quick migration +# +# Regression test for issue 945: +# https://gitlab.com/qemu-project/qemu/-/issues/945 +# Test adding an export on top of an iothread-ed block device while in +# -incoming defer. +# +# Copyright (C) 2022 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +import os +import iotests +from iotests import qemu_img_create + + +image_size = 1 * 1024 * 1024 +test_img = os.path.join(iotests.test_dir, 'test.img') +node_name = 'node0' +iothread_id = 'iothr0' + +nbd_sock = os.path.join(iotests.sock_dir, 'nbd.sock') + + +class TestExportIncomingIothread(iotests.QMPTestCase): + def setUp(self) -> None: + qemu_img_create('-f', iotests.imgfmt, test_img, str(image_size)) + + self.vm = iotests.VM() + self.vm.add_object(f'iothread,id={iothread_id}') + self.vm.add_blockdev(( + f'driver={iotests.imgfmt}', + f'node-name={node_name}', + 'file.driver=file', + f'file.filename={test_img}' + )) + self.vm.add_incoming('defer') + self.vm.launch() + + def tearDown(self): + self.vm.shutdown() + os.remove(test_img) + + def test_export_add(self): + result = self.vm.qmp('nbd-server-start', { + 'addr': { + 'type': 'unix', + 'data': { + 'path': nbd_sock + } + } + }) + self.assert_qmp(result, 'return', {}) + + # Regression test for issue 945: This should not fail an assertion + result = self.vm.qmp('block-export-add', { + 'type': 'nbd', + 'id': 'exp0', + 'node-name': node_name, + 'iothread': iothread_id + }) + self.assert_qmp(result, 'return', {}) + + +if __name__ == '__main__': + iotests.main(supported_fmts=['generic'], + unsupported_fmts=['luks'], # Would need a secret + supported_protocols=['file']) diff --git a/tests/qemu-iotests/tests/export-incoming-iothread.out b/tests/qemu-iotests/tests/export-incoming-iothread.out new file mode 100644 index 0000000000..ae1213e6f8 --- /dev/null +++ b/tests/qemu-iotests/tests/export-incoming-iothread.out @@ -0,0 +1,5 @@ +. +---------------------------------------------------------------------- +Ran 1 tests + +OK -- 2.35.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] iotests: Add regression test for issue 945 2022-04-27 11:40 ` [PATCH 4/4] iotests: Add regression test for issue 945 Hanna Reitz @ 2022-04-27 13:52 ` Eric Blake 0 siblings, 0 replies; 11+ messages in thread From: Eric Blake @ 2022-04-27 13:52 UTC (permalink / raw) To: Hanna Reitz Cc: Kevin Wolf, Emanuele Giuseppe Esposito, qemu-devel, qemu-block, Paolo Bonzini On Wed, Apr 27, 2022 at 01:40:57PM +0200, Hanna Reitz wrote: > Create a VM with a BDS in an iothread, add -incoming defer to the > command line, and then export this BDS via NBD. Doing so should not > fail an assertion. > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > .../tests/export-incoming-iothread | 81 +++++++++++++++++++ > .../tests/export-incoming-iothread.out | 5 ++ > 2 files changed, 86 insertions(+) > create mode 100755 tests/qemu-iotests/tests/export-incoming-iothread > create mode 100644 tests/qemu-iotests/tests/export-incoming-iothread.out The test looks sane, and appears to match the formula posted in issue 945. When applying just patches 3-4 (skipping 1-2), './check export-incoming-iothread' passed, but './check -qcow2 export-incoming-iothread' failed; so the image format is important. And the failure was rather verbose, which is a GOOD thing - our efforts to make the python framework point out abnormal exits is working! +WARNING:qemu.machine.machine:qemu received signal 6; command: "/home/eblake/qemu/build/tests/qemu-iotests/... ... +Traceback (most recent call last): + File "/home/eblake/qemu/tests/qemu-iotests/tests/export-incoming-iothread", line 69, in test_export_add + result = self.vm.qmp('block-export-add', { ... + File "/home/eblake/qemu/python/qemu/qmp/qmp_client.py", line 463, in _reply + raise result +qemu.qmp.qmp_client.ExecInterruptedError: Disconnected + +====================================================================== +ERROR: test_export_add (__main__.TestExportIncomingIothread) +---------------------------------------------------------------------- +Traceback (most recent call last): + File "/home/eblake/qemu/python/qemu/machine/machine.py", line 533, in _soft_shutdown + self.qmp('quit') ... +qemu.qmp.protocol.StateError: QMPClient is disconnecting. Call disconnect() to return to IDLE state. + +During handling of the above exception, another exception occurred: + +Traceback (most recent call last): + File "/home/eblake/qemu/python/qemu/machine/machine.py", line 554, in _do_shutdown + self._soft_shutdown(timeout) ... + File "/home/eblake/qemu/python/qemu/qmp/protocol.py", line 968, in _readline + raise EOFError +EOFError + +The above exception was the direct cause of the following exception: + +Traceback (most recent call last): + File "/home/eblake/qemu/tests/qemu-iotests/tests/export-incoming-iothread", line 54, in tearDown + self.vm.shutdown() + File "/home/eblake/qemu/python/qemu/machine/machine.py", line 583, in shutdown + self._do_shutdown(timeout) + File "/home/eblake/qemu/python/qemu/machine/machine.py", line 557, in _do_shutdown + raise AbnormalShutdown("Could not perform graceful shutdown") \ +qemu.machine.machine.AbnormalShutdown: Could not perform graceful shutdown + ---------------------------------------------------------------------- Ran 1 tests -OK +FAILED (errors=2) Then applying patches 1-2 and repeating the test passed, so your test is a good proof that we identified and fixed the problem at hand. Reviewed-by: Eric Blake <eblake@redhat.com> Tested-by: Eric Blake <eblake@redhat.com> Given that it is visible with NBD, I'm happy to queue this series through my tree if no one else grabs it first. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" 2022-04-27 11:40 [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz ` (3 preceding siblings ...) 2022-04-27 11:40 ` [PATCH 4/4] iotests: Add regression test for issue 945 Hanna Reitz @ 2022-04-27 16:13 ` Kevin Wolf 4 siblings, 0 replies; 11+ messages in thread From: Kevin Wolf @ 2022-04-27 16:13 UTC (permalink / raw) To: Hanna Reitz Cc: Emanuele Giuseppe Esposito, Paolo Bonzini, qemu-devel, qemu-block Am 27.04.2022 um 13:40 hat Hanna Reitz geschrieben: > Hi, > > This assertion was disabled in commit > b1c073490553f80594b903ceedfc7c1aef6b1b19. We wanted to re-enable it > once the 7.1 tree has opened (which is now), but to do so, we should > also fix the bug reported in > https://gitlab.com/qemu-project/qemu/-/issues/945 . > > Patches 1 and 2 fix that problem (I hope), patch 3 re-enables the > assertion, and patch 4 adds a regression test for issue 945. Thanks, applied to the block branch. Kevin ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-27 16:15 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-04-27 11:40 [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz 2022-04-27 11:40 ` [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function Hanna Reitz 2022-04-27 13:14 ` Eric Blake 2022-04-27 11:40 ` [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache Hanna Reitz 2022-04-27 13:20 ` Eric Blake 2022-04-27 16:05 ` Kevin Wolf 2022-04-27 11:40 ` [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Hanna Reitz 2022-04-27 13:22 ` Eric Blake 2022-04-27 11:40 ` [PATCH 4/4] iotests: Add regression test for issue 945 Hanna Reitz 2022-04-27 13:52 ` Eric Blake 2022-04-27 16:13 ` [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions" Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).