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 XXXXXXX..XXXXXXX 100644

--- a/MAINTAINERS

+++ b/MAINTAINERS

@@ -XXX,XX +XXX,XX @@ 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

@@ -XXX,XX +XXX,XX @@ 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

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 XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ 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());

@@ -XXX,XX +XXX,XX @@ 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

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 XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/qemu-nbd.c

+++ b/qemu-nbd.c

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/softmmu/runstate.c

+++ b/softmmu/runstate.c

@@ -XXX,XX +XXX,XX @@

#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"

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644

--- a/storage-daemon/qemu-storage-daemon.c

+++ b/storage-daemon/qemu-storage-daemon.c

@@ -XXX,XX +XXX,XX @@ int main(int argc, char *argv[])

main_loop_wait(false);

}

+ blk_exp_close_all();

bdrv_drain_all_begin();

bdrv_close_all();

--

2.29.2

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 XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/pylintrc

+++ b/tests/qemu-iotests/pylintrc

@@ -XXX,XX +XXX,XX @@ 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

If the qemu-system-{arch} binary for the host architecture can't be

found, the old 'check' implementation selected the alphabetically first

system emulator binary that it could find. The new Python implementation

just uses the first result of glob.iglob(), which has an undefined

order.

This is a problem that breaks CI because the iotests aren't actually

prepared to run on any emulator. They should be, so this is really a bug

in the failing test cases that should be fixed there, but as a quick

fix, let's revert to the old behaviour to let CI runs succeed again.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20210202142802.119999-1-kwolf@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/testenv.py | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/testenv.py

+++ b/tests/qemu-iotests/testenv.py

@@ -XXX,XX +XXX,XX @@ class TestEnv(ContextManager['TestEnv']):

if not os.path.exists(self.qemu_prog):

pattern = root('qemu-system-*')

try:

- progs = glob.iglob(pattern)

+ progs = sorted(glob.iglob(pattern))

self.qemu_prog = next(p for p in progs if isxfile(p))

except StopIteration:

sys.exit("Not found any Qemu executable binary by pattern "

--

2.29.2

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 XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/testrunner.py

+++ b/tests/qemu-iotests/testrunner.py

@@ -XXX,XX +XXX,XX @@ 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 = []

@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/check

+++ b/tests/qemu-iotests/check

@@ -XXX,XX +XXX,XX @@ 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

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 XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/testrunner.py

+++ b/tests/qemu-iotests/testrunner.py

@@ -XXX,XX +XXX,XX @@ 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

From: Philippe Mathieu-Daudé <philmd@redhat.com>

Commit 15b2260bef3 ("block/nvme: Trace controller capabilities")

misunderstood the doorbell stride value from the datasheet, use

the correct one. The 'doorbell_scale' variable used few lines

later is correct.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-Id: <20210127212137.3482291-2-philmd@redhat.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/nvme.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c

index XXXXXXX..XXXXXXX 100644

--- a/block/nvme.c

+++ b/block/nvme.c

@@ -XXX,XX +XXX,XX @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,

trace_nvme_controller_capability("Contiguous Queues Required",

NVME_CAP_CQR(cap));

trace_nvme_controller_capability("Doorbell Stride",

- 2 << (2 + NVME_CAP_DSTRD(cap)));

+ 1 << (2 + NVME_CAP_DSTRD(cap)));

trace_nvme_controller_capability("Subsystem Reset Supported",

NVME_CAP_NSSRS(cap));

trace_nvme_controller_capability("Memory Page Size Minimum",

--

2.29.2

From: Philippe Mathieu-Daudé <philmd@redhat.com>

NVMe controllers implement different versions of the spec,

and different features of it. It is useful to gather this

information when debugging.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-Id: <20210127212137.3482291-3-philmd@redhat.com>

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/nvme.c | 6 ++++++

block/trace-events | 1 +

2 files changed, 7 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c

index XXXXXXX..XXXXXXX 100644

--- a/block/nvme.c

+++ b/block/nvme.c

@@ -XXX,XX +XXX,XX @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,

AioContext *aio_context = bdrv_get_aio_context(bs);

int ret;

uint64_t cap;

+ uint32_t ver;

uint64_t timeout_ms;

uint64_t deadline, now;

volatile NvmeBar *regs = NULL;

@@ -XXX,XX +XXX,XX @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,

bs->bl.request_alignment = s->page_size;

timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);

+ ver = le32_to_cpu(regs->vs);

+ trace_nvme_controller_spec_version(extract32(ver, 16, 16),

+ extract32(ver, 8, 8),

+ extract32(ver, 0, 8));

+

/* Reset device to get a clean state. */

regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);

/* Wait for CSTS.RDY = 0. */

diff --git a/block/trace-events b/block/trace-events

index XXXXXXX..XXXXXXX 100644

--- a/block/trace-events

+++ b/block/trace-events

@@ -XXX,XX +XXX,XX @@ qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s

# nvme.c

nvme_controller_capability_raw(uint64_t value) "0x%08"PRIx64

nvme_controller_capability(const char *desc, uint64_t value) "%s: %"PRIu64

+nvme_controller_spec_version(uint32_t mjr, uint32_t mnr, uint32_t ter) "Specification supported: %u.%u.%u"

nvme_kick(void *s, unsigned q_index) "s %p q #%u"

nvme_dma_flush_queue_wait(void *s) "s %p"

nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"

--

2.29.2

size_to_str() can return a size like "4.24 MiB", with a single digit

integer part and two fractional digits. This is eight characters, but

commit b39847a5 changed the format string to only reserve seven

characters for the column.

This can result in unaligned columns, which in turn changes the output of

iotests case 267 because exceeding the column size defeats the attempt

to filter the size out of the output (observed with the ppc64 emulator).

The resulting change is only a whitespace change, but since commit

f203080b this is enough for iotests to consider the test failed.

Taking a character away from the tag name column and adding it to the VM

size column doesn't change anything in the common case (the tag name is

left justified, the VM size is right justified), but fixes this case.

Fixes: b39847a50553b7679d6d7fefbe6a108a17aacf8d

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20210202155911.179865-1-kwolf@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/qapi.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qapi.c

+++ b/block/qapi.c

@@ -XXX,XX +XXX,XX @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn)

char *sizing = NULL;

if (!sn) {

- qemu_printf("%-10s%-18s%7s%20s%13s%11s",

+ qemu_printf("%-10s%-17s%8s%20s%13s%11s",

"ID", "TAG", "VM SIZE", "DATE", "VM CLOCK", "ICOUNT");

} else {

ti = sn->date_sec;

@@ -XXX,XX +XXX,XX @@ void bdrv_snapshot_dump(QEMUSnapshotInfo *sn)

snprintf(icount_buf, sizeof(icount_buf),

"%"PRId64, sn->icount);

}

- qemu_printf("%-9s %-17s %7s%20s%13s%11s",

+ qemu_printf("%-9s %-16s %8s%20s%13s%11s",

sn->id_str, sn->name,

sizing,

date_buf,

--

2.29.2