[PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"

[PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"

[Hanna Reitz]

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

[PATCH 1/4] block: Classify bdrv_get_flags() as I/O function

[Hanna Reitz]

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

[PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache

[Hanna Reitz]

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

[PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"

[Hanna Reitz]

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

[PATCH 4/4] iotests: Add regression test for issue 945

[Hanna Reitz]

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

Re: [PATCH 1/4] block: Classify bdrv_get_flags() as I/O function

[Eric Blake]

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

Re: [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache

[Eric Blake]

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

Re: [PATCH 3/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"

[Eric Blake]

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

Re: [PATCH 4/4] iotests: Add regression test for issue 945

[Eric Blake]

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

Re: [PATCH 2/4] qcow2: Do not reopen data_file in invalidate_cache

[Kevin Wolf]

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

Re: [PATCH 0/4] Revert "main-loop: Disable GLOBAL_STATE_CODE() assertions"

[Kevin Wolf]

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