From: Max Reitz <mreitz@redhat.com>

gluster's block-status implementation is basically a copy of that in

block/file-posix.c, there is only one thing missing, and that is

aligning trailing data extents to the request alignment (as added by

commit 9c3db310ff0).

Note that 9c3db310ff0 mentions that "there seems to be no other block

driver that sets request_alignment and [...]", but while block/gluster.c

does indeed not set request_alignment, block/io.c's

bdrv_refresh_limits() will still default to an alignment of 512 because

block/gluster.c does not provide a byte-aligned read function.

Therefore, unaligned tails can conceivably occur, and so we should apply

the change from 9c3db310ff0 to gluster's block-status implementation.

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

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

Message-Id: <20210805143603.59503-1-mreitz@redhat.com>

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

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block/gluster.c | 16 ++++++++++++++++

1 file changed, 16 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/gluster.c

+++ b/block/gluster.c

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

off_t data = 0, hole = 0;

int ret = -EINVAL;

+ assert(QEMU_IS_ALIGNED(offset | bytes, bs->bl.request_alignment));

+

if (!s->fd) {

return ret;

}

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

/* On a data extent, compute bytes to the end of the extent,

* possibly including a partial sector at EOF. */

*pnum = MIN(bytes, hole - offset);

+

+ /*

+ * We are not allowed to return partial sectors, though, so

+ * round up if necessary.

+ */

+ if (!QEMU_IS_ALIGNED(*pnum, bs->bl.request_alignment)) {

+ int64_t file_length = qemu_gluster_getlength(bs);

+ if (file_length > 0) {

+ /* Ignore errors, this is just a safeguard */

+ assert(hole == file_length);

+ }

+ *pnum = ROUND_UP(*pnum, bs->bl.request_alignment);

+ }

+

ret = BDRV_BLOCK_DATA;

} else {

/* On a hole, compute bytes to the beginning of the next extent. */

--

2.31.1

There is a comment above the BDS definition stating care must be taken

to consider handling newly added fields in bdrv_append().

Actually, this comment should have said "bdrv_swap()" as of 4ddc07cac

(nine years ago), and in any case, bdrv_swap() was dropped in

8e419aefa (six years ago). So no such care is necessary anymore.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

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

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

Message-Id: <20210812084148.14458-2-hreitz@redhat.com>

---

include/block/block_int.h | 6 ------

1 file changed, 6 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ struct BdrvChild {

QLIST_ENTRY(BdrvChild) next_parent;

};

-/*

- * Note: the function bdrv_append() copies and swaps contents of

- * BlockDriverStates, so if you add new fields to this struct, please

- * inspect bdrv_append() to determine if the new fields need to be

- * copied as well.

- */

struct BlockDriverState {

/* Protected by big QEMU lock or read-only after opening. No special

* locking needed during I/O...

--

2.31.1

As we have attempted before

(https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06451.html,

"file-posix: Cache lseek result for data regions";

https://lists.nongnu.org/archive/html/qemu-block/2021-02/msg00934.html,

"file-posix: Cache next hole"), this patch seeks to reduce the number of

SEEK_DATA/HOLE operations the file-posix driver has to perform. The

main difference is that this time it is implemented as part of the

general block layer code.

The problem we face is that on some filesystems or in some

circumstances, SEEK_DATA/HOLE is unreasonably slow. Given the

implementation is outside of qemu, there is little we can do about its

performance.

We have already introduced the want_zero parameter to

bdrv_co_block_status() to reduce the number of SEEK_DATA/HOLE calls

unless we really want zero information; but sometimes we do want that

information, because for files that consist largely of zero areas,

special-casing those areas can give large performance boosts. So the

real problem is with files that consist largely of data, so that

inquiring the block status does not gain us much performance, but where

such an inquiry itself takes a lot of time.

To address this, we want to cache data regions. Most of the time, when

bad performance is reported, it is in places where the image is iterated

over from start to end (qemu-img convert or the mirror job), so a simple

yet effective solution is to cache only the current data region.

(Note that only caching data regions but not zero regions means that

returning false information from the cache is not catastrophic: Treating

zeroes as data is fine. While we try to invalidate the cache on zero

writes and discards, such incongruences may still occur when there are

other processes writing to the image.)

We only use the cache for nodes without children (i.e. protocol nodes),

because that is where the problem is: Drivers that rely on block-status

implementations outside of qemu (e.g. SEEK_DATA/HOLE).

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/307

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210812084148.14458-3-hreitz@redhat.com>

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

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

[hreitz: Added `local_file == bs` assertion, as suggested by Vladimir]

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

include/block/block_int.h | 50 ++++++++++++++++++++++++

block.c | 80 +++++++++++++++++++++++++++++++++++++++

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

3 files changed, 195 insertions(+), 3 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

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

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

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

#include "qemu/hbitmap.h"

#include "block/snapshot.h"

#include "qemu/throttle.h"

+#include "qemu/rcu.h"

#define BLOCK_FLAG_LAZY_REFCOUNTS 8

@@ -XXX,XX +XXX,XX @@ struct BdrvChild {

QLIST_ENTRY(BdrvChild) next_parent;

};

+/*

+ * Allows bdrv_co_block_status() to cache one data region for a

+ * protocol node.

+ *

+ * @valid: Whether the cache is valid (should be accessed with atomic

+ * functions so this can be reset by RCU readers)

+ * @data_start: Offset where we know (or strongly assume) is data

+ * @data_end: Offset where the data region ends (which is not necessarily

+ * the start of a zeroed region)

+ */

+typedef struct BdrvBlockStatusCache {

+ struct rcu_head rcu;

+

+ bool valid;

+ int64_t data_start;

+ int64_t data_end;

+} BdrvBlockStatusCache;

+

struct BlockDriverState {

/* Protected by big QEMU lock or read-only after opening. No special

* locking needed during I/O...

@@ -XXX,XX +XXX,XX @@ struct BlockDriverState {

/* BdrvChild links to this node may never be frozen */

bool never_freeze;

+

+ /* Lock for block-status cache RCU writers */

+ CoMutex bsc_modify_lock;

+ /* Always non-NULL, but must only be dereferenced under an RCU read guard */

+ BdrvBlockStatusCache *block_status_cache;

};

struct BlockBackendRootState {

@@ -XXX,XX +XXX,XX @@ static inline BlockDriverState *bdrv_primary_bs(BlockDriverState *bs)

*/

void bdrv_drain_all_end_quiesce(BlockDriverState *bs);

+/**

+ * Check whether the given offset is in the cached block-status data

+ * region.

+ *

+ * If it is, and @pnum is not NULL, *pnum is set to

+ * `bsc.data_end - offset`, i.e. how many bytes, starting from

+ * @offset, are data (according to the cache).

+ * Otherwise, *pnum is not touched.

+ */

+bool bdrv_bsc_is_data(BlockDriverState *bs, int64_t offset, int64_t *pnum);

+

+/**

+ * If [offset, offset + bytes) overlaps with the currently cached

+ * block-status region, invalidate the cache.

+ *

+ * (To be used by I/O paths that cause data regions to be zero or

+ * holes.)

+ */

+void bdrv_bsc_invalidate_range(BlockDriverState *bs,

+ int64_t offset, int64_t bytes);

+

+/**

+ * Mark the range [offset, offset + bytes) as a data region.

+ */

+void bdrv_bsc_fill(BlockDriverState *bs, int64_t offset, int64_t bytes);

+

#endif /* BLOCK_INT_H */

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

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

#include "qemu/timer.h"

#include "qemu/cutils.h"

#include "qemu/id.h"

+#include "qemu/range.h"

+#include "qemu/rcu.h"

#include "block/coroutines.h"

#ifdef CONFIG_BSD

@@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_new(void)

qemu_co_queue_init(&bs->flush_queue);

+ qemu_co_mutex_init(&bs->bsc_modify_lock);

+ bs->block_status_cache = g_new0(BdrvBlockStatusCache, 1);

+

for (i = 0; i < bdrv_drain_all_count; i++) {

bdrv_drained_begin(bs);

}

@@ -XXX,XX +XXX,XX @@ static void bdrv_close(BlockDriverState *bs)

bs->explicit_options = NULL;

qobject_unref(bs->full_open_options);

bs->full_open_options = NULL;

+ g_free(bs->block_status_cache);

+ bs->block_status_cache = NULL;

bdrv_release_named_dirty_bitmaps(bs);

assert(QLIST_EMPTY(&bs->dirty_bitmaps));

@@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs)

{

return bdrv_skip_filters(bdrv_cow_bs(bdrv_skip_filters(bs)));

}

+

+/**

+ * Check whether [offset, offset + bytes) overlaps with the cached

+ * block-status data region.

+ *

+ * If so, and @pnum is not NULL, set *pnum to `bsc.data_end - offset`,

+ * which is what bdrv_bsc_is_data()'s interface needs.

+ * Otherwise, *pnum is not touched.

+ */

+static bool bdrv_bsc_range_overlaps_locked(BlockDriverState *bs,

+ int64_t offset, int64_t bytes,

+ int64_t *pnum)

+{

+ BdrvBlockStatusCache *bsc = qatomic_rcu_read(&bs->block_status_cache);

+ bool overlaps;

+

+ overlaps =

+ qatomic_read(&bsc->valid) &&

+ ranges_overlap(offset, bytes, bsc->data_start,

+ bsc->data_end - bsc->data_start);

+

+ if (overlaps && pnum) {

+ *pnum = bsc->data_end - offset;

+ }

+

+ return overlaps;

+}

+

+/**

+ * See block_int.h for this function's documentation.

+ */

+bool bdrv_bsc_is_data(BlockDriverState *bs, int64_t offset, int64_t *pnum)

+{

+ RCU_READ_LOCK_GUARD();

+

+ return bdrv_bsc_range_overlaps_locked(bs, offset, 1, pnum);

+}

+

+/**

+ * See block_int.h for this function's documentation.

+ */

+void bdrv_bsc_invalidate_range(BlockDriverState *bs,

+ int64_t offset, int64_t bytes)

+{

+ RCU_READ_LOCK_GUARD();

+

+ if (bdrv_bsc_range_overlaps_locked(bs, offset, bytes, NULL)) {

+ qatomic_set(&bs->block_status_cache->valid, false);

+ }

+}

+

+/**

+ * See block_int.h for this function's documentation.

+ */

+void bdrv_bsc_fill(BlockDriverState *bs, int64_t offset, int64_t bytes)

+{

+ BdrvBlockStatusCache *new_bsc = g_new(BdrvBlockStatusCache, 1);

+ BdrvBlockStatusCache *old_bsc;

+

+ *new_bsc = (BdrvBlockStatusCache) {

+ .valid = true,

+ .data_start = offset,

+ .data_end = offset + bytes,

+ };

+

+ QEMU_LOCK_GUARD(&bs->bsc_modify_lock);

+

+ old_bsc = qatomic_rcu_read(&bs->block_status_cache);

+ qatomic_rcu_set(&bs->block_status_cache, new_bsc);

+ if (old_bsc) {

+ g_free_rcu(old_bsc, rcu);

+ }

+}

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_do_pwrite_zeroes(BlockDriverState *bs,

return -ENOTSUP;

}

+ /* Invalidate the cached block-status data range if this write overlaps */

+ bdrv_bsc_invalidate_range(bs, offset, bytes);

+

assert(alignment % bs->bl.request_alignment == 0);

head = offset % alignment;

tail = (offset + bytes) % alignment;

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

aligned_bytes = ROUND_UP(offset + bytes, align) - aligned_offset;

if (bs->drv->bdrv_co_block_status) {

- ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,

- aligned_bytes, pnum, &local_map,

- &local_file);

+ /*

+ * Use the block-status cache only for protocol nodes: Format

+ * drivers are generally quick to inquire the status, but protocol

+ * drivers often need to get information from outside of qemu, so

+ * we do not have control over the actual implementation. There

+ * have been cases where inquiring the status took an unreasonably

+ * long time, and we can do nothing in qemu to fix it.

+ * This is especially problematic for images with large data areas,

+ * because finding the few holes in them and giving them special

+ * treatment does not gain much performance. Therefore, we try to

+ * cache the last-identified data region.

+ *

+ * Second, limiting ourselves to protocol nodes allows us to assume

+ * the block status for data regions to be DATA | OFFSET_VALID, and

+ * that the host offset is the same as the guest offset.

+ *

+ * Note that it is possible that external writers zero parts of

+ * the cached regions without the cache being invalidated, and so

+ * we may report zeroes as data. This is not catastrophic,

+ * however, because reporting zeroes as data is fine.

+ */

+ if (QLIST_EMPTY(&bs->children) &&

+ bdrv_bsc_is_data(bs, aligned_offset, pnum))

+ {

+ ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;

+ local_file = bs;

+ local_map = aligned_offset;

+ } else {

+ ret = bs->drv->bdrv_co_block_status(bs, want_zero, aligned_offset,

+ aligned_bytes, pnum, &local_map,

+ &local_file);

+

+ /*

+ * Note that checking QLIST_EMPTY(&bs->children) is also done when

+ * the cache is queried above. Technically, we do not need to check

+ * it here; the worst that can happen is that we fill the cache for

+ * non-protocol nodes, and then it is never used. However, filling

+ * the cache requires an RCU update, so double check here to avoid

+ * such an update if possible.

+ */

+ if (ret == (BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID) &&

+ QLIST_EMPTY(&bs->children))

+ {

+ /*

+ * When a protocol driver reports BLOCK_OFFSET_VALID, the

+ * returned local_map value must be the same as the offset we

+ * have passed (aligned_offset), and local_bs must be the node

+ * itself.

+ * Assert this, because we follow this rule when reading from

+ * the cache (see the `local_file = bs` and

+ * `local_map = aligned_offset` assignments above), and the

+ * result the cache delivers must be the same as the driver

+ * would deliver.

+ */

+ assert(local_file == bs);

+ assert(local_map == aligned_offset);

+ bdrv_bsc_fill(bs, aligned_offset, *pnum);

+ }

+ }

} else {

/* Default code for filters */

@@ -XXX,XX +XXX,XX @@ int coroutine_fn bdrv_co_pdiscard(BdrvChild *child, int64_t offset,

return 0;

}

+ /* Invalidate the cached block-status data range if this discard overlaps */

+ bdrv_bsc_invalidate_range(bs, offset, bytes);

+

/* Discard is advisory, but some devices track and coalesce

* unaligned requests, so we must pass everything down rather than

* round here. Still, most devices will just silently ignore

--

2.31.1

.bdrv_co_block_status() implementations are free to return a *pnum that

exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp

*pnum as necessary.

On the other hand, if drivers' implementations return values for *pnum

that are as large as possible, our recently introduced block-status

cache will become more effective.

So, make a note in block_int.h that @bytes is no upper limit for *pnum.

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

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

Message-Id: <20210812084148.14458-4-hreitz@redhat.com>

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

---

include/block/block_int.h | 9 +++++++++

1 file changed, 9 insertions(+)

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ struct BlockDriver {

* clamped to bdrv_getlength() and aligned to request_alignment,

* as well as non-NULL pnum, map, and file; in turn, the driver

* must return an error or set pnum to an aligned non-zero value.

+ *

+ * Note that @bytes is just a hint on how big of a region the

+ * caller wants to inspect. It is not a limit on *pnum.

+ * Implementations are free to return larger values of *pnum if

+ * doing so does not incur a performance penalty.

+ *

+ * block/io.c's bdrv_co_block_status() will utilize an unclamped

+ * *pnum value for the block-status cache on protocol nodes, prior

+ * to clamping *pnum for return to its caller.

*/

int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,

bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,

--

2.31.1

bdrv_co_block_status() does it for us, we do not need to do it here.

The advantage of not capping *pnum is that bdrv_co_block_status() can

cache larger data regions than requested by its caller.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

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

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

Message-Id: <20210812084148.14458-5-hreitz@redhat.com>

---

block/file-posix.c | 7 ++++---

1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c

index XXXXXXX..XXXXXXX 100644

--- a/block/file-posix.c

+++ b/block/file-posix.c

@@ -XXX,XX +XXX,XX @@ static int find_allocation(BlockDriverState *bs, off_t start,

* the specified offset) that are known to be in the same

* allocated/unallocated state.

*

- * 'bytes' is the max value 'pnum' should be set to.

+ * 'bytes' is a soft cap for 'pnum'. If the information is free, 'pnum' may

+ * well exceed it.

*/

static int coroutine_fn raw_co_block_status(BlockDriverState *bs,

bool want_zero,

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

} else if (data == offset) {

/* On a data extent, compute bytes to the end of the extent,

* possibly including a partial sector at EOF. */

- *pnum = MIN(bytes, hole - offset);

+ *pnum = hole - offset;

/*

* We are not allowed to return partial sectors, though, so

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

} else {

/* On a hole, compute bytes to the beginning of the next extent. */

assert(hole == offset);

- *pnum = MIN(bytes, data - offset);

+ *pnum = data - offset;

ret = BDRV_BLOCK_ZERO;

}

*map = offset;

--

2.31.1

bdrv_co_block_status() does it for us, we do not need to do it here.

The advantage of not capping *pnum is that bdrv_co_block_status() can

cache larger data regions than requested by its caller.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

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

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

Message-Id: <20210812084148.14458-6-hreitz@redhat.com>

---

block/gluster.c | 7 ++++---

1 file changed, 4 insertions(+), 3 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/gluster.c

+++ b/block/gluster.c

@@ -XXX,XX +XXX,XX @@ exit:

* the specified offset) that are known to be in the same

* allocated/unallocated state.

*

- * 'bytes' is the max value 'pnum' should be set to.

+ * 'bytes' is a soft cap for 'pnum'. If the information is free, 'pnum' may

+ * well exceed it.

*

* (Based on raw_co_block_status() from file-posix.c.)

*/

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

} else if (data == offset) {

/* On a data extent, compute bytes to the end of the extent,

* possibly including a partial sector at EOF. */

- *pnum = MIN(bytes, hole - offset);

+ *pnum = hole - offset;

/*

* We are not allowed to return partial sectors, though, so

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

} else {

/* On a hole, compute bytes to the beginning of the next extent. */

assert(hole == offset);

- *pnum = MIN(bytes, data - offset);

+ *pnum = data - offset;

ret = BDRV_BLOCK_ZERO;

}

--

2.31.1

bdrv_co_block_status() does it for us, we do not need to do it here.

The advantage of not capping *pnum is that bdrv_co_block_status() can

cache larger data regions than requested by its caller.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

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

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

Message-Id: <20210812084148.14458-7-hreitz@redhat.com>

---

block/iscsi.c | 3 ---

1 file changed, 3 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/iscsi.c

+++ b/block/iscsi.c

@@ -XXX,XX +XXX,XX @@ retry:

iscsi_allocmap_set_allocated(iscsilun, offset, *pnum);

}

- if (*pnum > bytes) {

- *pnum = bytes;

- }

out_unlock:

qemu_mutex_unlock(&iscsilun->mutex);

g_free(iTask.err_str);

--

2.31.1

As of recently, pylint complains when `open()` calls are missing an

`encoding=` specified. Everything we have should be UTF-8 (and in fact,

everything should be UTF-8, period (exceptions apply)), so use that.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210824153540.177128-2-hreitz@redhat.com>

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

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

---

tests/qemu-iotests/297 | 2 +-

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

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/297

+++ b/tests/qemu-iotests/297

@@ -XXX,XX +XXX,XX @@ def is_python_file(filename):

if filename.endswith('.py'):

return True

- with open(filename) as f:

+ with open(filename, encoding='utf-8') as f:

try:

first_line = f.readline()

return re.match('^#!.*python', first_line) is not None

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 _post_shutdown(self) -> None:

return

valgrind_filename = f"{test_dir}/{self._popen.pid}.valgrind"

if self.exitcode() == 99:

- with open(valgrind_filename) as f:

+ with open(valgrind_filename, encoding='utf-8') as f:

print(f.read())

else:

os.remove(valgrind_filename)

@@ -XXX,XX +XXX,XX @@ def notrun(reason):

# Each test in qemu-iotests has a number ("seq")

seq = os.path.basename(sys.argv[0])

- with open('%s/%s.notrun' % (output_dir, seq), 'w') as outfile:

+ with open('%s/%s.notrun' % (output_dir, seq), 'w', encoding='utf-8') \

+ as outfile:

outfile.write(reason + '\n')

logger.warning("%s not run: %s", seq, reason)

sys.exit(0)

@@ -XXX,XX +XXX,XX @@ def case_notrun(reason):

# Each test in qemu-iotests has a number ("seq")

seq = os.path.basename(sys.argv[0])

- with open('%s/%s.casenotrun' % (output_dir, seq), 'a') as outfile:

+ with open('%s/%s.casenotrun' % (output_dir, seq), 'a', encoding='utf-8') \

+ as outfile:

outfile.write(' [case not run] ' + reason + '\n')

def _verify_image_format(supported_fmts: Sequence[str] = (),

--

2.31.1

pylint proposes using `[]` instead of `list()` and `{}` instead of

`dict()`, because it is faster. That seems simple enough, so heed its

advice.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210824153540.177128-3-hreitz@redhat.com>

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

---

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

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

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 hmp_qemu_io(self, drive: str, cmd: str,

def flatten_qmp_object(self, obj, output=None, basestr=''):

if output is None:

- output = dict()

+ output = {}

if isinstance(obj, list):

for i, item in enumerate(obj):

self.flatten_qmp_object(item, output, basestr + str(i) + '.')

@@ -XXX,XX +XXX,XX @@ def flatten_qmp_object(self, obj, output=None, basestr=''):

def qmp_to_opts(self, obj):

obj = self.flatten_qmp_object(obj)

- output_list = list()

+ output_list = []

for key in obj:

output_list += [key + '=' + obj[key]]

return ','.join(output_list)

--

2.31.1

169 and 199 have been renamed and moved to tests/ (commit a44be0334be:

"iotests: rename and move 169 and 199 tests"), so we can drop them from

the skip list.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

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

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

Message-Id: <20210902094017.32902-2-hreitz@redhat.com>

---

tests/qemu-iotests/297 | 2 +-

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/297

+++ b/tests/qemu-iotests/297

@@ -XXX,XX +XXX,XX @@ import iotests

SKIP_FILES = (

'030', '040', '041', '044', '045', '055', '056', '057', '065', '093',

'096', '118', '124', '132', '136', '139', '147', '148', '149',

- '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',

+ '151', '152', '155', '163', '165', '194', '196', '202',

'203', '205', '206', '207', '208', '210', '211', '212', '213', '216',

'218', '219', '224', '228', '234', '235', '236', '237', '238',

'240', '242', '245', '246', '248', '255', '256', '257', '258', '260',

--

2.31.1

pylint complains that discards1_sha256 and all_discards_sha256 are first

set in non-__init__ methods.

These variables are not really class-variables anyway, so let them

instead be returned by start_postcopy(), thus silencing pylint.

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

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

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

Message-Id: <20210902094017.32902-3-hreitz@redhat.com>

---

.../tests/migrate-bitmaps-postcopy-test | 13 +++++++------

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

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test

+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap0')

- self.discards1_sha256 = result['return']['sha256']

+ discards1_sha256 = result['return']['sha256']

# Check, that updating the bitmap by discards works

- assert self.discards1_sha256 != empty_sha256

+ assert discards1_sha256 != empty_sha256

# We want to calculate resulting sha256. Do it in bitmap0, so, disable

# other bitmaps

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap0')

- self.all_discards_sha256 = result['return']['sha256']

+ all_discards_sha256 = result['return']['sha256']

# Now, enable some bitmaps, to be updated during migration

for i in range(2, nb_bitmaps, 2):

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

event_resume = self.vm_b.event_wait('RESUME')

self.vm_b_events.append(event_resume)

- return event_resume

+ return (event_resume, discards1_sha256, all_discards_sha256)

def test_postcopy_success(self):

- event_resume = self.start_postcopy()

+ event_resume, discards1_sha256, all_discards_sha256 = \

+ self.start_postcopy()

# enabled bitmaps should be updated

apply_discards(self.vm_b, discards2)

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):

for i in range(0, nb_bitmaps, 5):

result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap{}'.format(i))

- sha = self.discards1_sha256 if i % 2 else self.all_discards_sha256

+ sha = discards1_sha256 if i % 2 else all_discards_sha256

self.assert_qmp(result, 'return/sha256', sha)

def test_early_shutdown_destination(self):

--

2.31.1

There are a couple of things pylint takes issue with:

- The "time" import is unused

- The import order (iotests should come last)

- get_bitmap_hash() doesn't use @self and so should be a function

- Semicolons at the end of some lines

- Parentheses after "if"

- Some lines are too long (80 characters instead of 79)

- inject_test_case()'s @name parameter shadows a top-level @name

variable

- "lambda self: mc(self)" were equivalent to just "mc", but in

inject_test_case(), it is not equivalent, so add a comment and disable

the warning locally

- Always put two empty lines after a function

- f'exec: cat > /dev/null' does not need to be an f-string

Fix them.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210902094017.32902-4-hreitz@redhat.com>

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

---

tests/qemu-iotests/tests/migrate-bitmaps-test | 43 +++++++++++--------

1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/tests/migrate-bitmaps-test

+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test

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

#

import os

-import iotests

-import time

import itertools

import operator

import re

+import iotests

from iotests import qemu_img, qemu_img_create, Timeout

@@ -XXX,XX +XXX,XX @@ mig_cmd = 'exec: cat > ' + mig_file

incoming_cmd = 'exec: cat ' + mig_file

+def get_bitmap_hash(vm):

+ result = vm.qmp('x-debug-block-dirty-bitmap-sha256',

+ node='drive0', name='bitmap0')

+ return result['return']['sha256']

+

+

class TestDirtyBitmapMigration(iotests.QMPTestCase):

def tearDown(self):

self.vm_a.shutdown()

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

params['persistent'] = True

result = vm.qmp('block-dirty-bitmap-add', **params)

- self.assert_qmp(result, 'return', {});

-

- def get_bitmap_hash(self, vm):

- result = vm.qmp('x-debug-block-dirty-bitmap-sha256',

- node='drive0', name='bitmap0')

- return result['return']['sha256']

+ self.assert_qmp(result, 'return', {})

def check_bitmap(self, vm, sha256):

result = vm.qmp('x-debug-block-dirty-bitmap-sha256',

node='drive0', name='bitmap0')

if sha256:

- self.assert_qmp(result, 'return/sha256', sha256);

+ self.assert_qmp(result, 'return/sha256', sha256)

else:

self.assert_qmp(result, 'error/desc',

- "Dirty bitmap 'bitmap0' not found");

+ "Dirty bitmap 'bitmap0' not found")

def do_test_migration_resume_source(self, persistent, migrate_bitmaps):

granularity = 512

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

self.add_bitmap(self.vm_a, granularity, persistent)

for r in regions:

self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)

- sha256 = self.get_bitmap_hash(self.vm_a)

+ sha256 = get_bitmap_hash(self.vm_a)

result = self.vm_a.qmp('migrate', uri=mig_cmd)

while True:

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

break

while True:

result = self.vm_a.qmp('query-status')

- if (result['return']['status'] == 'postmigrate'):

+ if result['return']['status'] == 'postmigrate':

break

# test that bitmap is still here

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

self.add_bitmap(self.vm_a, granularity, persistent)

for r in regions:

self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)

- sha256 = self.get_bitmap_hash(self.vm_a)

+ sha256 = get_bitmap_hash(self.vm_a)

if pre_shutdown:

self.vm_a.shutdown()

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):

self.check_bitmap(self.vm_b, sha256 if persistent else False)

-def inject_test_case(klass, name, method, *args, **kwargs):

+def inject_test_case(klass, suffix, method, *args, **kwargs):

mc = operator.methodcaller(method, *args, **kwargs)

- setattr(klass, 'test_' + method + name, lambda self: mc(self))

+ # We want to add a function attribute to `klass`, so that it is

+ # correctly converted to a method on instantiation. The

+ # methodcaller object `mc` is a callable, not a function, so we

+ # need the lambda to turn it into a function.

+ # pylint: disable=unnecessary-lambda

+ setattr(klass, 'test_' + method + suffix, lambda self: mc(self))

+

for cmb in list(itertools.product((True, False), repeat=5)):

name = ('_' if cmb[0] else '_not_') + 'persistent_'

name += ('_' if cmb[1] else '_not_') + 'migbitmap_'

name += '_online' if cmb[2] else '_offline'

name += '_shared' if cmb[3] else '_nonshared'

- if (cmb[4]):

+ if cmb[4]:

name += '__pre_shutdown'

inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):

self.assert_qmp(result, 'return', {})

# Check that the bitmaps are there

- for node in self.vm.qmp('query-named-block-nodes', flat=True)['return']:

+ nodes = self.vm.qmp('query-named-block-nodes', flat=True)['return']

+ for node in nodes:

if 'node0' in node['node-name']:

self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')

@@ -XXX,XX +XXX,XX @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):

"""

Continue the source after migration.

"""

- result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')

+ result = self.vm.qmp('migrate', uri='exec: cat > /dev/null')

self.assert_qmp(result, 'return', {})

with Timeout(10, 'Migration timeout'):

--

2.31.1

The AbnormalShutdown exception class is not in qemu.machine, but in

qemu.machine.machine. (qemu.machine.AbnormalShutdown was enough for

Python to find it in order to run this test, but pylint complains about

it.)

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Message-Id: <20210902094017.32902-5-hreitz@redhat.com>

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

---

tests/qemu-iotests/tests/mirror-top-perms | 2 +-

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

diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/tests/mirror-top-perms

+++ b/tests/qemu-iotests/tests/mirror-top-perms

@@ -XXX,XX +XXX,XX @@ class TestMirrorTopPerms(iotests.QMPTestCase):

def tearDown(self):

try:

self.vm.shutdown()

- except qemu.machine.AbnormalShutdown:

+ except qemu.machine.machine.AbnormalShutdown:

pass

if self.vm_b is not None:

--

2.31.1

297 so far does not check the named tests, which reside in the tests/

directory (i.e. full path tests/qemu-iotests/tests). Fix it.

Thanks to the previous two commits, all named tests pass its scrutiny,

so we do not have to add anything to SKIP_FILES.

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

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

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

Message-Id: <20210902094017.32902-6-hreitz@redhat.com>

---

tests/qemu-iotests/297 | 5 +++--

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

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

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/297

+++ b/tests/qemu-iotests/297

@@ -XXX,XX +XXX,XX @@ def is_python_file(filename):

def run_linters():

- files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))

- if is_python_file(filename)]

+ named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]

+ check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)

+ files = [filename for filename in check_tests if is_python_file(filename)]

iotests.logger.debug('Files to be checked:')

iotests.logger.debug(', '.join(sorted(files)))

--

2.31.1

From: Stefano Garzarella <sgarzare@redhat.com>

In mirror_iteration() we call mirror_wait_on_conflicts() with

`self` parameter set to NULL.

Starting from commit d44dae1a7c we dereference `self` pointer in

mirror_wait_on_conflicts() without checks if it is not NULL.

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.

#0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)

at ../block/mirror.c:172

172     self->waiting_for_op = op;

[Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]

(gdb) bt

#0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)

at ../block/mirror.c:172

#1 0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized out>) at ../block/mirror.c:491

#2 0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at ../job.c:917

#3 0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>)

at ../util/coroutine-ucontext.c:173

#4 0x00007f0909975820 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91

from /usr/lib64/libc.so.6

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404

Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in mirror_wait_on_conflicts")

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Message-Id: <20210910124533.288318-1-sgarzare@redhat.com>

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

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

block/mirror.c | 25 ++++++++++++++++---------

1 file changed, 16 insertions(+), 9 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,

if (ranges_overlap(self_start_chunk, self_nb_chunks,

op_start_chunk, op_nb_chunks))

{

- /*

- * If the operation is already (indirectly) waiting for us, or

- * will wait for us as soon as it wakes up, then just go on

- * (instead of producing a deadlock in the former case).

- */

- if (op->waiting_for_op) {

- continue;

+ if (self) {

+ /*

+ * If the operation is already (indirectly) waiting for us,

+ * or will wait for us as soon as it wakes up, then just go

+ * on (instead of producing a deadlock in the former case).

+ */

+ if (op->waiting_for_op) {

+ continue;

+ }

+

+ self->waiting_for_op = op;

}

- self->waiting_for_op = op;

qemu_co_queue_wait(&op->waiting_requests, NULL);

- self->waiting_for_op = NULL;

+

+ if (self) {

+ self->waiting_for_op = NULL;

+ }

+

break;

}

}

--

2.31.1

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

Add a simple test which tries to run migration during backup.

bdrv_inactivate_all() should fail. But due to bug (see next commit with

fix) it doesn't, nodes are inactivated and continued backup crashes

on assertion "assert(!(bs->open_flags & BDRV_O_INACTIVE));" in

bdrv_co_write_req_prepare().

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

Message-Id: <20210911120027.8063-2-vsementsov@virtuozzo.com>

Signed-off-by: Hanna Reitz <hreitz@redhat.com>

---

.../qemu-iotests/tests/migrate-during-backup | 97 +++++++++++++++++++

.../tests/migrate-during-backup.out | 5 +

2 files changed, 102 insertions(+)

create mode 100755 tests/qemu-iotests/tests/migrate-during-backup

create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out

diff --git a/tests/qemu-iotests/tests/migrate-during-backup b/tests/qemu-iotests/tests/migrate-during-backup

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/tests/migrate-during-backup

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

+#!/usr/bin/env python3

+# group: migration disabled

+#

+# Copyright (c) 2021 Virtuozzo International GmbH

+#

+# 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, qemu_io

+

+

+disk_a = os.path.join(iotests.test_dir, 'disk_a')

+disk_b = os.path.join(iotests.test_dir, 'disk_b')

+size = '1M'

+mig_file = os.path.join(iotests.test_dir, 'mig_file')

+mig_cmd = 'exec: cat > ' + mig_file

+

+

+class TestMigrateDuringBackup(iotests.QMPTestCase):

+ def tearDown(self):

+ self.vm.shutdown()

+ os.remove(disk_a)

+ os.remove(disk_b)

+ os.remove(mig_file)

+

+ def setUp(self):

+ qemu_img_create('-f', iotests.imgfmt, disk_a, size)

+ qemu_img_create('-f', iotests.imgfmt, disk_b, size)

+ qemu_io('-c', f'write 0 {size}', disk_a)

+

+ self.vm = iotests.VM().add_drive(disk_a)

+ self.vm.launch()

+ result = self.vm.qmp('blockdev-add', {

+ 'node-name': 'target',

+ 'driver': iotests.imgfmt,

+ 'file': {