* [PULL 01/22] qemu-iotests: Fix FilePaths cleanup
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 02/22] qemu-iotests: Fix FilePaths docstring Max Reitz
` (21 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Nir Soffer <nirsof@gmail.com>
If os.remove() fails to remove one of the paths, for example if the file
was removed by the test, the cleanup loop would exit silently, without
removing the rest of the files.
Fixes: de263986b5dc
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200828232152.205833-2-nsoffer@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 64ccaf9061..36814daf84 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -468,11 +468,11 @@ class FilePaths:
return self.paths
def __exit__(self, exc_type, exc_val, exc_tb):
- try:
- for path in self.paths:
+ for path in self.paths:
+ try:
os.remove(path)
- except OSError:
- pass
+ except OSError:
+ pass
return False
class FilePath(FilePaths):
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 02/22] qemu-iotests: Fix FilePaths docstring
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
2020-09-15 10:46 ` [PULL 01/22] qemu-iotests: Fix FilePaths cleanup Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 03/22] qemu-iotests: Support varargs syntax in FilePaths Max Reitz
` (20 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Nir Soffer <nirsof@gmail.com>
When this class was extracted from FilePath, the docstring was not
updated for generating multiple files, and the example usage was
referencing unrelated file.
While fixing the docstring, add example for creating sockets, which
should use iotests.sock_dir instead of the default base_dir.
Fixes: de263986b5dc
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Message-Id: <20200828232152.205833-3-nsoffer@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 36814daf84..bbe63a6da0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -450,14 +450,21 @@ def file_pattern(name):
class FilePaths:
"""
- FilePaths is an auto-generated filename that cleans itself up.
+ Context manager generating multiple file names. The generated files are
+ removed when exiting the context.
- Use this context manager to generate filenames and ensure that the file
- gets deleted::
+ Example usage:
+
+ with FilePaths(['a.img', 'b.img']) as (img_a, img_b):
+ # Use img_a and img_b here...
+
+ # a.img and b.img are automatically removed here.
+
+ By default images are created in iotests.test_dir. To create sockets use
+ iotests.sock_dir:
+
+ with FilePaths(['a.sock'], base_dir=iotests.sock_dir) as (sock,):
- with FilePaths(['test.img']) as img_path:
- qemu_img('create', img_path, '1G')
- # migration_sock_path is automatically deleted
"""
def __init__(self, names, base_dir=test_dir):
self.paths = []
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 03/22] qemu-iotests: Support varargs syntax in FilePaths
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
2020-09-15 10:46 ` [PULL 01/22] qemu-iotests: Fix FilePaths cleanup Max Reitz
2020-09-15 10:46 ` [PULL 02/22] qemu-iotests: Fix FilePaths docstring Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 04/22] qemu-iotests: Merge FilePaths and FilePath Max Reitz
` (19 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Nir Soffer <nirsof@gmail.com>
Accept variable number of names instead of a sequence:
with FilePaths("a", "b", "c") as (a, b, c):
The disadvantage is that base_dir must be used as kwarg:
with FilePaths("a", "b", base_dir=soc_dir) as (sock1, sock2):
But this is more clear and calling optional argument as positional
arguments is bad idea anyway.
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200828232152.205833-4-nsoffer@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/194 | 4 ++--
tests/qemu-iotests/257 | 10 ++++------
tests/qemu-iotests/iotests.py | 8 ++++----
3 files changed, 10 insertions(+), 12 deletions(-)
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index da7c4265ec..08389f474e 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -26,8 +26,8 @@ iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
with iotests.FilePath('source.img') as source_img_path, \
iotests.FilePath('dest.img') as dest_img_path, \
- iotests.FilePaths(['migration.sock', 'nbd.sock'], iotests.sock_dir) as \
- [migration_sock_path, nbd_sock_path], \
+ iotests.FilePaths('migration.sock', 'nbd.sock', base_dir=iotests.sock_dir) \
+ as (migration_sock_path, nbd_sock_path), \
iotests.VM('source') as source_vm, \
iotests.VM('dest') as dest_vm:
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index e1e6077219..a9aa65bbe3 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -275,10 +275,9 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
an incomplete backup. Testing limitations prevent
testing competing writes.
"""
- with iotests.FilePaths(['img', 'bsync1', 'bsync2',
- 'fbackup0', 'fbackup1', 'fbackup2']) as \
- (img_path, bsync1, bsync2,
- fbackup0, fbackup1, fbackup2), \
+ with iotests.FilePaths(
+ 'img', 'bsync1', 'bsync2', 'fbackup0', 'fbackup1', 'fbackup2') as \
+ (img_path, bsync1, bsync2, fbackup0, fbackup1, fbackup2), \
iotests.VM() as vm:
mode = "Mode {:s}; Bitmap Sync {:s}".format(msync_mode, bsync_mode)
@@ -441,8 +440,7 @@ def test_backup_api():
"""
Test malformed and prohibited invocations of the backup API.
"""
- with iotests.FilePaths(['img', 'bsync1']) as \
- (img_path, backup_path), \
+ with iotests.FilePaths('img', 'bsync1') as (img_path, backup_path), \
iotests.VM() as vm:
log("\n=== API failure tests ===\n")
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bbe63a6da0..635ec99431 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -455,7 +455,7 @@ class FilePaths:
Example usage:
- with FilePaths(['a.img', 'b.img']) as (img_a, img_b):
+ with FilePaths('a.img', 'b.img') as (img_a, img_b):
# Use img_a and img_b here...
# a.img and b.img are automatically removed here.
@@ -463,10 +463,10 @@ class FilePaths:
By default images are created in iotests.test_dir. To create sockets use
iotests.sock_dir:
- with FilePaths(['a.sock'], base_dir=iotests.sock_dir) as (sock,):
+ with FilePaths('a.sock', base_dir=iotests.sock_dir) as (sock,):
"""
- def __init__(self, names, base_dir=test_dir):
+ def __init__(self, *names, base_dir=test_dir):
self.paths = []
for name in names:
self.paths.append(os.path.join(base_dir, file_pattern(name)))
@@ -487,7 +487,7 @@ class FilePath(FilePaths):
FilePath is a specialization of FilePaths that takes a single filename.
"""
def __init__(self, name, base_dir=test_dir):
- super(FilePath, self).__init__([name], base_dir)
+ super(FilePath, self).__init__(name, base_dir=base_dir)
def __enter__(self):
return self.paths[0]
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 04/22] qemu-iotests: Merge FilePaths and FilePath
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (2 preceding siblings ...)
2020-09-15 10:46 ` [PULL 03/22] qemu-iotests: Support varargs syntax in FilePaths Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 05/22] qemu-iotests: Simplify FilePath __init__ Max Reitz
` (18 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Nir Soffer <nirsof@gmail.com>
FilePath creates now one temporary file:
with FilePath("a") as a:
Or more:
with FilePath("a", "b", "c") as (a, b, c):
This is also the behavior of the file_path() helper, used by some of the
tests. Now we have only 2 helpers for creating temporary files instead
of 3.
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200828232152.205833-5-nsoffer@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/194 | 2 +-
tests/qemu-iotests/208 | 2 +-
tests/qemu-iotests/222 | 2 +-
tests/qemu-iotests/257 | 4 ++--
tests/qemu-iotests/iotests.py | 23 ++++++++++-------------
5 files changed, 15 insertions(+), 18 deletions(-)
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 08389f474e..7a4863ab18 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -26,7 +26,7 @@ iotests.script_initialize(supported_fmts=['qcow2', 'qed', 'raw'],
with iotests.FilePath('source.img') as source_img_path, \
iotests.FilePath('dest.img') as dest_img_path, \
- iotests.FilePaths('migration.sock', 'nbd.sock', base_dir=iotests.sock_dir) \
+ iotests.FilePath('migration.sock', 'nbd.sock', base_dir=iotests.sock_dir) \
as (migration_sock_path, nbd_sock_path), \
iotests.VM('source') as source_vm, \
iotests.VM('dest') as dest_vm:
diff --git a/tests/qemu-iotests/208 b/tests/qemu-iotests/208
index 6cb642f821..54aa4be273 100755
--- a/tests/qemu-iotests/208
+++ b/tests/qemu-iotests/208
@@ -26,7 +26,7 @@ iotests.script_initialize(supported_fmts=['generic'])
with iotests.FilePath('disk.img') as disk_img_path, \
iotests.FilePath('disk-snapshot.img') as disk_snapshot_img_path, \
- iotests.FilePath('nbd.sock', iotests.sock_dir) as nbd_sock_path, \
+ iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \
iotests.VM() as vm:
img_size = '10M'
diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 6602f6b4ba..14d67c875b 100755
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -49,7 +49,7 @@ remainder = [("0xd5", "0x108000", "32k"), # Right-end of partial-left [1]
with iotests.FilePath('base.img') as base_img_path, \
iotests.FilePath('fleece.img') as fleece_img_path, \
- iotests.FilePath('nbd.sock', iotests.sock_dir) as nbd_sock_path, \
+ iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \
iotests.VM() as vm:
log('--- Setting up images ---')
diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index a9aa65bbe3..c80e06ae28 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -275,7 +275,7 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
an incomplete backup. Testing limitations prevent
testing competing writes.
"""
- with iotests.FilePaths(
+ with iotests.FilePath(
'img', 'bsync1', 'bsync2', 'fbackup0', 'fbackup1', 'fbackup2') as \
(img_path, bsync1, bsync2, fbackup0, fbackup1, fbackup2), \
iotests.VM() as vm:
@@ -440,7 +440,7 @@ def test_backup_api():
"""
Test malformed and prohibited invocations of the backup API.
"""
- with iotests.FilePaths('img', 'bsync1') as (img_path, backup_path), \
+ with iotests.FilePath('img', 'bsync1') as (img_path, backup_path), \
iotests.VM() as vm:
log("\n=== API failure tests ===\n")
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 635ec99431..1799338f8b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -448,14 +448,14 @@ class Timeout:
def file_pattern(name):
return "{0}-{1}".format(os.getpid(), name)
-class FilePaths:
+class FilePath:
"""
Context manager generating multiple file names. The generated files are
removed when exiting the context.
Example usage:
- with FilePaths('a.img', 'b.img') as (img_a, img_b):
+ with FilePath('a.img', 'b.img') as (img_a, img_b):
# Use img_a and img_b here...
# a.img and b.img are automatically removed here.
@@ -463,7 +463,10 @@ class FilePaths:
By default images are created in iotests.test_dir. To create sockets use
iotests.sock_dir:
- with FilePaths('a.sock', base_dir=iotests.sock_dir) as (sock,):
+ with FilePath('a.sock', base_dir=iotests.sock_dir) as sock:
+
+ For convenience, calling with one argument yields a single file instead of
+ a tuple with one item.
"""
def __init__(self, *names, base_dir=test_dir):
@@ -472,7 +475,10 @@ class FilePaths:
self.paths.append(os.path.join(base_dir, file_pattern(name)))
def __enter__(self):
- return self.paths
+ if len(self.paths) == 1:
+ return self.paths[0]
+ else:
+ return self.paths
def __exit__(self, exc_type, exc_val, exc_tb):
for path in self.paths:
@@ -482,15 +488,6 @@ class FilePaths:
pass
return False
-class FilePath(FilePaths):
- """
- FilePath is a specialization of FilePaths that takes a single filename.
- """
- def __init__(self, name, base_dir=test_dir):
- super(FilePath, self).__init__(name, base_dir=base_dir)
-
- def __enter__(self):
- return self.paths[0]
def file_path_remover():
for path in reversed(file_path_remover.paths):
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 05/22] qemu-iotests: Simplify FilePath __init__
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (3 preceding siblings ...)
2020-09-15 10:46 ` [PULL 04/22] qemu-iotests: Merge FilePaths and FilePath Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 06/22] block/quorum.c: stable children names Max Reitz
` (17 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Nir Soffer <nirsof@gmail.com>
Use list comprehension instead of append loop.
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20200828232152.205833-6-nsoffer@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/iotests.py | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1799338f8b..91e4a57126 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -470,9 +470,8 @@ class FilePath:
"""
def __init__(self, *names, base_dir=test_dir):
- self.paths = []
- for name in names:
- self.paths.append(os.path.join(base_dir, file_pattern(name)))
+ self.paths = [os.path.join(base_dir, file_pattern(name))
+ for name in names]
def __enter__(self):
if len(self.paths) == 1:
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 06/22] block/quorum.c: stable children names
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (4 preceding siblings ...)
2020-09-15 10:46 ` [PULL 05/22] qemu-iotests: Simplify FilePath __init__ Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 07/22] qemu-img: avoid unaligned read requests during convert Max Reitz
` (16 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Lukas Straub <lukasstraub2@web.de>
If we remove the child with the highest index from the quorum,
decrement s->next_child_index. This way we get stable children
names as long as we only remove the last child.
Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Fixes: https://bugs.launchpad.net/bugs/1881231
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-Id: <5d5f930424c1c770754041aa8ad6421dc4e2b58e.1596536719.git.lukasstraub2@web.de>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/quorum.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/block/quorum.c b/block/quorum.c
index 6df9449fc2..e846a7e892 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -29,6 +29,8 @@
#define HASH_LENGTH 32
+#define INDEXSTR_LEN 32
+
#define QUORUM_OPT_VOTE_THRESHOLD "vote-threshold"
#define QUORUM_OPT_BLKVERIFY "blkverify"
#define QUORUM_OPT_REWRITE "rewrite-corrupted"
@@ -970,9 +972,9 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
opened = g_new0(bool, s->num_children);
for (i = 0; i < s->num_children; i++) {
- char indexstr[32];
- ret = snprintf(indexstr, 32, "children.%d", i);
- assert(ret < 32);
+ char indexstr[INDEXSTR_LEN];
+ ret = snprintf(indexstr, INDEXSTR_LEN, "children.%d", i);
+ assert(ret < INDEXSTR_LEN);
s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
&child_of_bds, BDRV_CHILD_DATA, false,
@@ -1024,7 +1026,7 @@ static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
{
BDRVQuorumState *s = bs->opaque;
BdrvChild *child;
- char indexstr[32];
+ char indexstr[INDEXSTR_LEN];
int ret;
if (s->is_blkverify) {
@@ -1039,8 +1041,8 @@ static void quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs,
return;
}
- ret = snprintf(indexstr, 32, "children.%u", s->next_child_index);
- if (ret < 0 || ret >= 32) {
+ ret = snprintf(indexstr, INDEXSTR_LEN, "children.%u", s->next_child_index);
+ if (ret < 0 || ret >= INDEXSTR_LEN) {
error_setg(errp, "cannot generate child name");
return;
}
@@ -1068,6 +1070,7 @@ static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
Error **errp)
{
BDRVQuorumState *s = bs->opaque;
+ char indexstr[INDEXSTR_LEN];
int i;
for (i = 0; i < s->num_children; i++) {
@@ -1089,6 +1092,11 @@ static void quorum_del_child(BlockDriverState *bs, BdrvChild *child,
/* We know now that num_children > threshold, so blkverify must be false */
assert(!s->is_blkverify);
+ snprintf(indexstr, INDEXSTR_LEN, "children.%u", s->next_child_index - 1);
+ if (!strncmp(child->name, indexstr, INDEXSTR_LEN)) {
+ s->next_child_index--;
+ }
+
bdrv_drained_begin(bs);
/* We can safely remove this child now */
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 07/22] qemu-img: avoid unaligned read requests during convert
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (5 preceding siblings ...)
2020-09-15 10:46 ` [PULL 06/22] block/quorum.c: stable children names Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 08/22] qcow2: Use macros for the L1, refcount and bitmap table entry sizes Max Reitz
` (15 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Peter Lieven <pl@kamp.de>
in case of large continous areas that share the same allocation status
it happens that the value of s->sector_next_status is unaligned to the
cluster size or even request alignment of the source. Avoid this by
stripping down the s->sector_next_status position to cluster boundaries.
Signed-off-by: Peter Lieven <pl@kamp.de>
Message-Id: <20200901125129.6398-1-pl@kamp.de>
[mreitz: Disable vhdx for 251]
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qemu-img.c | 22 ++++++++++++++++++++++
tests/qemu-iotests/251 | 7 +++++--
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index d0b1c97562..37365d06fa 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1666,6 +1666,7 @@ enum ImgConvertBlockStatus {
typedef struct ImgConvertState {
BlockBackend **src;
int64_t *src_sectors;
+ int *src_alignment;
int src_num;
int64_t total_sectors;
int64_t allocated_sectors;
@@ -1732,6 +1733,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
if (s->sector_next_status <= sector_num) {
uint64_t offset = (sector_num - src_cur_offset) * BDRV_SECTOR_SIZE;
int64_t count;
+ int tail;
BlockDriverState *src_bs = blk_bs(s->src[src_cur]);
BlockDriverState *base;
@@ -1772,6 +1774,16 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
+ /*
+ * Avoid that s->sector_next_status becomes unaligned to the source
+ * request alignment and/or cluster size to avoid unnecessary read
+ * cycles.
+ */
+ tail = (sector_num - src_cur_offset + n) % s->src_alignment[src_cur];
+ if (n > tail) {
+ n -= tail;
+ }
+
if (ret & BDRV_BLOCK_ZERO) {
s->status = post_backing_zero ? BLK_BACKING_FILE : BLK_ZERO;
} else if (ret & BDRV_BLOCK_DATA) {
@@ -2410,8 +2422,10 @@ static int img_convert(int argc, char **argv)
s.src = g_new0(BlockBackend *, s.src_num);
s.src_sectors = g_new(int64_t, s.src_num);
+ s.src_alignment = g_new(int, s.src_num);
for (bs_i = 0; bs_i < s.src_num; bs_i++) {
+ BlockDriverState *src_bs;
s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
fmt, src_flags, src_writethrough, s.quiet,
force_share);
@@ -2426,6 +2440,13 @@ static int img_convert(int argc, char **argv)
ret = -1;
goto out;
}
+ src_bs = blk_bs(s.src[bs_i]);
+ s.src_alignment[bs_i] = DIV_ROUND_UP(src_bs->bl.request_alignment,
+ BDRV_SECTOR_SIZE);
+ if (!bdrv_get_info(src_bs, &bdi)) {
+ s.src_alignment[bs_i] = MAX(s.src_alignment[bs_i],
+ bdi.cluster_size / BDRV_SECTOR_SIZE);
+ }
s.total_sectors += s.src_sectors[bs_i];
}
@@ -2708,6 +2729,7 @@ out:
g_free(s.src);
}
g_free(s.src_sectors);
+ g_free(s.src_alignment);
fail_getopt:
g_free(options);
diff --git a/tests/qemu-iotests/251 b/tests/qemu-iotests/251
index 7918ba3559..294773bdc1 100755
--- a/tests/qemu-iotests/251
+++ b/tests/qemu-iotests/251
@@ -46,8 +46,11 @@ if [ "$IMGOPTSSYNTAX" = "true" ]; then
# We use json:{} filenames here, so we cannot work with additional options.
_unsupported_fmt $IMGFMT
else
- # With VDI, the output is ordered differently. Just disable it.
- _unsupported_fmt vdi
+ # - With VDI, the output is ordered differently. Just disable it.
+ # - VHDX has large clusters; because qemu-img convert tries to
+ # align the requests to the cluster size, the output is ordered
+ # differently, so disable it, too.
+ _unsupported_fmt vdi vhdx
fi
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 08/22] qcow2: Use macros for the L1, refcount and bitmap table entry sizes
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (6 preceding siblings ...)
2020-09-15 10:46 ` [PULL 07/22] qemu-img: avoid unaligned read requests during convert Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 09/22] qcow2: Fix removal of list members from BDRVQcow2State.cluster_allocs Max Reitz
` (14 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This patch replaces instances of sizeof(uint64_t) in the qcow2 driver
with macros that indicate what those sizes are actually referring to.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <20200828110828.13833-1-berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.h | 6 +++
block/qcow2-bitmap.c | 11 ++++--
block/qcow2-cluster.c | 24 ++++++------
block/qcow2-refcount.c | 89 ++++++++++++++++++++++--------------------
block/qcow2-snapshot.c | 20 +++++-----
block/qcow2.c | 27 ++++++-------
6 files changed, 94 insertions(+), 83 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 065ec3df0b..83cfa0c391 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -99,6 +99,12 @@
#define L2E_SIZE_NORMAL (sizeof(uint64_t))
#define L2E_SIZE_EXTENDED (sizeof(uint64_t) * 2)
+/* Size of L1 table entries */
+#define L1E_SIZE (sizeof(uint64_t))
+
+/* Size of reftable entries */
+#define REFTABLE_ENTRY_SIZE (sizeof(uint64_t))
+
#define MIN_CLUSTER_BITS 9
#define MAX_CLUSTER_BITS 21
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 8c34b2aef7..d7a31a8ddc 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -42,6 +42,9 @@
#define BME_MIN_GRANULARITY_BITS 9
#define BME_MAX_NAME_SIZE 1023
+/* Size of bitmap table entries */
+#define BME_TABLE_ENTRY_SIZE (sizeof(uint64_t))
+
QEMU_BUILD_BUG_ON(BME_MAX_NAME_SIZE != BDRV_BITMAP_MAX_NAME_SIZE);
#if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
@@ -232,7 +235,7 @@ static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb,
assert(tb->size <= BME_MAX_TABLE_SIZE);
ret = bdrv_pread(bs->file, tb->offset,
- table, tb->size * sizeof(uint64_t));
+ table, tb->size * BME_TABLE_ENTRY_SIZE);
if (ret < 0) {
goto fail;
}
@@ -265,7 +268,7 @@ static int free_bitmap_clusters(BlockDriverState *bs, Qcow2BitmapTable *tb)
}
clear_bitmap_table(bs, bitmap_table, tb->size);
- qcow2_free_clusters(bs, tb->offset, tb->size * sizeof(uint64_t),
+ qcow2_free_clusters(bs, tb->offset, tb->size * BME_TABLE_ENTRY_SIZE,
QCOW2_DISCARD_OTHER);
g_free(bitmap_table);
@@ -690,7 +693,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
ret = qcow2_inc_refcounts_imrt(bs, res,
refcount_table, refcount_table_size,
bm->table.offset,
- bm->table.size * sizeof(uint64_t));
+ bm->table.size * BME_TABLE_ENTRY_SIZE);
if (ret < 0) {
goto out;
}
@@ -1797,7 +1800,7 @@ uint64_t qcow2_get_persistent_dirty_bitmap_size(BlockDriverState *in_bs,
/* Assume the entire bitmap is allocated */
bitmaps_size += bmclusters * cluster_size;
/* Also reserve space for the bitmap table entries */
- bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
+ bitmaps_size += ROUND_UP(bmclusters * BME_TABLE_ENTRY_SIZE,
cluster_size);
/* And space for contribution to bitmap directory size */
bitmap_dir_size += calc_dir_entry_size(strlen(name), 0);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index fcdf7af8e6..4530e5e471 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -47,8 +47,8 @@ int qcow2_shrink_l1_table(BlockDriverState *bs, uint64_t exact_size)
BLKDBG_EVENT(bs->file, BLKDBG_L1_SHRINK_WRITE_TABLE);
ret = bdrv_pwrite_zeroes(bs->file, s->l1_table_offset +
- new_l1_size * sizeof(uint64_t),
- (s->l1_size - new_l1_size) * sizeof(uint64_t), 0);
+ new_l1_size * L1E_SIZE,
+ (s->l1_size - new_l1_size) * L1E_SIZE, 0);
if (ret < 0) {
goto fail;
}
@@ -76,7 +76,7 @@ fail:
* l1_table in memory to avoid possible image corruption.
*/
memset(s->l1_table + new_l1_size, 0,
- (s->l1_size - new_l1_size) * sizeof(uint64_t));
+ (s->l1_size - new_l1_size) * L1E_SIZE);
return ret;
}
@@ -96,7 +96,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
/* Do a sanity check on min_size before trying to calculate new_l1_size
* (this prevents overflows during the while loop for the calculation of
* new_l1_size) */
- if (min_size > INT_MAX / sizeof(uint64_t)) {
+ if (min_size > INT_MAX / L1E_SIZE) {
return -EFBIG;
}
@@ -114,7 +114,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
}
QEMU_BUILD_BUG_ON(QCOW_MAX_L1_SIZE > INT_MAX);
- if (new_l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
+ if (new_l1_size > QCOW_MAX_L1_SIZE / L1E_SIZE) {
return -EFBIG;
}
@@ -123,7 +123,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
s->l1_size, new_l1_size);
#endif
- new_l1_size2 = sizeof(uint64_t) * new_l1_size;
+ new_l1_size2 = L1E_SIZE * new_l1_size;
new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_size2);
if (new_l1_table == NULL) {
return -ENOMEM;
@@ -131,7 +131,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
memset(new_l1_table, 0, new_l1_size2);
if (s->l1_size) {
- memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t));
+ memcpy(new_l1_table, s->l1_table, s->l1_size * L1E_SIZE);
}
/* write new table (align to cluster) */
@@ -180,7 +180,7 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
s->l1_table = new_l1_table;
old_l1_size = s->l1_size;
s->l1_size = new_l1_size;
- qcow2_free_clusters(bs, old_l1_table_offset, old_l1_size * sizeof(uint64_t),
+ qcow2_free_clusters(bs, old_l1_table_offset, old_l1_size * L1E_SIZE,
QCOW2_DISCARD_OTHER);
return 0;
fail:
@@ -225,9 +225,9 @@ int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
BDRVQcow2State *s = bs->opaque;
int l1_start_index;
int i, ret;
- int bufsize = MAX(sizeof(uint64_t),
+ int bufsize = MAX(L1E_SIZE,
MIN(bs->file->bs->bl.request_alignment, s->cluster_size));
- int nentries = bufsize / sizeof(uint64_t);
+ int nentries = bufsize / L1E_SIZE;
g_autofree uint64_t *buf = g_try_new0(uint64_t, nentries);
if (buf == NULL) {
@@ -2410,7 +2410,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
Error *local_err = NULL;
ret = qcow2_validate_table(bs, s->snapshots[i].l1_table_offset,
- s->snapshots[i].l1_size, sizeof(uint64_t),
+ s->snapshots[i].l1_size, L1E_SIZE,
QCOW_MAX_L1_SIZE, "Snapshot L1 table",
&local_err);
if (ret < 0) {
@@ -2418,7 +2418,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
goto fail;
}
- l1_size2 = s->snapshots[i].l1_size * sizeof(uint64_t);
+ l1_size2 = s->snapshots[i].l1_size * L1E_SIZE;
new_l1_table = g_try_realloc(l1_table, l1_size2);
if (!new_l1_table) {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index aae52607eb..51e9778ed8 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -105,8 +105,8 @@ int qcow2_refcount_init(BlockDriverState *bs)
s->get_refcount = get_refcount_funcs[s->refcount_order];
s->set_refcount = set_refcount_funcs[s->refcount_order];
- assert(s->refcount_table_size <= INT_MAX / sizeof(uint64_t));
- refcount_table_size2 = s->refcount_table_size * sizeof(uint64_t);
+ assert(s->refcount_table_size <= INT_MAX / REFTABLE_ENTRY_SIZE);
+ refcount_table_size2 = s->refcount_table_size * REFTABLE_ENTRY_SIZE;
s->refcount_table = g_try_malloc(refcount_table_size2);
if (s->refcount_table_size > 0) {
@@ -434,8 +434,8 @@ static int alloc_refcount_block(BlockDriverState *bs,
if (refcount_table_index < s->refcount_table_size) {
uint64_t data64 = cpu_to_be64(new_block);
BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_HOOKUP);
- ret = bdrv_pwrite_sync(bs->file,
- s->refcount_table_offset + refcount_table_index * sizeof(uint64_t),
+ ret = bdrv_pwrite_sync(bs->file, s->refcount_table_offset +
+ refcount_table_index * REFTABLE_ENTRY_SIZE,
&data64, sizeof(data64));
if (ret < 0) {
goto fail;
@@ -562,8 +562,8 @@ int64_t qcow2_refcount_area(BlockDriverState *bs, uint64_t start_offset,
DIV_ROUND_UP(total_refblock_count, 2);
}
/* The qcow2 file can only store the reftable size in number of clusters */
- table_size = ROUND_UP(table_size, s->cluster_size / sizeof(uint64_t));
- table_clusters = (table_size * sizeof(uint64_t)) / s->cluster_size;
+ table_size = ROUND_UP(table_size, s->cluster_size / REFTABLE_ENTRY_SIZE);
+ table_clusters = (table_size * REFTABLE_ENTRY_SIZE) / s->cluster_size;
if (table_size > QCOW_MAX_REFTABLE_SIZE) {
return -EFBIG;
@@ -581,13 +581,13 @@ int64_t qcow2_refcount_area(BlockDriverState *bs, uint64_t start_offset,
if (table_size > s->max_refcount_table_index) {
/* We're actually growing the reftable */
memcpy(new_table, s->refcount_table,
- (s->max_refcount_table_index + 1) * sizeof(uint64_t));
+ (s->max_refcount_table_index + 1) * REFTABLE_ENTRY_SIZE);
} else {
/* Improbable case: We're shrinking the reftable. However, the caller
* has assured us that there is only empty space beyond @start_offset,
* so we can simply drop all of the refblocks that won't fit into the
* new reftable. */
- memcpy(new_table, s->refcount_table, table_size * sizeof(uint64_t));
+ memcpy(new_table, s->refcount_table, table_size * REFTABLE_ENTRY_SIZE);
}
if (new_refblock_offset) {
@@ -682,7 +682,7 @@ int64_t qcow2_refcount_area(BlockDriverState *bs, uint64_t start_offset,
BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_ALLOC_WRITE_TABLE);
ret = bdrv_pwrite_sync(bs->file, table_offset, new_table,
- table_size * sizeof(uint64_t));
+ table_size * REFTABLE_ENTRY_SIZE);
if (ret < 0) {
goto fail;
}
@@ -717,7 +717,8 @@ int64_t qcow2_refcount_area(BlockDriverState *bs, uint64_t start_offset,
update_max_refcount_table_index(s);
/* Free old table. */
- qcow2_free_clusters(bs, old_table_offset, old_table_size * sizeof(uint64_t),
+ qcow2_free_clusters(bs, old_table_offset,
+ old_table_size * REFTABLE_ENTRY_SIZE,
QCOW2_DISCARD_OTHER);
return end_offset;
@@ -1253,7 +1254,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
l2_slice = NULL;
l1_table = NULL;
- l1_size2 = l1_size * sizeof(uint64_t);
+ l1_size2 = l1_size * L1E_SIZE;
slice_size2 = s->l2_slice_size * l2_entry_size(s);
n_slices = s->cluster_size / slice_size2;
@@ -1784,7 +1785,7 @@ static int check_refcounts_l1(BlockDriverState *bs,
uint64_t *l1_table = NULL, l2_offset, l1_size2;
int i, ret;
- l1_size2 = l1_size * sizeof(uint64_t);
+ l1_size2 = l1_size * L1E_SIZE;
/* Mark L1 table as used */
ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, refcount_table_size,
@@ -2146,7 +2147,7 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
res->corruptions++;
continue;
}
- if (sn->l1_size > QCOW_MAX_L1_SIZE / sizeof(uint64_t)) {
+ if (sn->l1_size > QCOW_MAX_L1_SIZE / L1E_SIZE) {
fprintf(stderr, "ERROR snapshot %s (%s) l1_size=%#" PRIx32 ": "
"L1 table is too large; snapshot table entry corrupted\n",
sn->id_str, sn->name, sn->l1_size);
@@ -2169,7 +2170,8 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
/* refcount data */
ret = qcow2_inc_refcounts_imrt(bs, res, refcount_table, nb_clusters,
s->refcount_table_offset,
- s->refcount_table_size * sizeof(uint64_t));
+ s->refcount_table_size *
+ REFTABLE_ENTRY_SIZE);
if (ret < 0) {
return ret;
}
@@ -2390,11 +2392,11 @@ write_refblocks:
uint32_t old_reftable_size = reftable_size;
uint64_t *new_on_disk_reftable;
- reftable_size = ROUND_UP((refblock_index + 1) * sizeof(uint64_t),
- s->cluster_size) / sizeof(uint64_t);
+ reftable_size = ROUND_UP((refblock_index + 1) * REFTABLE_ENTRY_SIZE,
+ s->cluster_size) / REFTABLE_ENTRY_SIZE;
new_on_disk_reftable = g_try_realloc(on_disk_reftable,
reftable_size *
- sizeof(uint64_t));
+ REFTABLE_ENTRY_SIZE);
if (!new_on_disk_reftable) {
res->check_errors++;
ret = -ENOMEM;
@@ -2403,7 +2405,7 @@ write_refblocks:
on_disk_reftable = new_on_disk_reftable;
memset(on_disk_reftable + old_reftable_size, 0,
- (reftable_size - old_reftable_size) * sizeof(uint64_t));
+ (reftable_size - old_reftable_size) * REFTABLE_ENTRY_SIZE);
/* The offset we have for the reftable is now no longer valid;
* this will leak that range, but we can easily fix that by running
@@ -2420,7 +2422,7 @@ write_refblocks:
reftable_offset < 0)
{
uint64_t reftable_clusters = size_to_clusters(s, reftable_size *
- sizeof(uint64_t));
+ REFTABLE_ENTRY_SIZE);
reftable_offset = alloc_clusters_imrt(bs, reftable_clusters,
refcount_table, nb_clusters,
&first_free_cluster);
@@ -2460,8 +2462,8 @@ write_refblocks:
uint64_t post_refblock_start, reftable_clusters;
post_refblock_start = ROUND_UP(*nb_clusters, s->refcount_block_size);
- reftable_clusters = size_to_clusters(s,
- reftable_size * sizeof(uint64_t));
+ reftable_clusters =
+ size_to_clusters(s, reftable_size * REFTABLE_ENTRY_SIZE);
/* Not pretty but simple */
if (first_free_cluster < post_refblock_start) {
first_free_cluster = post_refblock_start;
@@ -2485,16 +2487,16 @@ write_refblocks:
}
ret = qcow2_pre_write_overlap_check(bs, 0, reftable_offset,
- reftable_size * sizeof(uint64_t),
+ reftable_size * REFTABLE_ENTRY_SIZE,
false);
if (ret < 0) {
fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret));
goto fail;
}
- assert(reftable_size < INT_MAX / sizeof(uint64_t));
+ assert(reftable_size < INT_MAX / REFTABLE_ENTRY_SIZE);
ret = bdrv_pwrite(bs->file, reftable_offset, on_disk_reftable,
- reftable_size * sizeof(uint64_t));
+ reftable_size * REFTABLE_ENTRY_SIZE);
if (ret < 0) {
fprintf(stderr, "ERROR writing reftable: %s\n", strerror(-ret));
goto fail;
@@ -2503,7 +2505,7 @@ write_refblocks:
/* Enter new reftable into the image header */
reftable_offset_and_clusters.reftable_offset = cpu_to_be64(reftable_offset);
reftable_offset_and_clusters.reftable_clusters =
- cpu_to_be32(size_to_clusters(s, reftable_size * sizeof(uint64_t)));
+ cpu_to_be32(size_to_clusters(s, reftable_size * REFTABLE_ENTRY_SIZE));
ret = bdrv_pwrite_sync(bs->file,
offsetof(QCowHeader, refcount_table_offset),
&reftable_offset_and_clusters,
@@ -2693,14 +2695,14 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
offset = start_of_cluster(s, offset);
if ((chk & QCOW2_OL_ACTIVE_L1) && s->l1_size) {
- if (overlaps_with(s->l1_table_offset, s->l1_size * sizeof(uint64_t))) {
+ if (overlaps_with(s->l1_table_offset, s->l1_size * L1E_SIZE)) {
return QCOW2_OL_ACTIVE_L1;
}
}
if ((chk & QCOW2_OL_REFCOUNT_TABLE) && s->refcount_table_size) {
if (overlaps_with(s->refcount_table_offset,
- s->refcount_table_size * sizeof(uint64_t))) {
+ s->refcount_table_size * REFTABLE_ENTRY_SIZE)) {
return QCOW2_OL_REFCOUNT_TABLE;
}
}
@@ -2715,7 +2717,7 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
for (i = 0; i < s->nb_snapshots; i++) {
if (s->snapshots[i].l1_size &&
overlaps_with(s->snapshots[i].l1_table_offset,
- s->snapshots[i].l1_size * sizeof(uint64_t))) {
+ s->snapshots[i].l1_size * L1E_SIZE)) {
return QCOW2_OL_INACTIVE_L1;
}
}
@@ -2749,11 +2751,11 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
for (i = 0; i < s->nb_snapshots; i++) {
uint64_t l1_ofs = s->snapshots[i].l1_table_offset;
uint32_t l1_sz = s->snapshots[i].l1_size;
- uint64_t l1_sz2 = l1_sz * sizeof(uint64_t);
+ uint64_t l1_sz2 = l1_sz * L1E_SIZE;
uint64_t *l1;
int ret;
- ret = qcow2_validate_table(bs, l1_ofs, l1_sz, sizeof(uint64_t),
+ ret = qcow2_validate_table(bs, l1_ofs, l1_sz, L1E_SIZE,
QCOW_MAX_L1_SIZE, "", NULL);
if (ret < 0) {
return ret;
@@ -2877,8 +2879,8 @@ static int alloc_refblock(BlockDriverState *bs, uint64_t **reftable,
uint64_t new_reftable_size;
new_reftable_size = ROUND_UP(reftable_index + 1,
- s->cluster_size / sizeof(uint64_t));
- if (new_reftable_size > QCOW_MAX_REFTABLE_SIZE / sizeof(uint64_t)) {
+ s->cluster_size / REFTABLE_ENTRY_SIZE);
+ if (new_reftable_size > QCOW_MAX_REFTABLE_SIZE / REFTABLE_ENTRY_SIZE) {
error_setg(errp,
"This operation would make the refcount table grow "
"beyond the maximum size supported by QEMU, aborting");
@@ -2886,14 +2888,14 @@ static int alloc_refblock(BlockDriverState *bs, uint64_t **reftable,
}
new_reftable = g_try_realloc(*reftable, new_reftable_size *
- sizeof(uint64_t));
+ REFTABLE_ENTRY_SIZE);
if (!new_reftable) {
error_setg(errp, "Failed to increase reftable buffer size");
return -ENOMEM;
}
memset(new_reftable + *reftable_size, 0,
- (new_reftable_size - *reftable_size) * sizeof(uint64_t));
+ (new_reftable_size - *reftable_size) * REFTABLE_ENTRY_SIZE);
*reftable = new_reftable;
*reftable_size = new_reftable_size;
@@ -3164,13 +3166,14 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
if (new_allocation) {
if (new_reftable_offset) {
- qcow2_free_clusters(bs, new_reftable_offset,
- allocated_reftable_size * sizeof(uint64_t),
- QCOW2_DISCARD_NEVER);
+ qcow2_free_clusters(
+ bs, new_reftable_offset,
+ allocated_reftable_size * REFTABLE_ENTRY_SIZE,
+ QCOW2_DISCARD_NEVER);
}
new_reftable_offset = qcow2_alloc_clusters(bs, new_reftable_size *
- sizeof(uint64_t));
+ REFTABLE_ENTRY_SIZE);
if (new_reftable_offset < 0) {
error_setg_errno(errp, -new_reftable_offset,
"Failed to allocate the new reftable");
@@ -3196,7 +3199,7 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
/* Write the new reftable */
ret = qcow2_pre_write_overlap_check(bs, 0, new_reftable_offset,
- new_reftable_size * sizeof(uint64_t),
+ new_reftable_size * REFTABLE_ENTRY_SIZE,
false);
if (ret < 0) {
error_setg_errno(errp, -ret, "Overlap check failed");
@@ -3208,7 +3211,7 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
}
ret = bdrv_pwrite(bs->file, new_reftable_offset, new_reftable,
- new_reftable_size * sizeof(uint64_t));
+ new_reftable_size * REFTABLE_ENTRY_SIZE);
for (i = 0; i < new_reftable_size; i++) {
be64_to_cpus(&new_reftable[i]);
@@ -3285,7 +3288,7 @@ done:
if (new_reftable_offset > 0) {
qcow2_free_clusters(bs, new_reftable_offset,
- new_reftable_size * sizeof(uint64_t),
+ new_reftable_size * REFTABLE_ENTRY_SIZE,
QCOW2_DISCARD_OTHER);
}
}
@@ -3374,7 +3377,7 @@ int qcow2_shrink_reftable(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
uint64_t *reftable_tmp =
- g_malloc(s->refcount_table_size * sizeof(uint64_t));
+ g_malloc(s->refcount_table_size * REFTABLE_ENTRY_SIZE);
int i, ret;
for (i = 0; i < s->refcount_table_size; i++) {
@@ -3412,7 +3415,7 @@ int qcow2_shrink_reftable(BlockDriverState *bs)
}
ret = bdrv_pwrite_sync(bs->file, s->refcount_table_offset, reftable_tmp,
- s->refcount_table_size * sizeof(uint64_t));
+ s->refcount_table_size * REFTABLE_ENTRY_SIZE);
/*
* If the write in the reftable failed the image may contain a partially
* overwritten reftable. In this case it would be better to clear the
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 2756b37d24..9b68690f56 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -659,7 +659,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
sn->extra_data_size = sizeof(QCowSnapshotExtraData);
/* Allocate the L1 table of the snapshot and copy the current one there. */
- l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
+ l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * L1E_SIZE);
if (l1_table_offset < 0) {
ret = l1_table_offset;
goto fail;
@@ -679,13 +679,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
}
ret = qcow2_pre_write_overlap_check(bs, 0, sn->l1_table_offset,
- s->l1_size * sizeof(uint64_t), false);
+ s->l1_size * L1E_SIZE, false);
if (ret < 0) {
goto fail;
}
ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
- s->l1_size * sizeof(uint64_t));
+ s->l1_size * L1E_SIZE);
if (ret < 0) {
goto fail;
}
@@ -768,7 +768,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
sn = &s->snapshots[snapshot_index];
ret = qcow2_validate_table(bs, sn->l1_table_offset, sn->l1_size,
- sizeof(uint64_t), QCOW_MAX_L1_SIZE,
+ L1E_SIZE, QCOW_MAX_L1_SIZE,
"Snapshot L1 table", &local_err);
if (ret < 0) {
error_report_err(local_err);
@@ -803,8 +803,8 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
goto fail;
}
- cur_l1_bytes = s->l1_size * sizeof(uint64_t);
- sn_l1_bytes = sn->l1_size * sizeof(uint64_t);
+ cur_l1_bytes = s->l1_size * L1E_SIZE;
+ sn_l1_bytes = sn->l1_size * L1E_SIZE;
/*
* Copy the snapshot L1 table to the current L1 table.
@@ -917,7 +917,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
sn = s->snapshots[snapshot_index];
ret = qcow2_validate_table(bs, sn.l1_table_offset, sn.l1_size,
- sizeof(uint64_t), QCOW_MAX_L1_SIZE,
+ L1E_SIZE, QCOW_MAX_L1_SIZE,
"Snapshot L1 table", errp);
if (ret < 0) {
return ret;
@@ -953,7 +953,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
error_setg_errno(errp, -ret, "Failed to free the cluster and L1 table");
return ret;
}
- qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t),
+ qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * L1E_SIZE,
QCOW2_DISCARD_SNAPSHOT);
/* must update the copied flag on the current cluster offsets */
@@ -1030,12 +1030,12 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs,
/* Allocate and read in the snapshot's L1 table */
ret = qcow2_validate_table(bs, sn->l1_table_offset, sn->l1_size,
- sizeof(uint64_t), QCOW_MAX_L1_SIZE,
+ L1E_SIZE, QCOW_MAX_L1_SIZE,
"Snapshot L1 table", errp);
if (ret < 0) {
return ret;
}
- new_l1_bytes = sn->l1_size * sizeof(uint64_t);
+ new_l1_bytes = sn->l1_size * L1E_SIZE;
new_l1_table = qemu_try_blockalign(bs->file->bs, new_l1_bytes);
if (new_l1_table == NULL) {
return -ENOMEM;
diff --git a/block/qcow2.c b/block/qcow2.c
index da56b1a4df..ef9a45e82f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1543,7 +1543,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* read the level 1 table */
ret = qcow2_validate_table(bs, header.l1_table_offset,
- header.l1_size, sizeof(uint64_t),
+ header.l1_size, L1E_SIZE,
QCOW_MAX_L1_SIZE, "Active L1 table", errp);
if (ret < 0) {
goto fail;
@@ -1568,15 +1568,14 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
}
if (s->l1_size > 0) {
- s->l1_table = qemu_try_blockalign(bs->file->bs,
- s->l1_size * sizeof(uint64_t));
+ s->l1_table = qemu_try_blockalign(bs->file->bs, s->l1_size * L1E_SIZE);
if (s->l1_table == NULL) {
error_setg(errp, "Could not allocate L1 table");
ret = -ENOMEM;
goto fail;
}
ret = bdrv_pread(bs->file, s->l1_table_offset, s->l1_table,
- s->l1_size * sizeof(uint64_t));
+ s->l1_size * L1E_SIZE);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not read L1 table");
goto fail;
@@ -3213,7 +3212,7 @@ int64_t qcow2_refcount_metadata_size(int64_t clusters, size_t cluster_size,
* where no further refcount blocks or table clusters are required to
* reference count every cluster.
*/
- int64_t blocks_per_table_cluster = cluster_size / sizeof(uint64_t);
+ int64_t blocks_per_table_cluster = cluster_size / REFTABLE_ENTRY_SIZE;
int64_t refcounts_per_block = cluster_size * 8 / (1 << refcount_order);
int64_t table = 0; /* number of refcount table clusters */
int64_t blocks = 0; /* number of refcount block clusters */
@@ -3270,8 +3269,8 @@ static int64_t qcow2_calc_prealloc_size(int64_t total_size,
/* total size of L1 tables */
nl1e = nl2e * l2e_size / cluster_size;
- nl1e = ROUND_UP(nl1e, cluster_size / sizeof(uint64_t));
- meta_size += nl1e * sizeof(uint64_t);
+ nl1e = ROUND_UP(nl1e, cluster_size / L1E_SIZE);
+ meta_size += nl1e * L1E_SIZE;
/* total size of refcount table and blocks */
meta_size += qcow2_refcount_metadata_size(
@@ -4460,7 +4459,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
/* write updated header.size */
offset = cpu_to_be64(offset);
ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
- &offset, sizeof(uint64_t));
+ &offset, sizeof(offset));
if (ret < 0) {
error_setg_errno(errp, -ret, "Failed to update the image size");
goto fail;
@@ -4700,8 +4699,8 @@ static int make_completely_empty(BlockDriverState *bs)
BLKDBG_EVENT(bs->file, BLKDBG_L1_UPDATE);
- l1_clusters = DIV_ROUND_UP(s->l1_size, s->cluster_size / sizeof(uint64_t));
- l1_size2 = (uint64_t)s->l1_size * sizeof(uint64_t);
+ l1_clusters = DIV_ROUND_UP(s->l1_size, s->cluster_size / L1E_SIZE);
+ l1_size2 = (uint64_t)s->l1_size * L1E_SIZE;
/* After this call, neither the in-memory nor the on-disk refcount
* information accurately describe the actual references */
@@ -4747,14 +4746,14 @@ static int make_completely_empty(BlockDriverState *bs)
s->l1_table_offset = 3 * s->cluster_size;
- new_reftable = g_try_new0(uint64_t, s->cluster_size / sizeof(uint64_t));
+ new_reftable = g_try_new0(uint64_t, s->cluster_size / REFTABLE_ENTRY_SIZE);
if (!new_reftable) {
ret = -ENOMEM;
goto fail_broken_refcounts;
}
s->refcount_table_offset = s->cluster_size;
- s->refcount_table_size = s->cluster_size / sizeof(uint64_t);
+ s->refcount_table_size = s->cluster_size / REFTABLE_ENTRY_SIZE;
s->max_refcount_table_index = 0;
g_free(s->refcount_table);
@@ -4826,7 +4825,7 @@ static int qcow2_make_empty(BlockDriverState *bs)
int step = QEMU_ALIGN_DOWN(INT_MAX, s->cluster_size);
int l1_clusters, ret = 0;
- l1_clusters = DIV_ROUND_UP(s->l1_size, s->cluster_size / sizeof(uint64_t));
+ l1_clusters = DIV_ROUND_UP(s->l1_size, s->cluster_size / L1E_SIZE);
if (s->qcow_version >= 3 && !s->snapshots && !s->nb_bitmaps &&
3 + l1_clusters <= s->refcount_block_size &&
@@ -4957,7 +4956,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
l2e_size = extended_l2 ? L2E_SIZE_EXTENDED : L2E_SIZE_NORMAL;
l2_tables = DIV_ROUND_UP(virtual_size / cluster_size,
cluster_size / l2e_size);
- if (l2_tables * sizeof(uint64_t) > QCOW_MAX_L1_SIZE) {
+ if (l2_tables * L1E_SIZE > QCOW_MAX_L1_SIZE) {
error_setg(&local_err, "The image size is too large "
"(try using a larger cluster size)");
goto err;
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 09/22] qcow2: Fix removal of list members from BDRVQcow2State.cluster_allocs
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (7 preceding siblings ...)
2020-09-15 10:46 ` [PULL 08/22] qcow2: Use macros for the L1, refcount and bitmap table entry sizes Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 10/22] qcow2: Don't check nb_clusters when removing l2meta from the list Max Reitz
` (13 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
When a write request needs to allocate new clusters (or change the L2
bitmap of existing ones) a QCowL2Meta structure is created so the L2
metadata can be later updated and any copy-on-write can be performed
if necessary.
A write request can span a region consisting of an arbitrary
combination of previously unallocated and allocated clusters, and if
the unallocated ones can be put contiguous to the existing ones then
QEMU will do so in order to minimize the number of write operations.
In practice this means that a write request has not just one but a
number of QCowL2Meta structures. All of them are added to the
cluster_allocs list that is stored in BDRVQcow2State and is used to
detect overlapping requests. After the write request finishes all its
associated QCowL2Meta are removed from that list. calculate_l2_meta()
takes care of creating and putting those structures in the list, and
qcow2_handle_l2meta() takes care of removing them.
The problem is that the error path in handle_alloc() also tries to
remove an item in that list, a remnant from the time when this was
handled there (that code would not even be correct anymore because
it only removes one struct and not all the ones from the same write
request).
This can trigger a double removal of the same item from the list,
causing a crash. This is not easy to reproduce in practice because
it requires that do_alloc_cluster_offset() fails after a successful
previous allocation during the same write request, but it can be
reproduced with the included test case.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <3440a1c4d53c4fe48312b478c96accb338cbef7c.1599150873.git.berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-cluster.c | 3 --
tests/qemu-iotests/305 | 74 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/305.out | 16 +++++++++
tests/qemu-iotests/group | 1 +
4 files changed, 91 insertions(+), 3 deletions(-)
create mode 100755 tests/qemu-iotests/305
create mode 100644 tests/qemu-iotests/305.out
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4530e5e471..f65ccc5840 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1710,9 +1710,6 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
out:
qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
- if (ret < 0 && *m && (*m)->nb_clusters > 0) {
- QLIST_REMOVE(*m, next_in_flight);
- }
return ret;
}
diff --git a/tests/qemu-iotests/305 b/tests/qemu-iotests/305
new file mode 100755
index 0000000000..768818af4a
--- /dev/null
+++ b/tests/qemu-iotests/305
@@ -0,0 +1,74 @@
+#!/usr/bin/env bash
+#
+# Test the handling of errors in write requests with multiple allocations
+#
+# Copyright (C) 2020 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.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/>.
+#
+
+# creator
+owner=berto@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_unsupported_imgopts cluster_size refcount_bits extended_l2 compat=0.10 data_file
+
+echo '### Create the image'
+_make_test_img -o refcount_bits=64,cluster_size=1k 1M
+
+# The reference counts of the clusters for the first 123k of this
+# write request are stored in the first refcount block. The last
+# cluster (guest offset 123k) is referenced in the second refcount
+# block.
+echo '### Fill the first refcount block and one data cluster from the second'
+$QEMU_IO -c 'write 0 124k' "$TEST_IMG" | _filter_qemu_io
+
+echo '### Discard two of the last data clusters, leave one in the middle'
+$QEMU_IO -c 'discard 121k 1k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'discard 123k 1k' "$TEST_IMG" | _filter_qemu_io
+
+echo '### Corrupt the offset of the second refcount block'
+refcount_table_offset=$(peek_file_be "$TEST_IMG" 48 8)
+poke_file "$TEST_IMG" $(($refcount_table_offset+14)) "\x06"
+
+# This tries to allocate the two clusters discarded earlier (guest
+# offsets 121k and 123k). Their reference counts are in the first and
+# second refcount blocks respectively, but only the first one can be
+# allocated correctly because the second entry of the refcount table
+# is corrupted.
+echo '### Try to allocate the discarded clusters again'
+$QEMU_IO -c 'write 121k 3k' "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/305.out b/tests/qemu-iotests/305.out
new file mode 100644
index 0000000000..538019e726
--- /dev/null
+++ b/tests/qemu-iotests/305.out
@@ -0,0 +1,16 @@
+QA output created by 305
+### Create the image
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+### Fill the first refcount block and one data cluster from the second
+wrote 126976/126976 bytes at offset 0
+124 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+### Discard two of the last data clusters, leave one in the middle
+discard 1024/1024 bytes at offset 123904
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 1024/1024 bytes at offset 125952
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+### Corrupt the offset of the second refcount block
+### Try to allocate the discarded clusters again
+qcow2: Marking image as corrupt: Refblock offset 0x20600 unaligned (reftable index: 0x1); further corruption events will be suppressed
+write failed: Input/output error
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5cad015231..ff59cfd2d4 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -313,3 +313,4 @@
302 quick
303 rw quick
304 rw quick
+305 rw quick
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 10/22] qcow2: Don't check nb_clusters when removing l2meta from the list
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (8 preceding siblings ...)
2020-09-15 10:46 ` [PULL 09/22] qcow2: Fix removal of list members from BDRVQcow2State.cluster_allocs Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 11/22] qcow2: Rewrite the documentation of qcow2_alloc_cluster_offset() Max Reitz
` (12 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
In the past, when a new cluster was allocated the l2meta structure was
a variable in the stack so it was necessary to have a way to tell
whether it had been initialized and contained valid data or not. The
nb_clusters field was used for this purpose. Since commit f50f88b9fe
this is no longer the case, l2meta (nowadays a pointer to a list) is
only allocated when needed and nb_clusters is guaranteed to be > 0 so
this check is unnecessary.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <ab0b67c29c7ba26e598db35f12aa5ab5982539c1.1599150873.git.berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index ef9a45e82f..3e8114dcf8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2111,9 +2111,7 @@ static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
}
/* Take the request off the list of running requests */
- if (l2meta->nb_clusters != 0) {
- QLIST_REMOVE(l2meta, next_in_flight);
- }
+ QLIST_REMOVE(l2meta, next_in_flight);
qemu_co_queue_restart_all(&l2meta->dependent_requests);
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 11/22] qcow2: Rewrite the documentation of qcow2_alloc_cluster_offset()
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (9 preceding siblings ...)
2020-09-15 10:46 ` [PULL 10/22] qcow2: Don't check nb_clusters when removing l2meta from the list Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 12/22] qemu-img: Explicit number replaced by a constant Max Reitz
` (11 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
The current text corresponds to an earlier, simpler version of this
function and it does not explain how it works now.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <bb5bd06f07c5a05b0818611de0d06ec5b66c8df3.1599150873.git.berto@igalia.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2-cluster.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f65ccc5840..ce1260e746 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1714,18 +1714,22 @@ out:
}
/*
- * alloc_cluster_offset
+ * For a given area on the virtual disk defined by @offset and @bytes,
+ * find the corresponding area on the qcow2 image, allocating new
+ * clusters (or subclusters) if necessary. The result can span a
+ * combination of allocated and previously unallocated clusters.
*
- * For a given offset on the virtual disk, find the cluster offset in qcow2
- * file. If the offset is not found, allocate a new cluster.
+ * On return, @host_offset is set to the beginning of the requested
+ * area. This area is guaranteed to be contiguous on the qcow2 file
+ * but it can be smaller than initially requested. In this case @bytes
+ * is updated with the actual size.
*
- * If the cluster was already allocated, m->nb_clusters is set to 0 and
- * other fields in m are meaningless.
- *
- * If the cluster is newly allocated, m->nb_clusters is set to the number of
- * contiguous clusters that have been allocated. In this case, the other
- * fields of m are valid and contain information about the first allocated
- * cluster.
+ * If any clusters or subclusters were allocated then @m contains a
+ * list with the information of all the affected regions. Note that
+ * this can happen regardless of whether this function succeeds or
+ * not. The caller is responsible for updating the L2 metadata of the
+ * allocated clusters (on success) or freeing them (on failure), and
+ * for clearing the contents of @m afterwards in both cases.
*
* If the request conflicts with another write request in flight, the coroutine
* is queued and will be reentered when the dependency has completed.
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 12/22] qemu-img: Explicit number replaced by a constant
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (10 preceding siblings ...)
2020-09-15 10:46 ` [PULL 11/22] qcow2: Rewrite the documentation of qcow2_alloc_cluster_offset() Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 13/22] iotests: Skip test_stream_parallel in test 030 when doing "make check" Max Reitz
` (10 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Yi Li <yili@winhong.com>
Signed-off-by: Yi Li <yili@winhong.com>
Message-Id: <20200819013607.32280-1-yili@winhong.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
qemu-img.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 37365d06fa..1d8c5cd778 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1201,10 +1201,10 @@ static int is_allocated_sectors(const uint8_t *buf, int n, int *pnum,
*pnum = 0;
return 0;
}
- is_zero = buffer_is_zero(buf, 512);
+ is_zero = buffer_is_zero(buf, BDRV_SECTOR_SIZE);
for(i = 1; i < n; i++) {
- buf += 512;
- if (is_zero != buffer_is_zero(buf, 512)) {
+ buf += BDRV_SECTOR_SIZE;
+ if (is_zero != buffer_is_zero(buf, BDRV_SECTOR_SIZE)) {
break;
}
}
@@ -2513,8 +2513,8 @@ static int img_convert(int argc, char **argv)
}
}
- qemu_opt_set_number(opts, BLOCK_OPT_SIZE, s.total_sectors * 512,
- &error_abort);
+ qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
+ s.total_sectors * BDRV_SECTOR_SIZE, &error_abort);
ret = add_old_style_options(out_fmt, opts, out_baseimg, NULL);
if (ret < 0) {
goto out;
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 13/22] iotests: Skip test_stream_parallel in test 030 when doing "make check"
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (11 preceding siblings ...)
2020-09-15 10:46 ` [PULL 12/22] qemu-img: Explicit number replaced by a constant Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 14/22] block/vhdx: Support vhdx image only with 512 bytes logical sector size Max Reitz
` (9 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Thomas Huth <thuth@redhat.com>
The test_stream_parallel test still occasionally fails in the CI.
Thus let's disable it during "make check" for now so that it does
not cause trouble during merge tests. We can enable it again once
the problem has been resolved.
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20200907113824.134788-1-thuth@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/check-block.sh | 3 +++
tests/qemu-iotests/030 | 2 ++
2 files changed, 5 insertions(+)
diff --git a/tests/check-block.sh b/tests/check-block.sh
index 8e29c868e5..a5a69060e1 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -55,6 +55,9 @@ fi
cd tests/qemu-iotests
+# QEMU_CHECK_BLOCK_AUTO is used to disable some unstable sub-tests
+export QEMU_CHECK_BLOCK_AUTO=1
+
ret=0
for fmt in $format_list ; do
./check -makecheck -$fmt $group || ret=1
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 31c028306b..dcb4b5d6a6 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,6 +21,7 @@
import time
import os
import iotests
+import unittest
from iotests import qemu_img, qemu_io
backing_img = os.path.join(iotests.test_dir, 'backing.img')
@@ -228,6 +229,7 @@ class TestParallelOps(iotests.QMPTestCase):
# Test that it's possible to run several block-stream operations
# in parallel in the same snapshot chain
+ @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 'disabled in CI')
def test_stream_parallel(self):
self.assert_no_active_block_jobs()
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 14/22] block/vhdx: Support vhdx image only with 512 bytes logical sector size
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (12 preceding siblings ...)
2020-09-15 10:46 ` [PULL 13/22] iotests: Skip test_stream_parallel in test 030 when doing "make check" Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 15/22] qcow2: Handle QCowL2Meta on error in preallocate_co() Max Reitz
` (8 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Swapnil Ingle <swapnil.ingle@nutanix.com>
block/vhdx uses qemu block layer where sector size is always 512 bytes.
This may have issues with 4K logical sector sized vhdx image.
For e.g qemu-img convert on such images fails with following assert:
$qemu-img convert -f vhdx -O raw 4KTest1.vhdx test.raw
qemu-img: util/iov.c:388: qiov_slice: Assertion `offset + len <=
qiov->size' failed.
Aborted
This patch adds an check to return ENOTSUP for vhdx images which
have logical sector size other than 512 bytes.
Signed-off-by: Swapnil Ingle <swapnil.ingle@nutanix.com>
Message-Id: <1596794594-44531-1-git-send-email-swapnil.ingle@nutanix.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/vhdx.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/block/vhdx.c b/block/vhdx.c
index 791eb90263..356ec4c455 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -816,9 +816,9 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
goto exit;
}
- /* only 2 supported sector sizes */
- if (s->logical_sector_size != 512 && s->logical_sector_size != 4096) {
- ret = -EINVAL;
+ /* Currently we only support 512 */
+ if (s->logical_sector_size != 512) {
+ ret = -ENOTSUP;
goto exit;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 15/22] qcow2: Handle QCowL2Meta on error in preallocate_co()
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (13 preceding siblings ...)
2020-09-15 10:46 ` [PULL 14/22] block/vhdx: Support vhdx image only with 512 bytes logical sector size Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 16/22] qcow2: Make qcow2_free_any_clusters() free only one cluster Max Reitz
` (7 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
If qcow2_alloc_cluster_offset() or qcow2_alloc_cluster_link_l2() fail
then this function simply returns the error code, potentially leaking
the QCowL2Meta structure and leaving stale items in s->cluster_allocs.
A second problem is that this function calls qcow2_free_any_clusters()
on failure but passing a host cluster offset instead of an L2 entry.
Luckily for normal uncompressed clusters a raw offset also works like
a valid L2 entry so it works just the same, but we should be using
qcow2_free_clusters() instead.
This patch fixes both problems by using qcow2_handle_l2meta().
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <cd3a6b9abd43f9c0b60be413d760f0cacc67eb66.1599573989.git.berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 40 +++++++++++++++++-----------------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3e8114dcf8..d241fb734c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2101,7 +2101,6 @@ static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
QCowL2Meta *next;
if (link_l2) {
- assert(!l2meta->prealloc);
ret = qcow2_alloc_cluster_link_l2(bs, l2meta);
if (ret) {
goto out;
@@ -3123,7 +3122,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
int64_t file_length;
unsigned int cur_bytes;
int ret;
- QCowL2Meta *meta;
+ QCowL2Meta *meta = NULL, *m;
assert(offset <= new_length);
bytes = new_length - offset;
@@ -3134,27 +3133,17 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
&host_offset, &meta);
if (ret < 0) {
error_setg_errno(errp, -ret, "Allocating clusters failed");
- return ret;
+ goto out;
}
- while (meta) {
- QCowL2Meta *next = meta->next;
- meta->prealloc = true;
-
- ret = qcow2_alloc_cluster_link_l2(bs, meta);
- if (ret < 0) {
- error_setg_errno(errp, -ret, "Mapping clusters failed");
- qcow2_free_any_clusters(bs, meta->alloc_offset,
- meta->nb_clusters, QCOW2_DISCARD_NEVER);
- return ret;
- }
-
- /* There are no dependent requests, but we need to remove our
- * request from the list of in-flight requests */
- QLIST_REMOVE(meta, next_in_flight);
+ for (m = meta; m != NULL; m = m->next) {
+ m->prealloc = true;
+ }
- g_free(meta);
- meta = next;
+ ret = qcow2_handle_l2meta(bs, &meta, true);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Mapping clusters failed");
+ goto out;
}
/* TODO Preallocate data if requested */
@@ -3171,7 +3160,8 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
file_length = bdrv_getlength(s->data_file->bs);
if (file_length < 0) {
error_setg_errno(errp, -file_length, "Could not get file size");
- return file_length;
+ ret = file_length;
+ goto out;
}
if (host_offset + cur_bytes > file_length) {
@@ -3181,11 +3171,15 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
ret = bdrv_co_truncate(s->data_file, host_offset + cur_bytes, false,
mode, 0, errp);
if (ret < 0) {
- return ret;
+ goto out;
}
}
- return 0;
+ ret = 0;
+
+out:
+ qcow2_handle_l2meta(bs, &meta, false);
+ return ret;
}
/* qcow2_refcount_metadata_size:
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 16/22] qcow2: Make qcow2_free_any_clusters() free only one cluster
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (14 preceding siblings ...)
2020-09-15 10:46 ` [PULL 15/22] qcow2: Handle QCowL2Meta on error in preallocate_co() Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 17/22] qcow2: Return the original error code in qcow2_co_pwrite_zeroes() Max Reitz
` (6 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This function takes an L2 entry and a number of clusters to free.
Although in principle it can free any type of cluster (using the L2
entry to determine its type) in practice the API is broken because
compressed clusters have a variable size and there is no way to free
more than one without having the L2 entry of each one of them.
The good news all callers are passing nb_clusters=1 so we can simply
get rid of that parameter.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <77cea0f4616f921d37e971b3c5b18a2faa24b173.1599573989.git.berto@igalia.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.h | 4 ++--
block/qcow2-cluster.c | 6 +++---
block/qcow2-refcount.c | 8 ++++----
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 83cfa0c391..b73a4cf1f8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -861,8 +861,8 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size);
void qcow2_free_clusters(BlockDriverState *bs,
int64_t offset, int64_t size,
enum qcow2_discard_type type);
-void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
- int nb_clusters, enum qcow2_discard_type type);
+void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
+ enum qcow2_discard_type type);
int qcow2_update_snapshot_refcount(BlockDriverState *bs,
int64_t l1_table_offset, int l1_size, int addend);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ce1260e746..1a67b2d928 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1096,7 +1096,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
*/
if (!m->keep_old_clusters && j != 0) {
for (i = 0; i < j; i++) {
- qcow2_free_any_clusters(bs, old_cluster[i], 1, QCOW2_DISCARD_NEVER);
+ qcow2_free_any_cluster(bs, old_cluster[i], QCOW2_DISCARD_NEVER);
}
}
@@ -1913,7 +1913,7 @@ static int discard_in_l2_slice(BlockDriverState *bs, uint64_t offset,
set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap);
}
/* Then decrease the refcount */
- qcow2_free_any_clusters(bs, old_l2_entry, 1, type);
+ qcow2_free_any_cluster(bs, old_l2_entry, type);
}
qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
@@ -2005,7 +2005,7 @@ static int zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
if (unmap) {
- qcow2_free_any_clusters(bs, old_l2_entry, 1, QCOW2_DISCARD_REQUEST);
+ qcow2_free_any_cluster(bs, old_l2_entry, QCOW2_DISCARD_REQUEST);
}
set_l2_entry(s, l2_slice, l2_index + i, new_l2_entry);
if (has_subclusters(s)) {
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 51e9778ed8..8e649b008e 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1157,8 +1157,8 @@ void qcow2_free_clusters(BlockDriverState *bs,
* Free a cluster using its L2 entry (handles clusters of all types, e.g.
* normal cluster, compressed cluster, etc.)
*/
-void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
- int nb_clusters, enum qcow2_discard_type type)
+void qcow2_free_any_cluster(BlockDriverState *bs, uint64_t l2_entry,
+ enum qcow2_discard_type type)
{
BDRVQcow2State *s = bs->opaque;
QCow2ClusterType ctype = qcow2_get_cluster_type(bs, l2_entry);
@@ -1169,7 +1169,7 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
ctype == QCOW2_CLUSTER_ZERO_ALLOC))
{
bdrv_pdiscard(s->data_file, l2_entry & L2E_OFFSET_MASK,
- nb_clusters << s->cluster_bits);
+ s->cluster_size);
}
return;
}
@@ -1192,7 +1192,7 @@ void qcow2_free_any_clusters(BlockDriverState *bs, uint64_t l2_entry,
l2_entry & L2E_OFFSET_MASK);
} else {
qcow2_free_clusters(bs, l2_entry & L2E_OFFSET_MASK,
- nb_clusters << s->cluster_bits, type);
+ s->cluster_size, type);
}
break;
case QCOW2_CLUSTER_ZERO_PLAIN:
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 17/22] qcow2: Return the original error code in qcow2_co_pwrite_zeroes()
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (15 preceding siblings ...)
2020-09-15 10:46 ` [PULL 16/22] qcow2: Make qcow2_free_any_clusters() free only one cluster Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 18/22] block/rbd: remove runtime_opts Max Reitz
` (5 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This function checks the current status of a (sub)cluster in order to
see if an unaligned 'write zeroes' request can be done efficiently by
simply updating the L2 metadata and without having to write actual
zeroes to disk.
If the situation does not allow using the fast path then the function
returns -ENOTSUP and the caller falls back to writing zeroes.
If can happen however that the aforementioned check returns an actual
error code so in this case we should pass it to the caller.
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <20200909123739.719-1-berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index d241fb734c..77c43ce178 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3907,7 +3907,7 @@ static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,
type != QCOW2_SUBCLUSTER_ZERO_PLAIN &&
type != QCOW2_SUBCLUSTER_ZERO_ALLOC)) {
qemu_co_mutex_unlock(&s->lock);
- return -ENOTSUP;
+ return ret < 0 ? ret : -ENOTSUP;
}
} else {
qemu_co_mutex_lock(&s->lock);
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 18/22] block/rbd: remove runtime_opts
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (16 preceding siblings ...)
2020-09-15 10:46 ` [PULL 17/22] qcow2: Return the original error code in qcow2_co_pwrite_zeroes() Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 19/22] block/qcow: remove runtime opts Max Reitz
` (4 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: John Snow <jsnow@redhat.com>
This saw its last use in 4bfb274165ba.
Signed-off-by: John Snow <jsnow@redhat.com>
Message-Id: <20200806211345.2925343-2-jsnow@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/rbd.c | 42 ------------------------------------------
1 file changed, 42 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index 688074c64b..171c67e3a0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -341,48 +341,6 @@ static void qemu_rbd_memset(RADOSCB *rcb, int64_t offs)
}
}
-static QemuOptsList runtime_opts = {
- .name = "rbd",
- .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
- .desc = {
- {
- .name = "pool",
- .type = QEMU_OPT_STRING,
- .help = "Rados pool name",
- },
- {
- .name = "namespace",
- .type = QEMU_OPT_STRING,
- .help = "Rados namespace name in the pool",
- },
- {
- .name = "image",
- .type = QEMU_OPT_STRING,
- .help = "Image name in the pool",
- },
- {
- .name = "conf",
- .type = QEMU_OPT_STRING,
- .help = "Rados config file location",
- },
- {
- .name = "snapshot",
- .type = QEMU_OPT_STRING,
- .help = "Ceph snapshot name",
- },
- {
- /* maps to 'id' in rados_create() */
- .name = "user",
- .type = QEMU_OPT_STRING,
- .help = "Rados id name",
- },
- /*
- * server.* extracted manually, see qemu_rbd_mon_host()
- */
- { /* end of list */ }
- },
-};
-
/* FIXME Deprecate and remove keypairs or make it available in QMP. */
static int qemu_rbd_do_create(BlockdevCreateOptions *options,
const char *keypairs, const char *password_secret,
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 19/22] block/qcow: remove runtime opts
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (17 preceding siblings ...)
2020-09-15 10:46 ` [PULL 18/22] block/rbd: remove runtime_opts Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 20/22] qcow2: Make preallocate_co() resize the image to the correct size Max Reitz
` (3 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: John Snow <jsnow@redhat.com>
Introduced by d85f4222b468,
These were seemingly never used at all.
Signed-off-by: John Snow <jsnow@redhat.com>
Message-Id: <20200806211345.2925343-3-jsnow@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/block/qcow.c b/block/qcow.c
index e514a86fe5..f8919a44d1 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -105,15 +105,6 @@ static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static QemuOptsList qcow_runtime_opts = {
- .name = "qcow",
- .head = QTAILQ_HEAD_INITIALIZER(qcow_runtime_opts.head),
- .desc = {
- BLOCK_CRYPTO_OPT_DEF_QCOW_KEY_SECRET("encrypt."),
- { /* end of list */ }
- },
-};
-
static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 20/22] qcow2: Make preallocate_co() resize the image to the correct size
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (18 preceding siblings ...)
2020-09-15 10:46 ` [PULL 19/22] block/qcow: remove runtime opts Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 21/22] qcow2: Convert qcow2_alloc_cluster_offset() into qcow2_alloc_host_offset() Max Reitz
` (2 subsequent siblings)
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
This function preallocates metadata structures and then extends the
image to its new size, but that new size calculation is wrong because
it doesn't take into account that the host_offset variable is always
cluster-aligned.
This problem can be reproduced with preallocation=metadata when the
original size is not cluster-aligned but the new size is. In this case
the final image size will be shorter than expected.
qemu-img create -f qcow2 img.qcow2 31k
qemu-img resize --preallocation=metadata img.qcow2 128k
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <adeb8b059917b141d5f5b3bd2a016262d3052c79.1599833007.git.berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
[mreitz: Mark compat=0.10 unsupported for iotest 125]
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 1 +
tests/qemu-iotests/125 | 44 ++++++++++++++++++++++----------------
tests/qemu-iotests/125.out | 28 ++++++++++++++++++++++--
3 files changed, 53 insertions(+), 20 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 77c43ce178..1cb5daf39a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3135,6 +3135,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
error_setg_errno(errp, -ret, "Allocating clusters failed");
goto out;
}
+ host_offset += offset_into_cluster(s, offset);
for (m = meta; m != NULL; m = m->next) {
m->prealloc = true;
diff --git a/tests/qemu-iotests/125 b/tests/qemu-iotests/125
index 7cb1c19730..5720e86dce 100755
--- a/tests/qemu-iotests/125
+++ b/tests/qemu-iotests/125
@@ -43,6 +43,10 @@ get_image_size_on_host()
_supported_fmt qcow2
_supported_proto file
+# Growing a file with a backing file (without preallocation=full or
+# =falloc) requires zeroing the newly added area, which is impossible
+# to do quickly for v2 images, and hence is unsupported.
+_unsupported_imgopts 'compat=0.10'
if [ -z "$TEST_IMG_FILE" ]; then
TEST_IMG_FILE=$TEST_IMG
@@ -168,24 +172,28 @@ done
$QEMU_IMG create -f raw "$TEST_IMG.base" 128k | _filter_img_create
$QEMU_IO -c 'write -q -P 1 0 128k' -f raw "$TEST_IMG.base"
for orig_size in 31k 33k; do
- echo "--- Resizing image from $orig_size to 96k ---"
- _make_test_img -F raw -b "$TEST_IMG.base" -o cluster_size=64k "$orig_size"
- $QEMU_IMG resize -f "$IMGFMT" --preallocation=full "$TEST_IMG" 96k
- # The first part of the image should contain data from the backing file
- $QEMU_IO -c "read -q -P 1 0 ${orig_size}" "$TEST_IMG"
- # The resized part of the image should contain zeroes
- $QEMU_IO -c "read -q -P 0 ${orig_size} 63k" "$TEST_IMG"
- # If the image does not have an external data file we can also verify its
- # actual size. The resized image should have 7 clusters:
- # header, L1 table, L2 table, refcount table, refcount block, 2 data clusters
- if ! _get_data_file "$TEST_IMG" > /dev/null; then
- expected_file_length=$((65536 * 7))
- file_length=$(stat -c '%s' "$TEST_IMG_FILE")
- if [ "$file_length" != "$expected_file_length" ]; then
- echo "ERROR: file length $file_length (expected $expected_file_length)"
- fi
- fi
- echo
+ for dst_size in 96k 128k; do
+ for prealloc in metadata full; do
+ echo "--- Resizing image from $orig_size to $dst_size (preallocation=$prealloc) ---"
+ _make_test_img -F raw -b "$TEST_IMG.base" -o cluster_size=64k "$orig_size"
+ $QEMU_IMG resize -f "$IMGFMT" --preallocation="$prealloc" "$TEST_IMG" "$dst_size"
+ # The first part of the image should contain data from the backing file
+ $QEMU_IO -c "read -q -P 1 0 ${orig_size}" "$TEST_IMG"
+ # The resized part of the image should contain zeroes
+ $QEMU_IO -c "read -q -P 0 ${orig_size} 63k" "$TEST_IMG"
+ # If the image does not have an external data file we can also verify its
+ # actual size. The resized image should have 7 clusters:
+ # header, L1 table, L2 table, refcount table, refcount block, 2 data clusters
+ if ! _get_data_file "$TEST_IMG" > /dev/null; then
+ expected_file_length=$((65536 * 7))
+ file_length=$(stat -c '%s' "$TEST_IMG_FILE")
+ if [ "$file_length" != "$expected_file_length" ]; then
+ echo "ERROR: file length $file_length (expected $expected_file_length)"
+ fi
+ fi
+ echo
+ done
+ done
done
# success, all done
diff --git a/tests/qemu-iotests/125.out b/tests/qemu-iotests/125.out
index 7f76f7af20..63a6e9e8a9 100644
--- a/tests/qemu-iotests/125.out
+++ b/tests/qemu-iotests/125.out
@@ -768,11 +768,35 @@ wrote 81920/81920 bytes at offset 2048000
80 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=raw size=131072
---- Resizing image from 31k to 96k ---
+--- Resizing image from 31k to 96k (preallocation=metadata) ---
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
Image resized.
---- Resizing image from 33k to 96k ---
+--- Resizing image from 31k to 96k (preallocation=full) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 31k to 128k (preallocation=metadata) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 31k to 128k (preallocation=full) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=31744 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 96k (preallocation=metadata) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 96k (preallocation=full) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 128k (preallocation=metadata) ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
+Image resized.
+
+--- Resizing image from 33k to 128k (preallocation=full) ---
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33792 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw
Image resized.
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 21/22] qcow2: Convert qcow2_alloc_cluster_offset() into qcow2_alloc_host_offset()
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (19 preceding siblings ...)
2020-09-15 10:46 ` [PULL 20/22] qcow2: Make preallocate_co() resize the image to the correct size Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 10:46 ` [PULL 22/22] block/rbd: add 'namespace' to qemu_rbd_strong_runtime_opts[] Max Reitz
2020-09-15 12:04 ` [PULL 00/22] Block patches Peter Maydell
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Alberto Garcia <berto@igalia.com>
qcow2_alloc_cluster_offset() takes an (unaligned) guest offset and
returns the (aligned) offset of the corresponding cluster in the qcow2
image.
In practice none of the callers need to know where the cluster starts
so this patch makes the function calculate and return the final host
offset directly. The function is also renamed accordingly.
See 388e581615 for a similar change to qcow2_get_cluster_offset().
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <9bfef50ec9200d752413be4fc2aeb22a28378817.1599833007.git.berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.h | 6 +++---
block/qcow2-cluster.c | 14 ++++++++++----
block/qcow2.c | 36 +++++++++++++-----------------------
3 files changed, 26 insertions(+), 30 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index b73a4cf1f8..b71e444fca 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -901,9 +901,9 @@ int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
int qcow2_get_host_offset(BlockDriverState *bs, uint64_t offset,
unsigned int *bytes, uint64_t *host_offset,
QCow2SubclusterType *subcluster_type);
-int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *host_offset,
- QCowL2Meta **m);
+int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
+ unsigned int *bytes, uint64_t *host_offset,
+ QCowL2Meta **m);
int qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
uint64_t offset,
int compressed_size,
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1a67b2d928..9acc6ce4ae 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1719,6 +1719,10 @@ out:
* clusters (or subclusters) if necessary. The result can span a
* combination of allocated and previously unallocated clusters.
*
+ * Note that offset may not be cluster aligned. In this case, the returned
+ * *host_offset points to exact byte referenced by offset and therefore
+ * isn't cluster aligned as well.
+ *
* On return, @host_offset is set to the beginning of the requested
* area. This area is guaranteed to be contiguous on the qcow2 file
* but it can be smaller than initially requested. In this case @bytes
@@ -1736,9 +1740,9 @@ out:
*
* Return 0 on success and -errno in error cases
*/
-int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
- unsigned int *bytes, uint64_t *host_offset,
- QCowL2Meta **m)
+int qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
+ unsigned int *bytes, uint64_t *host_offset,
+ QCowL2Meta **m)
{
BDRVQcow2State *s = bs->opaque;
uint64_t start, remaining;
@@ -1759,7 +1763,7 @@ again:
while (true) {
if (*host_offset == INV_OFFSET && cluster_offset != INV_OFFSET) {
- *host_offset = start_of_cluster(s, cluster_offset);
+ *host_offset = cluster_offset;
}
assert(remaining >= cur_bytes);
@@ -1842,6 +1846,8 @@ again:
*bytes -= remaining;
assert(*bytes > 0);
assert(*host_offset != INV_OFFSET);
+ assert(offset_into_cluster(s, *host_offset) ==
+ offset_into_cluster(s, offset));
return 0;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 1cb5daf39a..b05512718c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2559,7 +2559,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
int offset_in_cluster;
int ret;
unsigned int cur_bytes; /* number of sectors in current iteration */
- uint64_t cluster_offset;
+ uint64_t host_offset;
QCowL2Meta *l2meta = NULL;
AioTaskPool *aio = NULL;
@@ -2580,16 +2580,13 @@ static coroutine_fn int qcow2_co_pwritev_part(
qemu_co_mutex_lock(&s->lock);
- ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
- &cluster_offset, &l2meta);
+ ret = qcow2_alloc_host_offset(bs, offset, &cur_bytes,
+ &host_offset, &l2meta);
if (ret < 0) {
goto out_locked;
}
- assert(offset_into_cluster(s, cluster_offset) == 0);
-
- ret = qcow2_pre_write_overlap_check(bs, 0,
- cluster_offset + offset_in_cluster,
+ ret = qcow2_pre_write_overlap_check(bs, 0, host_offset,
cur_bytes, true);
if (ret < 0) {
goto out_locked;
@@ -2601,7 +2598,7 @@ static coroutine_fn int qcow2_co_pwritev_part(
aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
}
ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_task_entry, 0,
- cluster_offset + offset_in_cluster, offset,
+ host_offset, offset,
cur_bytes, qiov, qiov_offset, l2meta);
l2meta = NULL; /* l2meta is consumed by qcow2_co_pwritev_task() */
if (ret < 0) {
@@ -3129,13 +3126,12 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
while (bytes) {
cur_bytes = MIN(bytes, QEMU_ALIGN_DOWN(INT_MAX, s->cluster_size));
- ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
- &host_offset, &meta);
+ ret = qcow2_alloc_host_offset(bs, offset, &cur_bytes,
+ &host_offset, &meta);
if (ret < 0) {
error_setg_errno(errp, -ret, "Allocating clusters failed");
goto out;
}
- host_offset += offset_into_cluster(s, offset);
for (m = meta; m != NULL; m = m->next) {
m->prealloc = true;
@@ -4043,10 +4039,9 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
BdrvRequestFlags write_flags)
{
BDRVQcow2State *s = bs->opaque;
- int offset_in_cluster;
int ret;
unsigned int cur_bytes; /* number of sectors in current iteration */
- uint64_t cluster_offset;
+ uint64_t host_offset;
QCowL2Meta *l2meta = NULL;
assert(!bs->encrypted);
@@ -4057,31 +4052,26 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
l2meta = NULL;
- offset_in_cluster = offset_into_cluster(s, dst_offset);
cur_bytes = MIN(bytes, INT_MAX);
/* TODO:
* If src->bs == dst->bs, we could simply copy by incrementing
* the refcnt, without copying user data.
* Or if src->bs == dst->bs->backing->bs, we could copy by discarding. */
- ret = qcow2_alloc_cluster_offset(bs, dst_offset, &cur_bytes,
- &cluster_offset, &l2meta);
+ ret = qcow2_alloc_host_offset(bs, dst_offset, &cur_bytes,
+ &host_offset, &l2meta);
if (ret < 0) {
goto fail;
}
- assert(offset_into_cluster(s, cluster_offset) == 0);
-
- ret = qcow2_pre_write_overlap_check(bs, 0,
- cluster_offset + offset_in_cluster, cur_bytes, true);
+ ret = qcow2_pre_write_overlap_check(bs, 0, host_offset, cur_bytes,
+ true);
if (ret < 0) {
goto fail;
}
qemu_co_mutex_unlock(&s->lock);
- ret = bdrv_co_copy_range_to(src, src_offset,
- s->data_file,
- cluster_offset + offset_in_cluster,
+ ret = bdrv_co_copy_range_to(src, src_offset, s->data_file, host_offset,
cur_bytes, read_flags, write_flags);
qemu_co_mutex_lock(&s->lock);
if (ret < 0) {
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PULL 22/22] block/rbd: add 'namespace' to qemu_rbd_strong_runtime_opts[]
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (20 preceding siblings ...)
2020-09-15 10:46 ` [PULL 21/22] qcow2: Convert qcow2_alloc_cluster_offset() into qcow2_alloc_host_offset() Max Reitz
@ 2020-09-15 10:46 ` Max Reitz
2020-09-15 12:04 ` [PULL 00/22] Block patches Peter Maydell
22 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2020-09-15 10:46 UTC (permalink / raw)
To: qemu-block; +Cc: Peter Maydell, qemu-devel, Max Reitz
From: Stefano Garzarella <sgarzare@redhat.com>
Commit 19ae9ae014 ("block/rbd: Add support for ceph namespaces")
introduced namespace support for RBD, but we forgot to add the
new 'namespace' options to qemu_rbd_strong_runtime_opts[].
The 'namespace' is used to identify the image, so it is a strong
option since it can changes the data of a BDS.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1821528
Fixes: 19ae9ae014 ("block/rbd: Add support for ceph namespaces")
Cc: Florian Florensa <fflorensa@online.net>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Message-Id: <20200914190553.74871-1-sgarzare@redhat.com>
Reviewed-by: Jason Dillaman <dillaman@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/rbd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/rbd.c b/block/rbd.c
index 171c67e3a0..9bd2bce716 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1247,6 +1247,7 @@ static QemuOptsList qemu_rbd_create_opts = {
static const char *const qemu_rbd_strong_runtime_opts[] = {
"pool",
+ "namespace",
"image",
"conf",
"snapshot",
--
2.26.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PULL 00/22] Block patches
2020-09-15 10:46 [PULL 00/22] Block patches Max Reitz
` (21 preceding siblings ...)
2020-09-15 10:46 ` [PULL 22/22] block/rbd: add 'namespace' to qemu_rbd_strong_runtime_opts[] Max Reitz
@ 2020-09-15 12:04 ` Peter Maydell
22 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2020-09-15 12:04 UTC (permalink / raw)
To: Max Reitz; +Cc: QEMU Developers, Qemu-block
On Tue, 15 Sep 2020 at 11:46, Max Reitz <mreitz@redhat.com> wrote:
>
> The following changes since commit 2d2c73d0e3d504a61f868e46e6abd5643f38091b:
>
> Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20200914-1' into staging (2020-09-14 16:03:08 +0100)
>
> are available in the Git repository at:
>
> https://github.com/XanClic/qemu.git tags/pull-block-2020-09-15
>
> for you to fetch changes up to 7bae7c805d82675eb3a02c744093703d84ada2d6:
>
> block/rbd: add 'namespace' to qemu_rbd_strong_runtime_opts[] (2020-09-15 11:31:10 +0200)
>
> ----------------------------------------------------------------
> Block patches:
> - Several qcow2 fixes and refactorings
> - Let qemu-img convert try to stay at cluster boundaries
> - Stable child names for quorum (with x-blockdev-change)
> - Explicitly drop vhdx 4k sector support, as it was never actually
> working
> - rbd: Mark @namespace a strong runtime option
> - iotests.py improvements
> - Drop unused runtime_opts objects
> - Skip a test case in 030 when run through make check-block
>
> ----------------------------------------------------------------
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread