[PULL 0/6] Block layer patches

[PULL 0/6] Block layer patches

[Kevin Wolf]

The following changes since commit 74208cd252c5da9d867270a178799abd802b9338:

  Merge remote-tracking branch 'remotes/berrange-gitlab/tags/misc-fixes-pull-request' into staging (2021-01-29 19:51:25 +0000)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 47c9af7f4c78d03986aecc9afb0aab9b19d77cb5:

  iotests: Fix -makecheck output (2021-02-01 17:58:49 +0100)

----------------------------------------------------------------
Block layer patches:

- Fix double processing of nodes in bdrv_set_aio_context()
- Fix potential hang in block export shutdown
- iotests: Some more fixups for the 'check' rewrite
- MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs

----------------------------------------------------------------
Kevin Wolf (1):
      iotests: Fix -makecheck output

Sergio Lopez (2):
      block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
      block: move blk_exp_close_all() to qemu_cleanup()

Vladimir Sementsov-Ogievskiy (3):
      MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs
      iotests/297: pylint: ignore too many statements
      iotests: check: return 1 on failure

 block.c                              | 35 +++++++++++++++++++++++++++--------
 qemu-nbd.c                           |  1 +
 softmmu/runstate.c                   |  9 +++++++++
 storage-daemon/qemu-storage-daemon.c |  1 +
 tests/qemu-iotests/testrunner.py     | 10 +++++++---
 MAINTAINERS                          | 10 ++++++++++
 tests/qemu-iotests/check             |  5 ++++-
 tests/qemu-iotests/pylintrc          |  2 ++
 8 files changed, 61 insertions(+), 12 deletions(-)

[PULL 1/6] MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

I'm developing Qemu backup for several years, and finally new backup
architecture, including block-copy generic engine and backup-top filter
landed upstream, great thanks to reviewers and especially to
Max Reitz!

I also have plans of moving other block-jobs onto block-copy, so that
we finally have one generic block copying path, fast and well-formed.

So, now I suggest to bring all parts of backup architecture into
"Block Jobs" subsystem (actually, aio_task is shared with qcow2 and
qemu-co-shared-resource can be reused somewhere else, but I'd keep an
eye on them in context of block-jobs) and add myself as co-maintainer.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210128144144.27617-1-vsementsov@virtuozzo.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bcd88668bc..00626941f1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2210,6 +2210,7 @@ F: scsi/*
 
 Block Jobs
 M: John Snow <jsnow@redhat.com>
+M: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
 L: qemu-block@nongnu.org
 S: Supported
 F: blockjob.c
@@ -2222,7 +2223,16 @@ F: block/commit.c
 F: block/stream.c
 F: block/mirror.c
 F: qapi/job.json
+F: block/block-copy.c
+F: include/block/block-copy.c
+F: block/backup-top.h
+F: block/backup-top.c
+F: include/block/aio_task.h
+F: block/aio_task.c
+F: util/qemu-co-shared-resource.c
+F: include/qemu/co-shared-resource.h
 T: git https://gitlab.com/jsnow/qemu.git jobs
+T: git https://src.openvz.org/scm/~vsementsov/qemu.git jobs
 
 Block QAPI, monitor, command line
 M: Markus Armbruster <armbru@redhat.com>
-- 
2.29.2

[PULL 2/6] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()

[Kevin Wolf]

From: Sergio Lopez <slp@redhat.com>

Some graphs may contain an indirect reference to the first BDS in the
chain that can be reached while walking it bottom->up from one its
children.

Doubling-processing of a BDS is especially problematic for the
aio_notifiers, as they might attempt to work on both the old and the
new AIO contexts.

To avoid this problem, add every child and parent to the ignore list
before actually processing them.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Sergio Lopez <slp@redhat.com>
Message-Id: <20210201125032.44713-2-slp@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 91a66d4f3e..5c428e1595 100644
--- a/block.c
+++ b/block.c
@@ -6439,7 +6439,10 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
                                  AioContext *new_context, GSList **ignore)
 {
     AioContext *old_context = bdrv_get_aio_context(bs);
-    BdrvChild *child;
+    GSList *children_to_process = NULL;
+    GSList *parents_to_process = NULL;
+    GSList *entry;
+    BdrvChild *child, *parent;
 
     g_assert(qemu_get_current_aio_context() == qemu_get_aio_context());
 
@@ -6454,16 +6457,33 @@ void bdrv_set_aio_context_ignore(BlockDriverState *bs,
             continue;
         }
         *ignore = g_slist_prepend(*ignore, child);
-        bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
+        children_to_process = g_slist_prepend(children_to_process, child);
     }
-    QLIST_FOREACH(child, &bs->parents, next_parent) {
-        if (g_slist_find(*ignore, child)) {
+
+    QLIST_FOREACH(parent, &bs->parents, next_parent) {
+        if (g_slist_find(*ignore, parent)) {
             continue;
         }
-        assert(child->klass->set_aio_ctx);
-        *ignore = g_slist_prepend(*ignore, child);
-        child->klass->set_aio_ctx(child, new_context, ignore);
+        *ignore = g_slist_prepend(*ignore, parent);
+        parents_to_process = g_slist_prepend(parents_to_process, parent);
+    }
+
+    for (entry = children_to_process;
+         entry != NULL;
+         entry = g_slist_next(entry)) {
+        child = entry->data;
+        bdrv_set_aio_context_ignore(child->bs, new_context, ignore);
+    }
+    g_slist_free(children_to_process);
+
+    for (entry = parents_to_process;
+         entry != NULL;
+         entry = g_slist_next(entry)) {
+        parent = entry->data;
+        assert(parent->klass->set_aio_ctx);
+        parent->klass->set_aio_ctx(parent, new_context, ignore);
     }
+    g_slist_free(parents_to_process);
 
     bdrv_detach_aio_context(bs);
 
-- 
2.29.2

[PULL 3/6] block: move blk_exp_close_all() to qemu_cleanup()

[Kevin Wolf]

From: Sergio Lopez <slp@redhat.com>

Move blk_exp_close_all() from bdrv_close() to qemu_cleanup(), before
bdrv_drain_all_begin().

Export drivers may have coroutines yielding at some point in the block
layer, so we need to shut them down before draining the block layer,
as otherwise they may get stuck blk_wait_while_drained().

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1900505
Signed-off-by: Sergio Lopez <slp@redhat.com>
Message-Id: <20210201125032.44713-3-slp@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                              | 1 -
 qemu-nbd.c                           | 1 +
 softmmu/runstate.c                   | 9 +++++++++
 storage-daemon/qemu-storage-daemon.c | 1 +
 4 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 5c428e1595..4e52b1c588 100644
--- a/block.c
+++ b/block.c
@@ -4435,7 +4435,6 @@ static void bdrv_close(BlockDriverState *bs)
 void bdrv_close_all(void)
 {
     assert(job_next(NULL) == NULL);
-    blk_exp_close_all();
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0d513cb38c..608c63e82a 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -503,6 +503,7 @@ static const char *socket_activation_validate_opts(const char *device,
 static void qemu_nbd_shutdown(void)
 {
     job_cancel_sync_all();
+    blk_exp_close_all();
     bdrv_close_all();
 }
 
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index beee050815..a7fcb603f7 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "audio/audio.h"
 #include "block/block.h"
+#include "block/export.h"
 #include "chardev/char.h"
 #include "crypto/cipher.h"
 #include "crypto/init.h"
@@ -784,6 +785,14 @@ void qemu_cleanup(void)
      */
     migration_shutdown();
 
+    /*
+     * Close the exports before draining the block layer. The export
+     * drivers may have coroutines yielding on it, so we need to clean
+     * them up before the drain, as otherwise they may be get stuck in
+     * blk_wait_while_drained().
+     */
+    blk_exp_close_all();
+
     /*
      * We must cancel all block jobs while the block layer is drained,
      * or cancelling will be affected by throttling and thus may block
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
index e0c87edbdd..d8d172cc60 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -314,6 +314,7 @@ int main(int argc, char *argv[])
         main_loop_wait(false);
     }
 
+    blk_exp_close_all();
     bdrv_drain_all_begin();
     bdrv_close_all();
 
-- 
2.29.2

[PULL 4/6] iotests/297: pylint: ignore too many statements

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Ignore two complains, which now lead to 297 failure on testenv.py and
testrunner.py.

Fixes: 2e5a2f57db481f18fcf70be2a36b1417370b8476
Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210129161323.615027-1-vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/pylintrc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index cd3702e23c..7a6c0a9474 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -21,6 +21,8 @@ disable=invalid-name,
         unsubscriptable-object,
         # These are temporary, and should be removed:
         missing-docstring,
+        too-many-return-statements,
+        too-many-statements
 
 [FORMAT]
 
-- 
2.29.2

[PULL 5/6] iotests: check: return 1 on failure

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

We should indicate failure by exit code, not only output.

Reported-by: Peter Maydell
Fixes: f203080bbd9f9e5b31041b1f2afcd6040c5aaec5
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210201085041.3079-1-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 4 +++-
 tests/qemu-iotests/check         | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 24b3fba115..25754e9a09 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -318,7 +318,7 @@ class TestRunner(ContextManager['TestRunner']):
 
         return res
 
-    def run_tests(self, tests: List[str]) -> None:
+    def run_tests(self, tests: List[str]) -> bool:
         n_run = 0
         failed = []
         notrun = []
@@ -363,5 +363,7 @@ class TestRunner(ContextManager['TestRunner']):
         if failed:
             print('Failures:', ' '.join(failed))
             print(f'Failed {len(failed)} of {n_run} iotests')
+            return False
         else:
             print(f'Passed all {n_run} iotests')
+            return True
diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 5190dee82e..d1c87ceaf1 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -140,4 +140,7 @@ if __name__ == '__main__':
     else:
         with TestRunner(env, makecheck=args.makecheck,
                         color=args.color) as tr:
-            tr.run_tests([os.path.join(env.source_iotests, t) for t in tests])
+            paths = [os.path.join(env.source_iotests, t) for t in tests]
+            ok = tr.run_tests(paths)
+            if not ok:
+                sys.exit(1)
-- 
2.29.2

[PULL 6/6] iotests: Fix -makecheck output

[Kevin Wolf]

For -makecheck, the old 'check' implementation skipped the output when
starting a test. It only had the condensed output at the end of a test.

testrunner.py prints the normal output when starting a test even for
-makecheck. This output contains '\r' at the end so that it can be
overwritten with the result at the end of the test. However, for
-makecheck this is shorter output in a different format, so effectively
we end up with garbled output that mixes both output forms.

Revert to the old behaviour of only printing a message after the test
had completed in -makecheck mode.

Fixes: d74c754c924ca34e90b7c96ce2f5609d82c0e628
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-Id: <20210201161024.127921-1-kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/testrunner.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 25754e9a09..1fc61fcaa3 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -301,8 +301,10 @@ class TestRunner(ContextManager['TestRunner']):
         last_el = self.last_elapsed.get(test)
         start = datetime.datetime.now().strftime('%H:%M:%S')
 
-        self.test_print_one_line(test=test, starttime=start, lasttime=last_el,
-                                 end='\r', test_field_width=test_field_width)
+        if not self.makecheck:
+            self.test_print_one_line(test=test, starttime=start,
+                                     lasttime=last_el, end='\r',
+                                     test_field_width=test_field_width)
 
         res = self.do_run_test(test)
 
-- 
2.29.2

Re: [PULL 0/6] Block layer patches

[Peter Maydell]

On Mon, 1 Feb 2021 at 19:07, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 74208cd252c5da9d867270a178799abd802b9338:
>
>   Merge remote-tracking branch 'remotes/berrange-gitlab/tags/misc-fixes-pull-request' into staging (2021-01-29 19:51:25 +0000)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 47c9af7f4c78d03986aecc9afb0aab9b19d77cb5:
>
>   iotests: Fix -makecheck output (2021-02-01 17:58:49 +0100)
>
> ----------------------------------------------------------------
> Block layer patches:
>
> - Fix double processing of nodes in bdrv_set_aio_context()
> - Fix potential hang in block export shutdown
> - iotests: Some more fixups for the 'check' rewrite
> - MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs
>
> ----------------------------------------------------------------
> Kevin Wolf (1):
>       iotests: Fix -makecheck output
>
> Sergio Lopez (2):
>       block: Avoid processing BDS twice in bdrv_set_aio_context_ignore()
>       block: move blk_exp_close_all() to qemu_cleanup()
>
> Vladimir Sementsov-Ogievskiy (3):
>       MAINTAINERS: Add Vladimir as co-maintainer for Block Jobs
>       iotests/297: pylint: ignore too many statements
>       iotests: check: return 1 on failure

Fails iotest 267 on ppc64:

  TEST   iotest-qcow2: 267 [fail]
QEMU          --
"/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-system-ppc64"
-nodefaults -display none -accel q
test
QEMU_IMG      -- "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-img"
QEMU_IO       --
"/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-io" --cache
writeback --aio threads -f qcow2
QEMU_NBD      -- "/home/pm215/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
IMGFMT        -- qcow2
IMGPROTO      -- file
PLATFORM      -- Linux/ppc64 gcc1-power7.osuosl.org 3.10.0-862.14.4.el7.ppc64
TEST_DIR      -- /home/pm215/qemu/build/all/tests/qemu-iotests/scratch
SOCK_DIR      -- /tmp/tmpvthy7ogi
SOCKET_SCM_HELPER --
/home/pm215/qemu/build/all/tests/qemu-iotests/socket_scm_helper
--- /home/pm215/qemu/tests/qemu-iotests/267.out
+++ 267.out.bad
@@ -34,7 +34,7 @@
 (qemu) info snapshots
 List of snapshots present on all disks:
 ID        TAG               VM SIZE                DATE     VM CLOCK     ICOUNT
---        snap0                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+--        snap0                 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
 (qemu) loadvm snap0
 (qemu) quit

@@ -45,7 +45,7 @@
 (qemu) info snapshots
 List of snapshots present on all disks:
 ID        TAG               VM SIZE                DATE     VM CLOCK     ICOUNT
---        snap0                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+--        snap0                 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
 (qemu) loadvm snap0
 (qemu) quit

@@ -70,7 +70,7 @@
 (qemu) info snapshots
 List of snapshots present on all disks:
 ID        TAG               VM SIZE                DATE     VM CLOCK     ICOUNT
---        snap0                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+--        snap0                 SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
 (qemu) loadvm snap0
 (qemu) quit

(snip a bunch of other diffs which are all the same thing)

Also, I can't apply this until the NetBSD "iotests 030 040 041 127 256
fail almost all the time" issue is fixed, because it makes that test
run fail rather than succeed-despite-the-failures.

thanks
-- PMM