Instead of just asserting that no requests are in flight in

bdrv_replace_node(), which is a requirement that most callers ignore, we

can just drain the source node right there. This fixes at least starting

a commit job while I/O is active on the backing chain, but probably

other callers, too.

Having requests in flight on the target node isn't a problem because the

target just gets new parents, but the call path of running requests

isn't modified. So we can just drop this assertion without a replacement.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1711643

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

Reviewed-by: Max Reitz <mreitz@redhat.com>

---

block.c | 7 ++++---

1 file changed, 4 insertions(+), 3 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_replace_node(BlockDriverState *from, BlockDriverState *to,

uint64_t perm = 0, shared = BLK_PERM_ALL;

int ret;

- assert(!atomic_read(&from->in_flight));

- assert(!atomic_read(&to->in_flight));

-

/* Make sure that @from doesn't go away until we have successfully attached

* all of its parents to @to. */

bdrv_ref(from);

+ assert(qemu_get_current_aio_context() == qemu_get_aio_context());

+ bdrv_drained_begin(from);

+

/* Put all parents into @list and calculate their cumulative permissions */

QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {

assert(c->bs == from);

@@ -XXX,XX +XXX,XX @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,

out:

g_slist_free(list);

+ bdrv_drained_end(from);

bdrv_unref(from);

}

--

2.20.1

This tests that concurrent requests are correctly drained before making

graph modifications instead of running into assertions in

bdrv_replace_node().

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

Reviewed-by: Max Reitz <mreitz@redhat.com>

---

tests/qemu-iotests/255 | 83 +++++++++++++++++++++++++++++++++++

tests/qemu-iotests/255.out | 16 +++++++

tests/qemu-iotests/group | 1 +

tests/qemu-iotests/iotests.py | 10 ++++-

4 files changed, 109 insertions(+), 1 deletion(-)

create mode 100755 tests/qemu-iotests/255

create mode 100644 tests/qemu-iotests/255.out

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/255

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

+#!/usr/bin/env python

+#

+# Test commit job graph modifications while requests are active

+#

+# Copyright (C) 2019 Red Hat, Inc.

+#

+# Creator/Owner: Kevin Wolf <kwolf@redhat.com>

+#

+# 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 iotests

+from iotests import imgfmt

+

+iotests.verify_image_format(supported_fmts=['qcow2'])

+

+def blockdev_create(vm, options):

+ result = vm.qmp_log('blockdev-create',

+ filters=[iotests.filter_qmp_testfiles],

+ job_id='job0', options=options)

+

+ if 'return' in result:

+ assert result['return'] == {}

+ vm.run_job('job0')

+ iotests.log("")

+

+with iotests.FilePath('t.qcow2') as disk_path, \

+ iotests.FilePath('t.qcow2.mid') as mid_path, \

+ iotests.FilePath('t.qcow2.base') as base_path, \

+ iotests.VM() as vm:

+

+ iotests.log("=== Create backing chain and start VM ===")

+ iotests.log("")

+

+ size = 128 * 1024 * 1024

+ size_str = str(size)

+

+ iotests.create_image(base_path, size)

+ iotests.qemu_img_log('create', '-f', iotests.imgfmt, mid_path, size_str)

+ iotests.qemu_img_log('create', '-f', iotests.imgfmt, disk_path, size_str)

+

+ # Create a backing chain like this:

+ # base <- [throttled: bps-read=4096] <- mid <- overlay

+

+ vm.add_object('throttle-group,x-bps-read=4096,id=throttle0')

+ vm.add_blockdev('file,filename=%s,node-name=base' % (base_path))

+ vm.add_blockdev('throttle,throttle-group=throttle0,file=base,node-name=throttled')

+ vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid_path))

+ vm.add_blockdev('qcow2,file=mid-file,node-name=mid,backing=throttled')

+ vm.add_drive_raw('if=none,id=overlay,driver=qcow2,file=%s,backing=mid' % (disk_path))

+

+ vm.launch()

+

+ iotests.log("=== Start background read requests ===")

+ iotests.log("")

+

+ def start_requests():

+ vm.hmp_qemu_io('overlay', 'aio_read 0 4k')

+ vm.hmp_qemu_io('overlay', 'aio_read 0 4k')

+

+ start_requests()

+

+ iotests.log("=== Run a commit job ===")

+ iotests.log("")

+

+ result = vm.qmp_log('block-commit', job_id='job0', auto_finalize=False,

+ device='overlay', top_node='mid')

+

+ vm.run_job('job0', auto_finalize=False, pre_finalize=start_requests,

+ auto_dismiss=True)

+

+ vm.shutdown()

diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/255.out

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

+=== Create backing chain and start VM ===

+

+Formatting 'TEST_DIR/PID-t.qcow2.mid', fmt=qcow2 size=134217728 cluster_size=65536 lazy_refcounts=off refcount_bits=16

+

+Formatting 'TEST_DIR/PID-t.qcow2', fmt=qcow2 size=134217728 cluster_size=65536 lazy_refcounts=off refcount_bits=16

+

+=== Start background read requests ===

+

+=== Run a commit job ===

+

+{"execute": "block-commit", "arguments": {"auto-finalize": false, "device": "overlay", "job-id": "job0", "top-node": "mid"}}

+{"return": {}}

+{"execute": "job-finalize", "arguments": {"id": "job0"}}

+{"return": {}}

+{"data": {"id": "job0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+{"data": {"device": "job0", "len": 134217728, "offset": 134217728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/group

+++ b/tests/qemu-iotests/group

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

252 rw auto backing quick

253 rw auto quick

254 rw auto backing quick

+255 rw auto quick

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ def qemu_img_pipe(*args):

sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))

return subp.communicate()[0]

+def qemu_img_log(*args):

+ result = qemu_img_pipe(*args)

+ log(result, filters=[filter_testfiles])

+ return result

+

def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):

args = [ 'info' ]

if imgopts:

@@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine):

return result

# Returns None on success, and an error string on failure

- def run_job(self, job, auto_finalize=True, auto_dismiss=False):

+ def run_job(self, job, auto_finalize=True, auto_dismiss=False,

+ pre_finalize=None):

error = None

while True:

for ev in self.get_qmp_events_filtered(wait=True):

@@ -XXX,XX +XXX,XX @@ class VM(qtest.QEMUQtestMachine):

error = j['error']

log('Job failed: %s' % (j['error']))

elif status == 'pending' and not auto_finalize:

+ if pre_finalize:

+ pre_finalize()

self.qmp_log('job-finalize', id=job)

elif status == 'concluded' and not auto_dismiss:

self.qmp_log('job-dismiss', id=job)

--

2.20.1

From: John Snow <jsnow@redhat.com>

If the bitmap can't be used for whatever reason, we skip putting down

the reference. Fix that.

In practice, this means that if you attempt to gracefully exit QEMU

after a backup command being rejected, bdrv_close_all will fail and

tell you some unpleasant things via assert().

Reported-by: aihua liang <aliang@redhat.com>

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1703916

Signed-off-by: John Snow <jsnow@redhat.com>

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

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

---

blockdev.c | 13 ++++++-------

1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c

index XXXXXXX..XXXXXXX 100644

--- a/blockdev.c

+++ b/blockdev.c

@@ -XXX,XX +XXX,XX @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,

if (set_backing_hd) {

bdrv_set_backing_hd(target_bs, source, &local_err);

if (local_err) {

- bdrv_unref(target_bs);

- goto out;

+ goto unref;

}

}

@@ -XXX,XX +XXX,XX @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,

bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);

if (!bmap) {

error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);

- bdrv_unref(target_bs);

- goto out;

+ goto unref;

}

if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_DEFAULT, errp)) {

- goto out;

+ goto unref;

}

}

if (!backup->auto_finalize) {

@@ -XXX,XX +XXX,XX @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,

backup->sync, bmap, backup->compress,

backup->on_source_error, backup->on_target_error,

job_flags, NULL, NULL, txn, &local_err);

- bdrv_unref(target_bs);

if (local_err != NULL) {

error_propagate(errp, local_err);

- goto out;

+ goto unref;

}

+unref:

+ bdrv_unref(target_bs);

out:

aio_context_release(aio_context);

return job;

--

2.20.1

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

Block layer may recursively check block_status in file child of qcow2,

if qcow2 driver returned DATA. There are several test cases to check

influence of lseek on block_status performance. To see real difference

run on tmpfs.

Tests originally created by Kevin, I just refactored and put them

together into one executable file with simple output.

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

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

---

tests/perf/block/qcow2/convert-blockstatus | 71 ++++++++++++++++++++++

1 file changed, 71 insertions(+)

create mode 100755 tests/perf/block/qcow2/convert-blockstatus

diff --git a/tests/perf/block/qcow2/convert-blockstatus b/tests/perf/block/qcow2/convert-blockstatus

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/perf/block/qcow2/convert-blockstatus

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

+#!/bin/bash

+#

+# Test lseek influence on qcow2 block-status

+#

+# Block layer may recursively check block_status in file child of qcow2, if

+# qcow2 driver returned DATA. There are several test cases to check influence

+# of lseek on block_status performance. To see real difference run on tmpfs.

+#

+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.

+#

+# Tests originally written by Kevin Wolf

+#

+# 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/>.

+#

+

+if [ "$#" -lt 1 ]; then

+ echo "Usage: $0 SOURCE_FILE"

+ exit 1

+fi

+

+ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"

+QEMU_IMG="$ROOT_DIR/qemu-img"

+QEMU_IO="$ROOT_DIR/qemu-io"

+

+size=1G

+src="$1"

+

+# test-case plain

+

+(

+$QEMU_IMG create -f qcow2 "$src" $size

+for i in $(seq 16384 -1 0); do

+ echo "write $((i * 65536)) 64k"

+done | $QEMU_IO "$src"

+) > /dev/null

+

+echo -n "plain: "

+/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://

+

+# test-case forward

+

+(

+$QEMU_IMG create -f qcow2 "$src" $size

+for i in $(seq 0 2 16384); do

+ echo "write $((i * 65536)) 64k"

+done | $QEMU_IO "$src"

+for i in $(seq 1 2 16384); do

+ echo "write $((i * 65536)) 64k"

+done | $QEMU_IO "$src"

+) > /dev/null

+

+echo -n "forward: "

+/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://

+

+# test-case prealloc

+

+$QEMU_IMG create -f qcow2 -o preallocation=metadata "$src" $size > /dev/null

+

+echo -n "prealloc: "

+/usr/bin/time -f %e $QEMU_IMG convert -n "$src" null-co://

--

2.20.1

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

drv_co_block_status digs bs->file for additional, more accurate search

for hole inside region, reported as DATA by bs since 5daa74a6ebc.

This accuracy is not free: assume we have qcow2 disk. Actually, qcow2

knows, where are holes and where is data. But every block_status

request calls lseek additionally. Assume a big disk, full of

data, in any iterative copying block job (or img convert) we'll call

lseek(HOLE) on every iteration, and each of these lseeks will have to

iterate through all metadata up to the end of file. It's obviously

ineffective behavior. And for many scenarios we don't need this lseek

at all.

However, lseek is needed when we have metadata-preallocated image.

So, let's detect metadata-preallocation case and don't dig qcow2's

protocol file in other cases.

The idea is to compare allocation size in POV of filesystem with

allocations size in POV of Qcow2 (by refcounts). If allocation in fs is

significantly lower, consider it as metadata-preallocation case.

102 iotest changed, as our detector can't detect shrinked file as

metadata-preallocation, which don't seem to be wrong, as with metadata

preallocation we always have valid file length.

Two other iotests have a slight change in their QMP output sequence:

Active 'block-commit' returns earlier because the job coroutine yields

earlier on a blocking operation. This operation is loading the refcount

blocks in qcow2_detect_metadata_preallocation().

Suggested-by: Denis V. Lunev <den@openvz.org>

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

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

---

block/qcow2.h | 4 ++++

include/block/block.h | 8 +++++++-

block/io.c | 9 ++++++++-

block/qcow2-refcount.c | 32 ++++++++++++++++++++++++++++++++

block/qcow2.c | 11 +++++++++++

tests/qemu-iotests/102 | 2 +-

tests/qemu-iotests/102.out | 3 ++-

tests/qemu-iotests/141.out | 2 +-

tests/qemu-iotests/144.out | 2 +-

9 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2.h

+++ b/block/qcow2.h

@@ -XXX,XX +XXX,XX @@ typedef struct BDRVQcow2State {

int nb_threads;

BdrvChild *data_file;

+

+ bool metadata_preallocation_checked;

+ bool metadata_preallocation;

} BDRVQcow2State;

typedef struct Qcow2COWRegion {

@@ -XXX,XX +XXX,XX @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,

void *cb_opaque, Error **errp);

int qcow2_shrink_reftable(BlockDriverState *bs);

int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);

+int qcow2_detect_metadata_preallocation(BlockDriverState *bs);

/* qcow2-cluster.c functions */

int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,

diff --git a/include/block/block.h b/include/block/block.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block.h

+++ b/include/block/block.h

@@ -XXX,XX +XXX,XX @@ typedef struct HDGeometry {

* BDRV_BLOCK_EOF: the returned pnum covers through end of file for this

* layer, set by block layer

*

- * Internal flag:

+ * Internal flags:

* BDRV_BLOCK_RAW: for use by passthrough drivers, such as raw, to request

* that the block layer recompute the answer from the returned

* BDS; must be accompanied by just BDRV_BLOCK_OFFSET_VALID.

+ * BDRV_BLOCK_RECURSE: request that the block layer will recursively search for

+ * zeroes in file child of current block node inside

+ * returned region. Only valid together with both

+ * BDRV_BLOCK_DATA and BDRV_BLOCK_OFFSET_VALID. Should not

+ * appear with BDRV_BLOCK_ZERO.

*

* If BDRV_BLOCK_OFFSET_VALID is set, the map parameter represents the

* host offset within the returned BDS that is allocated for the

@@ -XXX,XX +XXX,XX @@ typedef struct HDGeometry {

#define BDRV_BLOCK_RAW 0x08

#define BDRV_BLOCK_ALLOCATED 0x10

#define BDRV_BLOCK_EOF 0x20

+#define BDRV_BLOCK_RECURSE 0x40

#define BDRV_BLOCK_OFFSET_MASK BDRV_SECTOR_MASK

typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;

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

index XXXXXXX..XXXXXXX 100644

--- a/block/io.c

+++ b/block/io.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,

*/

assert(*pnum && QEMU_IS_ALIGNED(*pnum, align) &&

align > offset - aligned_offset);

+ if (ret & BDRV_BLOCK_RECURSE) {

+ assert(ret & BDRV_BLOCK_DATA);

+ assert(ret & BDRV_BLOCK_OFFSET_VALID);

+ assert(!(ret & BDRV_BLOCK_ZERO));

+ }

+

*pnum -= offset - aligned_offset;

if (*pnum > bytes) {

*pnum = bytes;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_block_status(BlockDriverState *bs,

}

}

- if (want_zero && local_file && local_file != bs &&

+ if (want_zero && ret & BDRV_BLOCK_RECURSE &&

+ local_file && local_file != bs &&

(ret & BDRV_BLOCK_DATA) && !(ret & BDRV_BLOCK_ZERO) &&

(ret & BDRV_BLOCK_OFFSET_VALID)) {

int64_t file_pnum;

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2-refcount.c

+++ b/block/qcow2-refcount.c

@@ -XXX,XX +XXX,XX @@ int64_t qcow2_get_last_cluster(BlockDriverState *bs, int64_t size)

"There are no references in the refcount table.");

return -EIO;

}

+

+int qcow2_detect_metadata_preallocation(BlockDriverState *bs)

+{

+ BDRVQcow2State *s = bs->opaque;

+ int64_t i, end_cluster, cluster_count = 0, threshold;

+ int64_t file_length, real_allocation, real_clusters;

+

+ file_length = bdrv_getlength(bs->file->bs);

+ if (file_length < 0) {

+ return file_length;

+ }

+

+ real_allocation = bdrv_get_allocated_file_size(bs->file->bs);

+ if (real_allocation < 0) {

+ return real_allocation;

+ }

+

+ real_clusters = real_allocation / s->cluster_size;

+ threshold = MAX(real_clusters * 10 / 9, real_clusters + 2);

+

+ end_cluster = size_to_clusters(s, file_length);

+ for (i = 0; i < end_cluster && cluster_count < threshold; i++) {

+ uint64_t refcount;

+ int ret = qcow2_get_refcount(bs, i, &refcount);

+ if (ret < 0) {

+ return ret;

+ }

+ cluster_count += !!refcount;

+ }

+

+ return cluster_count >= threshold;

+}

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

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,

unsigned int bytes;

int status = 0;

+ if (!s->metadata_preallocation_checked) {

+ ret = qcow2_detect_metadata_preallocation(bs);

+ s->metadata_preallocation = (ret == 1);

+ s->metadata_preallocation_checked = true;

+ }

+

bytes = MIN(INT_MAX, count);

qemu_co_mutex_lock(&s->lock);

ret = qcow2_get_cluster_offset(bs, offset, &bytes, &cluster_offset);

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,

} else if (ret != QCOW2_CLUSTER_UNALLOCATED) {

status |= BDRV_BLOCK_DATA;

}

+ if (s->metadata_preallocation && (status & BDRV_BLOCK_DATA) &&

+ (status & BDRV_BLOCK_OFFSET_VALID))

+ {

+ status |= BDRV_BLOCK_RECURSE;

+ }

return status;

}

diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/102

+++ b/tests/qemu-iotests/102

@@ -XXX,XX +XXX,XX @@ $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io

$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024))

$QEMU_IO -c map "$TEST_IMG"

-$QEMU_IMG map "$TEST_IMG"

+$QEMU_IMG map "$TEST_IMG" | _filter_qemu_img_map

echo

echo '=== Testing map on an image file truncated outside of qemu ==='

diff --git a/tests/qemu-iotests/102.out b/tests/qemu-iotests/102.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/102.out

+++ b/tests/qemu-iotests/102.out

@@ -XXX,XX +XXX,XX @@ wrote 65536/65536 bytes at offset 0

64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

Image resized.

64 KiB (0x10000) bytes allocated at offset 0 bytes (0x0)

-Offset Length Mapped to File

+Offset Length File

+0 0x10000 TEST_DIR/t.IMGFMT

=== Testing map on an image file truncated outside of qemu ===

diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/141.out

+++ b/tests/qemu-iotests/141.out

@@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.

{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}

+{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "job0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}

-{"return": {}}

{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}

{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}

diff --git a/tests/qemu-iotests/144.out b/tests/qemu-iotests/144.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/144.out

+++ b/tests/qemu-iotests/144.out

@@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/tmp.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "virtio0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "virtio0"}}

+{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "virtio0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}

{"return": {}}

-{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "virtio0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "virtio0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}

--

2.20.1

From: Max Reitz <mreitz@redhat.com>

When ending a drained section, bdrv_do_drained_end() currently first

decrements the quiesce_counter, and only then actually ends the drain.

The bdrv_drain_invoke(bs, false) call may cause graph changes. Say the

graph change involves replacing an existing BB's ("blk") BDS

(blk_bs(blk)) by @bs. Let us introducing the following values:

- bs_oqc = old_quiesce_counter

(so bs->quiesce_counter == bs_oqc - 1)

- obs_qc = blk_bs(blk)->quiesce_counter (before bdrv_drain_invoke())

Let us assume there is no blk_pread_unthrottled() involved, so

blk->quiesce_counter == obs_qc (before bdrv_drain_invoke()).

Now replacing blk_bs(blk) by @bs will reduce blk->quiesce_counter by

obs_qc (making it 0) and increase it by bs_oqc-1 (making it bs_oqc-1).

bdrv_drain_invoke() returns and we invoke bdrv_parent_drained_end().

This will decrement blk->quiesce_counter by one, so it would be -1 --

were there not an assertion against that in blk_root_drained_end().

We therefore have to keep the quiesce_counter up at least until

bdrv_drain_invoke() returns, so that bdrv_parent_drained_end() does the

right thing for the parents @bs got during bdrv_drain_invoke().

But let us delay it even further, namely until bdrv_parent_drained_end()

returns, because then it mirrors bdrv_do_drained_begin(): There, we

first increment the quiesce_counter, then begin draining the parents,

and then call bdrv_drain_invoke(). It makes sense to let

bdrv_do_drained_end() unravel this exactly in reverse.

Signed-off-by: Max Reitz <mreitz@redhat.com>

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

---

block/io.c | 3 ++-

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

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

index XXXXXXX..XXXXXXX 100644

--- a/block/io.c

+++ b/block/io.c

@@ -XXX,XX +XXX,XX @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,

return;

}

assert(bs->quiesce_counter > 0);

- old_quiesce_counter = atomic_fetch_dec(&bs->quiesce_counter);

/* Re-enable things in child-to-parent order */

bdrv_drain_invoke(bs, false);

bdrv_parent_drained_end(bs, parent, ignore_bds_parents);

+

+ old_quiesce_counter = atomic_fetch_dec(&bs->quiesce_counter);

if (old_quiesce_counter == 1) {

aio_enable_external(bdrv_get_aio_context(bs));

}

--

2.20.1

From: Max Reitz <mreitz@redhat.com>

This patch adds a test where we cancel a throttled mirror job and

immediately close the VM before it can be cancelled. Doing so will

invoke bdrv_drain_all() while the mirror job tries to drain the

throttled node. When bdrv_drain_all_end() tries to lift its drain on

the throttle node, the job will exit and replace the current root node

of the BB drive0 (which is the job's filter node) by the throttle node.

Before the previous patch, this replacement did not increase drive0's

quiesce_counter by a sufficient amount, so when

bdrv_parent_drained_end() (invoked by bdrv_do_drained_end(), invoked by

bdrv_drain_all_end()) tried to end the drain on all of the throttle

node's parents, it decreased drive0's quiesce_counter below 0 -- which

fails an assertion.

Signed-off-by: Max Reitz <mreitz@redhat.com>

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

---

tests/qemu-iotests/255 | 52 ++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/255.out | 24 ++++++++++++++++++

2 files changed, 76 insertions(+)

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/255

+++ b/tests/qemu-iotests/255

@@ -XXX,XX +XXX,XX @@ def blockdev_create(vm, options):

vm.run_job('job0')

iotests.log("")

+iotests.log('Finishing a commit job with background reads')

+iotests.log('============================================')

+iotests.log('')

+

with iotests.FilePath('t.qcow2') as disk_path, \

iotests.FilePath('t.qcow2.mid') as mid_path, \

iotests.FilePath('t.qcow2.base') as base_path, \

@@ -XXX,XX +XXX,XX @@ with iotests.FilePath('t.qcow2') as disk_path, \

auto_dismiss=True)

vm.shutdown()

+

+iotests.log('')

+iotests.log('Closing the VM while a job is being cancelled')

+iotests.log('=============================================')

+iotests.log('')

+

+with iotests.FilePath('src.qcow2') as src_path, \

+ iotests.FilePath('dst.qcow2') as dst_path, \

+ iotests.VM() as vm:

+

+ iotests.log('=== Create images and start VM ===')

+ iotests.log('')

+

+ size = 128 * 1024 * 1024

+ size_str = str(size)

+

+ iotests.qemu_img_log('create', '-f', iotests.imgfmt, src_path, size_str)

+ iotests.qemu_img_log('create', '-f', iotests.imgfmt, dst_path, size_str)

+

+ iotests.log(iotests.qemu_io('-f', iotests.imgfmt, '-c', 'write 0 1M',

+ src_path),

+ filters=[iotests.filter_test_dir, iotests.filter_qemu_io])

+

+ vm.add_object('throttle-group,x-bps-read=4096,id=throttle0')

+

+ vm.add_blockdev('file,node-name=src-file,filename=%s' % (src_path))

+ vm.add_blockdev('%s,node-name=src,file=src-file' % (iotests.imgfmt))

+

+ vm.add_blockdev('file,node-name=dst-file,filename=%s' % (dst_path))

+ vm.add_blockdev('%s,node-name=dst,file=dst-file' % (iotests.imgfmt))

+

+ vm.add_blockdev('throttle,node-name=src-throttled,' +

+ 'throttle-group=throttle0,file=src')

+

+ vm.add_device('virtio-blk,drive=src-throttled')

+

+ vm.launch()

+

+ iotests.log('=== Start a mirror job ===')

+ iotests.log('')

+

+ vm.qmp_log('blockdev-mirror', job_id='job0', device='src-throttled',

+ target='dst', sync='full')

+

+ vm.qmp_log('block-job-cancel', device='job0')

+ vm.qmp_log('quit')

+

+ vm.shutdown()

diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/255.out

+++ b/tests/qemu-iotests/255.out

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

+Finishing a commit job with background reads

+============================================

+

=== Create backing chain and start VM ===

Formatting 'TEST_DIR/PID-t.qcow2.mid', fmt=qcow2 size=134217728 cluster_size=65536 lazy_refcounts=off refcount_bits=16

@@ -XXX,XX +XXX,XX @@ Formatting 'TEST_DIR/PID-t.qcow2', fmt=qcow2 size=134217728 cluster_size=65536 l

{"return": {}}

{"data": {"id": "job0", "type": "commit"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

{"data": {"device": "job0", "len": 134217728, "offset": 134217728, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

+

+Closing the VM while a job is being cancelled

+=============================================

+

+=== Create images and start VM ===

+

+Formatting 'TEST_DIR/PID-src.qcow2', fmt=qcow2 size=134217728 cluster_size=65536 lazy_refcounts=off refcount_bits=16

+

+Formatting 'TEST_DIR/PID-dst.qcow2', fmt=qcow2 size=134217728 cluster_size=65536 lazy_refcounts=off refcount_bits=16

+

+wrote 1048576/1048576 bytes at offset 0

+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+

+=== Start a mirror job ===

+

+{"execute": "blockdev-mirror", "arguments": {"device": "src-throttled", "job-id": "job0", "sync": "full", "target": "dst"}}

+{"return": {}}

+{"execute": "block-job-cancel", "arguments": {"device": "job0"}}

+{"return": {}}

+{"execute": "quit", "arguments": {}}

+{"return": {}}

--

2.20.1

From: Julia Suvorova <jusual@mail.ru>

Callback-based laio_submit() and laio_cancel() were left after

rewriting Linux AIO backend to coroutines in hope that they would be

used in other code that could bypass coroutines. They can be safely

removed because they have not been used since that time.

Signed-off-by: Julia Suvorova <jusual@mail.ru>

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

---

include/block/raw-aio.h | 3 --

block/linux-aio.c | 72 ++++++-----------------------------------

2 files changed, 10 insertions(+), 65 deletions(-)

diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/raw-aio.h

+++ b/include/block/raw-aio.h

@@ -XXX,XX +XXX,XX @@ LinuxAioState *laio_init(Error **errp);

void laio_cleanup(LinuxAioState *s);

int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,

uint64_t offset, QEMUIOVector *qiov, int type);

-BlockAIOCB *laio_submit(BlockDriverState *bs, LinuxAioState *s, int fd,

- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,

- BlockCompletionFunc *cb, void *opaque, int type);

void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context);

void laio_attach_aio_context(LinuxAioState *s, AioContext *new_context);

void laio_io_plug(BlockDriverState *bs, LinuxAioState *s);

diff --git a/block/linux-aio.c b/block/linux-aio.c

index XXXXXXX..XXXXXXX 100644

--- a/block/linux-aio.c

+++ b/block/linux-aio.c

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

#define MAX_EVENTS 128

struct qemu_laiocb {

- BlockAIOCB common;

Coroutine *co;

LinuxAioState *ctx;

struct iocb iocb;

@@ -XXX,XX +XXX,XX @@ static inline ssize_t io_event_ret(struct io_event *ev)

}

/*

- * Completes an AIO request (calls the callback and frees the ACB).

+ * Completes an AIO request.

*/

static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)

{

@@ -XXX,XX +XXX,XX @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)

}

laiocb->ret = ret;

- if (laiocb->co) {

- /* If the coroutine is already entered it must be in ioq_submit() and

- * will notice laio->ret has been filled in when it eventually runs

- * later. Coroutines cannot be entered recursively so avoid doing

- * that!

- */

- if (!qemu_coroutine_entered(laiocb->co)) {

- aio_co_wake(laiocb->co);

- }

- } else {

- laiocb->common.cb(laiocb->common.opaque, ret);

- qemu_aio_unref(laiocb);

+

+ /*

+ * If the coroutine is already entered it must be in ioq_submit() and

+ * will notice laio->ret has been filled in when it eventually runs

+ * later. Coroutines cannot be entered recursively so avoid doing

+ * that!

+ */

+ if (!qemu_coroutine_entered(laiocb->co)) {

+ aio_co_wake(laiocb->co);

}

}

@@ -XXX,XX +XXX,XX @@ static bool qemu_laio_poll_cb(void *opaque)

return true;

}

-static void laio_cancel(BlockAIOCB *blockacb)

-{

- struct qemu_laiocb *laiocb = (struct qemu_laiocb *)blockacb;

- struct io_event event;

- int ret;

-

- if (laiocb->ret != -EINPROGRESS) {

- return;

- }

- ret = io_cancel(laiocb->ctx->ctx, &laiocb->iocb, &event);

- laiocb->ret = -ECANCELED;

- if (ret != 0) {

- /* iocb is not cancelled, cb will be called by the event loop later */

- return;

- }

-

- laiocb->common.cb(laiocb->common.opaque, laiocb->ret);

-}

-

-static const AIOCBInfo laio_aiocb_info = {

- .aiocb_size = sizeof(struct qemu_laiocb),

- .cancel_async = laio_cancel,

-};

-

static void ioq_init(LaioQueue *io_q)

{

QSIMPLEQ_INIT(&io_q->pending);

@@ -XXX,XX +XXX,XX @@ int coroutine_fn laio_co_submit(BlockDriverState *bs, LinuxAioState *s, int fd,

return laiocb.ret;

}

-BlockAIOCB *laio_submit(BlockDriverState *bs, LinuxAioState *s, int fd,

- int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,

- BlockCompletionFunc *cb, void *opaque, int type)

-{

- struct qemu_laiocb *laiocb;

- off_t offset = sector_num * BDRV_SECTOR_SIZE;

- int ret;

-

- laiocb = qemu_aio_get(&laio_aiocb_info, bs, cb, opaque);

- laiocb->nbytes = nb_sectors * BDRV_SECTOR_SIZE;

- laiocb->ctx = s;

- laiocb->ret = -EINPROGRESS;

- laiocb->is_read = (type == QEMU_AIO_READ);

- laiocb->qiov = qiov;

-

- ret = laio_do_submit(fd, laiocb, offset, type);

- if (ret < 0) {

- qemu_aio_unref(laiocb);

- return NULL;

- }

-

- return &laiocb->common;

-}

-

void laio_detach_aio_context(LinuxAioState *s, AioContext *old_context)

{

aio_set_event_notifier(old_context, &s->e, false, NULL, NULL);

--

2.20.1

From: Kenneth Heitke <kenneth.heitke@intel.com>

Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>

Reviewed-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>

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

---

hw/block/nvme.h | 2 +

include/block/nvme.h | 2 +

hw/block/nvme.c | 106 +++++++++++++++++++++++++++++++++++++++++-

hw/block/trace-events | 2 +

4 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.h b/hw/block/nvme.h

index XXXXXXX..XXXXXXX 100644

--- a/hw/block/nvme.h

+++ b/hw/block/nvme.h

@@ -XXX,XX +XXX,XX @@ typedef struct NvmeCtrl {

uint32_t cmbloc;

uint8_t *cmbuf;

uint64_t irq_status;

+ uint64_t host_timestamp; /* Timestamp sent by the host */

+ uint64_t timestamp_set_qemu_clock_ms; /* QEMU clock time */

char *serial;

NvmeNamespace *namespaces;

diff --git a/include/block/nvme.h b/include/block/nvme.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/nvme.h

+++ b/include/block/nvme.h

@@ -XXX,XX +XXX,XX @@ enum NvmeIdCtrlOncs {

NVME_ONCS_WRITE_ZEROS = 1 << 3,

NVME_ONCS_FEATURES = 1 << 4,

NVME_ONCS_RESRVATIONS = 1 << 5,

+ NVME_ONCS_TIMESTAMP = 1 << 6,

};

#define NVME_CTRL_SQES_MIN(sqes) ((sqes) & 0xf)

@@ -XXX,XX +XXX,XX @@ enum NvmeFeatureIds {

NVME_INTERRUPT_VECTOR_CONF = 0x9,

NVME_WRITE_ATOMICITY = 0xa,

NVME_ASYNCHRONOUS_EVENT_CONF = 0xb,

+ NVME_TIMESTAMP = 0xe,

NVME_SOFTWARE_PROGRESS_MARKER = 0x80

};

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

index XXXXXXX..XXXXXXX 100644

--- a/hw/block/nvme.c

+++ b/hw/block/nvme.c

@@ -XXX,XX +XXX,XX @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,

return NVME_INVALID_FIELD | NVME_DNR;

}

+static uint16_t nvme_dma_write_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,

+ uint64_t prp1, uint64_t prp2)

+{

+ QEMUSGList qsg;

+ QEMUIOVector iov;

+ uint16_t status = NVME_SUCCESS;

+

+ if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {

+ return NVME_INVALID_FIELD | NVME_DNR;

+ }

+ if (qsg.nsg > 0) {

+ if (dma_buf_write(ptr, len, &qsg)) {

+ status = NVME_INVALID_FIELD | NVME_DNR;

+ }

+ qemu_sglist_destroy(&qsg);

+ } else {

+ if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {

+ status = NVME_INVALID_FIELD | NVME_DNR;

+ }

+ qemu_iovec_destroy(&iov);

+ }

+ return status;

+}

+

static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,

uint64_t prp1, uint64_t prp2)

{

@@ -XXX,XX +XXX,XX @@ static uint16_t nvme_identify_nslist(NvmeCtrl *n, NvmeIdentify *c)

return ret;

}

-

static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)

{

NvmeIdentify *c = (NvmeIdentify *)cmd;

@@ -XXX,XX +XXX,XX @@ static uint16_t nvme_identify(NvmeCtrl *n, NvmeCmd *cmd)

}

}

+static inline void nvme_set_timestamp(NvmeCtrl *n, uint64_t ts)

+{

+ trace_nvme_setfeat_timestamp(ts);

+

+ n->host_timestamp = le64_to_cpu(ts);

+ n->timestamp_set_qemu_clock_ms = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);

+}

+

+static inline uint64_t nvme_get_timestamp(const NvmeCtrl *n)

+{

+ uint64_t current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);

+ uint64_t elapsed_time = current_time - n->timestamp_set_qemu_clock_ms;

+

+ union nvme_timestamp {

+ struct {

+ uint64_t timestamp:48;

+ uint64_t sync:1;

+ uint64_t origin:3;

+ uint64_t rsvd1:12;

+ };

+ uint64_t all;

+ };

+

+ union nvme_timestamp ts;

+ ts.all = 0;