Re: Dropping posix_fallocate for -o preallocation falloc
On Tue, Aug 25, 2020 at 2:27 PM Kevin Wolf wrote: > > Am 23.08.2020 um 16:46 hat Nir Soffer geschrieben: > > Using -o preallocation falloc works great on NFS 4.2 and local file system, > > when fallocate() is supported, but when it is not, posix_fallocate falls > > back > > to very inefficient way: > > https://code.woboq.org/userspace/glibc/sysdeps/posix/posix_fallocate.c.html#96 > > > > This will read the last byte for every 4k block, and if the byte is > > null, write one null byte. > > > > This minimizes the amount of data sent over the wire, but is very slow. > > > > In file-posix we optimize this flow by not truncating the file to the > > final size, so this will only write one null byte for every 4k block, > > but this is still very slow. > > > > Except the poor performance, we have a bug showing that for some reason, > > this does not work well with OFD locking: > > https://bugzilla.redhat.com/1851097 > > > > In oVirt 4.4.2 we avoid the issue by not using -o preallocation > > falloc. Instead we use our own fallocate helper: > > https://github.com/oVirt/vdsm/blob/master/helpers/fallocate > > > > (We got feedback that the name of this helper is confusing since it does > > destructive operation when fallocate() is not supported. We will > > change the name) > > > > This helper is similar to posix_fallocate, but instead of falling back > > to writing one byte per 4k block, it falls back to writing zeros in > > large blocks. > > > > Testing shows that this improves fallocation time by 385% for one disk, and > > 468% for 10 concurrent disk preallocation: > > https://bugzilla.redhat.com/1850267#c25 > > > > I think the next step is to move this change into qemu, so all users can > > benefit from this change. > > > > I think the way to do this is to replace posix_fallocate() with fallocate(), > > and fallback to "full" preallocation if fallocate is not supported. > > Yes, this makes sense to me. > > However, what full preallocation uses, is not exactly a large buffer > either (64k). Especially with your desire to use this with O_DIRECT, > we'd better use something like a 2 MB buffer. > > Actually, looking at the code, it's completely broken for O_DIRECT > because it g_malloc()s the (then misaligned) buffer. This needs to > become qemu_try_blockalign0(). It's currently failing: > > $ ./qemu-img create -f raw ~/tmp/test.raw 64M > Formatting '/home/kwolf/tmp/test.raw', fmt=raw size=67108864 > $ ./qemu-img resize --image-opts --preallocation=full > driver=file,filename=/home/kwolf/tmp/test.raw,cache.direct=on 72M > qemu-img: Could not write zeros for preallocation: Invalid argument > > > However with current code, in qemu-img create, we don't have a way to > > force O_DIRECT for the preallocation, and in qemu-img convert the > > preallocation step does not respect the -t none flag. Not using > > O_DIRECT in oVirt is very bad, and likely to cause timeouts in sanlock > > when the kernel flushes the page cache. > > If we're talking about regular files, the easy way without changing QEMU > would be to use the same way as in my example above: Create a 0 byte > image file and then resize it with full preallocation. > > But I can see that passing a cache mode to image creation is desirable. > So are probably some other options for image creation processes where > the image is actually opened with its block driver. > > Possible options I see in the order from an ad-hoc hack to a fully > generic interface: > > 1. Just add a cache mode create option to file-posix. qemu-img create >would mostly ignore this option for qcow2 (it would use O_DIRECT for >creating the raw image file, but not for opening when writing the >qcow2 data, including preallocation) > >QMP blockde-create would allow you to get the desired behaviour >because you open the raw layer explicitly there. It just wouldn't be >available in qemu-img create. > > 2. Add a cache mode/flags parameter to .bdrv_co_create_opts (the legacy >callback used for qemu-img create, but not QMP blockdev-create). >Format drivers would then use that for their bdrv_open() call on the >protocol layer; file-posix would directly use it for opening the >file. > > 3. Add a full BlockdevOptions parameter to .bdrv_co_create_opts() that >is used for the bdrv_open() of the protocol layer. I guess this would >bring in features that are currently not accessible from qemu-img >create because they can't be encoded in a URL but require options >(such as multiple servers for rbd or gluster). > > 4. Have some kind of full blockdev-create support in qemu-img create. >This wouldn't require any changes to the block drivers, but finding a >good syntax for qemu-img create might not be trivial. > > Given that 1-3 are all hacks to the legacy interface, maybe 4 is what we > should be doing to expose the full set of QEMU features in qemu-img > create, even though it poses interface questions that we wouldn't have > to answer with the other
[PATCH v3 3/5] qemu-iotests: Support varargs syntax in FilePaths
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
Reviewed-by: Max Reitz
---
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 4abfe63662..0d22ad5b03 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 socket 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
[PATCH v3 1/5] qemu-iotests: Fix FilePaths cleanup
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 Reviewed-by: Max Reitz --- 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 e197c73ca5..2fd5c916fa 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
[PATCH v3 5/5] qemu-iotests: Simplify FilePath __init__
Use list comprehension instead of append loop. Signed-off-by: Nir Soffer Reviewed-by: Max Reitz --- 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 7a20c9..cd0abf37e5 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
[PATCH v3 4/5] qemu-iotests: Merge FilePaths and FilePath
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
Reviewed-by: Max Reitz
---
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 0d22ad5b03..7a20c9 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 socket 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
[PATCH v3 2/5] qemu-iotests: Fix FilePaths docstring
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
---
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 2fd5c916fa..4abfe63662 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 socket 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
[PATCH v3 0/5] iotest.FilePath fixes and cleanups
Fix some issues introduced when iotests.FilePaths was added and merge it back into FilePath keeping the option to create multiple file names. Changes since v2: - Improve dosting to show how sockets should be created [Max] v2 was here: https://lists.nongnu.org/archive/html/qemu-block/2020-08/msg00780.html Changes since v1: - Remove unwanted submodule change [Eric] - Add Fixes: tag v1 was here: https://lists.nongnu.org/archive/html/qemu-block/2020-08/msg00773.html Nir Soffer (5): qemu-iotests: Fix FilePaths cleanup qemu-iotests: Fix FilePaths docstring qemu-iotests: Support varargs syntax in FilePaths qemu-iotests: Merge FilePaths and FilePath qemu-iotests: Simplify FilePath __init__ tests/qemu-iotests/194| 4 +-- tests/qemu-iotests/208| 2 +- tests/qemu-iotests/222| 2 +- tests/qemu-iotests/257| 10 +++ tests/qemu-iotests/iotests.py | 53 ++- 5 files changed, 36 insertions(+), 35 deletions(-) -- 2.26.2
[PATCH] block: always link with zlib
The qcow2 driver needs the zlib dependency. While emulators
provided it through the migration code, this is not true of
the tools. Move the dependency from the qcow1 rule directly
into block_ss so that it is included unconditionally.
Fixes build with --disable-qcow1.
Reported-by: Thomas Huth
Cc: qemu-block@nongnu.org
Signed-off-by: Paolo Bonzini
---
block/meson.build | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/meson.build b/block/meson.build
index 4dbbfe60b4..a3e56b7cd1 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -40,9 +40,9 @@ block_ss.add(files(
'vmdk.c',
'vpc.c',
'write-threshold.c',
-), zstd)
+), zstd, zlib)
-block_ss.add(when: [zlib, 'CONFIG_QCOW1'], if_true: files('qcow.c'))
+block_ss.add(when: 'CONFIG_QCOW1', if_true: files('qcow.c'))
block_ss.add(when: 'CONFIG_VDI', if_true: files('vdi.c'))
block_ss.add(when: 'CONFIG_CLOOP', if_true: files('cloop.c'))
block_ss.add(when: 'CONFIG_BOCHS', if_true: files('bochs.c'))
--
2.26.2
Re: QEMU build error with --disable-qcow1
On 28/08/20 18:34, Thomas Huth wrote:
>
> Linking target qemu-nbd
> libblock.fa(block_qcow2-threads.c.o): In function `qcow2_zlib_compress':
> /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:101:
> undefined reference to `deflateInit2_'
> /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:116:
> undefined reference to `deflate'
> /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:123:
> undefined reference to `deflateEnd'
> libblock.fa(block_qcow2-threads.c.o): In function `qcow2_zlib_decompress':
> /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:152:
> undefined reference to `inflateInit2_'
> /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:157:
> undefined reference to `inflate'
> /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:169:
> undefined reference to `inflateEnd'
> etc.
>
> Not sure whether this is due to the recent conversion to meson, or a
> recent change to that file ... anybody got a clue what's going on here?
It's a missing zlib dependency for the qcow2 files:
diff --git a/block/meson.build b/block/meson.build
index 4dbbfe60b4..a3e56b7cd1 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -40,9 +40,9 @@ block_ss.add(files(
'vmdk.c',
'vpc.c',
'write-threshold.c',
-), zstd)
+), zstd, zlib)
-block_ss.add(when: [zlib, 'CONFIG_QCOW1'], if_true: files('qcow.c'))
+block_ss.add(when: 'CONFIG_QCOW1', if_true: files('qcow.c'))
block_ss.add(when: 'CONFIG_VDI', if_true: files('vdi.c'))
block_ss.add(when: 'CONFIG_CLOOP', if_true: files('cloop.c'))
block_ss.add(when: 'CONFIG_BOCHS', if_true: files('bochs.c'))
(and then it becomes redundant for qcow1). I'll send a formal patch
shortly.
Paolo
[PATCH v8 7/7] block: apply COR-filter to block-stream jobs
This patch completes the series with the COR-filter insertion for
block-stream operations. Adding the filter makes it possible for copied
regions to be discarded in backing files during the block-stream job,
what will reduce the disk overuse.
The COR-filter insertion incurs changes in the iotests case
245:test_block_stream_4 that reopens the backing chain during a
block-stream job. There are changes in the iotests #030 as well.
The iotests case 030:test_stream_parallel was deleted due to multiple
conflicts between the concurrent job operations over the same backing
chain. The base backing node for one job is the top node for another
job. It may change due to the filter node inserted into the backing
chain while both jobs are running. Another issue is that the parts of
the backing chain are being frozen by the running job and may not be
changed by the concurrent job when needed. The concept of the parallel
jobs with common nodes is considered vital no more.
Signed-off-by: Andrey Shinkevich
---
block/stream.c | 78 +++---
tests/qemu-iotests/030 | 50 +++--
tests/qemu-iotests/030.out | 4 +--
tests/qemu-iotests/245 | 19 ---
4 files changed, 67 insertions(+), 84 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index fee4117..ab8ba39 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -19,6 +19,7 @@
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
#include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
enum {
/*
@@ -33,6 +34,8 @@ typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *base; /* The base node */
+BlockDriverState *cor_filter_bs;
+BlockDriverState *target_bs;
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -53,22 +56,19 @@ static void stream_abort(Job *job)
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
if (s->chain_frozen) {
-BlockJob *bjob = >common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
+bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->base);
}
}
static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-BlockJob *bjob = >common;
-BlockDriverState *bs = blk_bs(bjob->blk);
-BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
BlockDriverState *base = s->base;
Error *local_err = NULL;
int ret = 0;
-bdrv_unfreeze_backing_chain(bs, s->base);
+bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->base);
s->chain_frozen = false;
if (bdrv_cow_child(unfiltered_bs)) {
@@ -94,13 +94,14 @@ static void stream_clean(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockJob *bjob = >common;
-BlockDriverState *bs = blk_bs(bjob->blk);
+
+bdrv_cor_filter_drop(s->cor_filter_bs);
/* Reopen the image back in read-only mode if necessary */
if (s->bs_read_only) {
/* Give up write permissions before making it read-only */
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
-bdrv_reopen_set_read_only(bs, true, NULL);
+bdrv_reopen_set_read_only(s->target_bs, true, NULL);
}
g_free(s->backing_file_str);
@@ -110,9 +111,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
BlockBackend *blk = s->common.blk;
-BlockDriverState *bs = blk_bs(blk);
-BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
-bool enable_cor = !bdrv_cow_child(s->base_overlay);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
int64_t len;
int64_t offset = 0;
uint64_t delay_ns = 0;
@@ -124,21 +123,12 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
return 0;
}
-len = bdrv_getlength(bs);
+len = bdrv_getlength(s->target_bs);
if (len < 0) {
return len;
}
job_progress_set_remaining(>common.job, len);
-/* Turn on copy-on-read for the whole block device so that guest read
- * requests help us make progress. Only do this when copying the entire
- * backing chain since the copy-on-read operation does not take base into
- * account.
- */
-if (enable_cor) {
-bdrv_enable_copy_on_read(bs);
-}
-
for ( ; offset < len; offset += n) {
bool copy;
int ret;
@@ -197,10 +187,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
}
}
-if (enable_cor) {
-bdrv_disable_copy_on_read(bs);
-}
-
/* Do not remove the backing file if an error was there but ignored. */
return error;
}
@@ -230,6 +216,7 @@ void stream_start(const char
[PATCH v8 5/7] copy-on-read: limit guest writes to base in COR driver
Limit the guest's COR operations by the base node in the backing chain
during a stream job.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 49 +
1 file changed, 49 insertions(+)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 1f858bb..ecbd1f8 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -57,6 +57,37 @@ static BlockDriverState *get_base_by_name(BlockDriverState
*bs,
return base_bs;
}
+/*
+ * Returns 1 if the block is allocated in a node between
cor_filter_bs->file->bs
+ * and the base_bs in the backing chain. Otherwise, returns 0.
+ * The COR operation is allowed if the base_bs is not specified - return 1.
+ * Returns negative errno on failure.
+ */
+static int node_above_base(BlockDriverState *cor_filter_bs, uint64_t offset,
+ uint64_t bytes)
+{
+int ret;
+int64_t dummy;
+BlockDriverState *file = NULL;
+BDRVStateCOR *state = cor_filter_bs->opaque;
+
+if (!state->base_bs) {
+return 1;
+}
+
+ret = bdrv_block_status_above(cor_filter_bs->file->bs, state->base_bs,
+ offset, bytes, , NULL, );
+if (ret < 0) {
+return ret;
+}
+
+if (file) {
+return 1;
+}
+
+return 0;
+}
+
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -153,6 +184,12 @@ static int coroutine_fn
cor_co_pwritev_part(BlockDriverState *bs,
QEMUIOVector *qiov,
size_t qiov_offset, int flags)
{
+int ret = node_above_base(bs, offset, bytes);
+
+if (!ret || ret < 0) {
+return ret;
+}
+
return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
flags);
}
@@ -162,6 +199,12 @@ static int coroutine_fn
cor_co_pwrite_zeroes(BlockDriverState *bs,
int64_t offset, int bytes,
BdrvRequestFlags flags)
{
+int ret = node_above_base(bs, offset, bytes);
+
+if (!ret || ret < 0) {
+return ret;
+}
+
return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
}
@@ -178,6 +221,12 @@ static int coroutine_fn
cor_co_pwritev_compressed(BlockDriverState *bs,
uint64_t bytes,
QEMUIOVector *qiov)
{
+int ret = node_above_base(bs, offset, bytes);
+
+if (!ret || ret < 0) {
+return ret;
+}
+
return bdrv_co_pwritev(bs->file, offset, bytes, qiov,
BDRV_REQ_WRITE_COMPRESSED);
}
--
1.8.3.1
[PATCH v8 6/7] block-stream: freeze link to base node during stream job
To keep the base node unchanged during the block-stream operation,
freeze it as the other part of the backing chain with the intermediate
nodes related to the job.
This patch revers the change made with the commit c624b015bf as the
correct base file name and its format have to be written down to the
QCOW2 header on the disk when the backing file is being changed after
the stream job completes.
This reversion incurs changes in the tests 030, 245 and discards the
test 258 where concurrent stream/commit jobs are tested. When the link
to a base node is frozen, the concurrent job cannot change the common
backing chain.
Signed-off-by: Andrey Shinkevich
---
block/stream.c | 29 ++--
tests/qemu-iotests/030 | 10 +--
tests/qemu-iotests/245 | 2 +-
tests/qemu-iotests/258 | 161 -
tests/qemu-iotests/258.out | 33 --
5 files changed, 14 insertions(+), 221 deletions(-)
delete mode 100755 tests/qemu-iotests/258
delete mode 100644 tests/qemu-iotests/258.out
diff --git a/block/stream.c b/block/stream.c
index 8bf6b6d..fee4117 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -32,7 +32,7 @@ enum {
typedef struct StreamBlockJob {
BlockJob common;
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
-BlockDriverState *above_base; /* Node directly above the base */
+BlockDriverState *base; /* The base node */
BlockdevOnError on_error;
char *backing_file_str;
bool bs_read_only;
@@ -54,7 +54,7 @@ static void stream_abort(Job *job)
if (s->chain_frozen) {
BlockJob *bjob = >common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
+bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->base);
}
}
@@ -64,11 +64,11 @@ static int stream_prepare(Job *job)
BlockJob *bjob = >common;
BlockDriverState *bs = blk_bs(bjob->blk);
BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
-BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+BlockDriverState *base = s->base;
Error *local_err = NULL;
int ret = 0;
-bdrv_unfreeze_backing_chain(bs, s->above_base);
+bdrv_unfreeze_backing_chain(bs, s->base);
s->chain_frozen = false;
if (bdrv_cow_child(unfiltered_bs)) {
@@ -230,7 +230,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
bool bs_read_only;
int basic_flags = BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED;
BlockDriverState *base_overlay = bdrv_find_overlay(bs, base);
-BlockDriverState *above_base;
if (!base_overlay) {
error_setg(errp, "'%s' is not in the backing chain of '%s'",
@@ -238,20 +237,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
return;
}
-/*
- * Find the node directly above @base. @base_overlay is a COW overlay, so
- * it must have a bdrv_cow_child(), but it is the immediate overlay of
- * @base, so between the two there can only be filters.
- */
-above_base = base_overlay;
-if (bdrv_cow_bs(above_base) != base) {
-above_base = bdrv_cow_bs(above_base);
-while (bdrv_filter_bs(above_base) != base) {
-above_base = bdrv_filter_bs(above_base);
-}
-}
-
-if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
+if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
return;
}
@@ -284,7 +270,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* above_base node might change after the call to
* bdrv_reopen_set_read_only() due to parallel block jobs running.
*/
-base = bdrv_filter_or_cow_bs(above_base);
for (iter = bdrv_filter_or_cow_bs(bs); iter != base;
iter = bdrv_filter_or_cow_bs(iter))
{
@@ -293,7 +278,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
}
s->base_overlay = base_overlay;
-s->above_base = above_base;
+s->base = base;
s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
@@ -307,5 +292,5 @@ fail:
if (bs_read_only) {
bdrv_reopen_set_read_only(bs, true, NULL);
}
-bdrv_unfreeze_backing_chain(bs, above_base);
+bdrv_unfreeze_backing_chain(bs, base);
}
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1cdd7e2..c565e76 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -382,7 +382,7 @@ class TestParallelOps(iotests.QMPTestCase):
# Stream from node2 into node4
result = self.vm.qmp('block-stream', device='node4',
base_node='node2', job_id='node4')
self.assert_qmp(result, 'error/desc',
-"Cannot freeze 'backing' link to 'commit-filter'")
+"Cannot change 'backing' link from 'commit-filter' to 'node2'")
result = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
self.assert_qmp(result, 'return', {})
@@
[PATCH v8 3/7] qapi: add filter-node-name to block-stream
Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job.
Signed-off-by: Andrey Shinkevich
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/monitor/block-hmp-cmds.c | 4 ++--
block/stream.c | 4 +++-
blockdev.c | 4 +++-
include/block/block_int.h | 7 ++-
qapi/block-core.json | 6 ++
5 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4d3db5e..4e66775 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -507,8 +507,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
false, NULL, qdict_haskey(qdict, "speed"), speed, true,
- BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
- );
+ BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
false,
+ false, );
hmp_handle_error(mon, error);
}
diff --git a/block/stream.c b/block/stream.c
index b9c1141..8bf6b6d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,7 +221,9 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp)
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp)
{
StreamBlockJob *s;
BlockDriverState *iter;
diff --git a/blockdev.c b/blockdev.c
index 237fffb..cc531cb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2476,6 +2476,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
+ bool has_filter_node_name, const char *filter_node_name,
bool has_auto_finalize, bool auto_finalize,
bool has_auto_dismiss, bool auto_dismiss,
Error **errp)
@@ -2558,7 +2559,8 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
}
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- job_flags, has_speed ? speed : 0, on_error, _err);
+ job_flags, has_speed ? speed : 0, on_error,
+ filter_node_name, _err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 465a601..3efde33 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1122,6 +1122,9 @@ int is_windows_drive(const char *filename);
* See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
+ * @filter_node_name: The node name that should be assigned to the filter
+ * driver that the commit job inserts into the graph above @bs. NULL means
+ * that a node name should be autogenerated.
* @errp: Error object.
*
* Start a streaming operation on @bs. Clusters that are unallocated
@@ -1134,7 +1137,9 @@ int is_windows_drive(const char *filename);
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
int creation_flags, int64_t speed,
- BlockdevOnError on_error, Error **errp);
+ BlockdevOnError on_error,
+ const char *filter_node_name,
+ Error **errp);
/**
* commit_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0b8ccd3..e5ccf8a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2524,6 +2524,11 @@
#'stop' and 'enospc' can only be used if the block device
#supports io-status (see BlockInfo). Since 1.3.
#
+# @filter-node-name: the node name that should be assigned to the
+#filter driver that the stream job inserts into the graph
+#above @device. If this option is not given, a node name is
+#autogenerated. (Since: 5.2)
+#
# @auto-finalize: When false, this job will wait in a PENDING state after it
has
# finished its work, waiting for @block-job-finalize before
# making any block graph changes.
@@ -2554,6 +2559,7 @@
'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
'*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
'*on-error': 'BlockdevOnError',
+
[PATCH v8 4/7] copy-on-read: pass base file name to COR driver
To limit the guest's COR operations by the base node in the backing
chain during stream job, pass the base file name to the copy-on-read
driver. The rest of the functionality will be implemented in the patch
that follows.
Signed-off-by: Andrey Shinkevich
---
block/copy-on-read.c | 41 -
block/copy-on-read.h | 1 +
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 0ede7aa..1f858bb 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,19 +24,45 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi/error.h"
+#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qdict.h"
#include "block/copy-on-read.h"
typedef struct BDRVStateCOR {
bool active;
+BlockDriverState *base_bs;
} BDRVStateCOR;
+/*
+ * Non-zero pointers are the caller's responsibility.
+ */
+static BlockDriverState *get_base_by_name(BlockDriverState *bs,
+ const char *base_name, Error **errp)
+{
+BlockDriverState *base_bs = NULL;
+AioContext *aio_context;
+
+base_bs = bdrv_find_backing_image(bs, base_name);
+if (base_bs == NULL) {
+error_setg(errp, QERR_BASE_NOT_FOUND, base_name);
+return NULL;
+}
+
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+assert(bdrv_get_aio_context(base_bs) == aio_context);
+aio_context_release(aio_context);
+
+return base_bs;
+}
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BlockDriverState *base_bs = NULL;
BDRVStateCOR *state = bs->opaque;
+const char *base_name = qdict_get_try_str(options, "base");
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -52,7 +78,15 @@ static int cor_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+if (base_name) {
+qdict_del(options, "base");
+base_bs = get_base_by_name(bs, base_name, errp);
+if (!base_bs) {
+return -EINVAL;
+}
+}
state->active = true;
+state->base_bs = base_bs;
/*
* We don't need to call bdrv_child_refresh_perms() now as the permissions
@@ -190,6 +224,7 @@ static void bdrv_copy_on_read_init(void)
static BlockDriverState *create_filter_node(BlockDriverState *bs,
+const char *base_name,
const char *filter_node_name,
Error **errp)
{
@@ -197,6 +232,9 @@ static BlockDriverState
*create_filter_node(BlockDriverState *bs,
qdict_put_str(opts, "driver", "copy-on-read");
qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+if (base_name) {
+qdict_put_str(opts, "base", base_name);
+}
if (filter_node_name) {
qdict_put_str(opts, "node-name", filter_node_name);
}
@@ -206,13 +244,14 @@ static BlockDriverState
*create_filter_node(BlockDriverState *bs,
BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *base_name,
const char *filter_node_name,
Error **errp)
{
BlockDriverState *cor_filter_bs;
Error *local_err = NULL;
-cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
+cor_filter_bs = create_filter_node(bs, base_name, filter_node_name, errp);
if (cor_filter_bs == NULL) {
error_prepend(errp, "Could not create filter node: ");
return NULL;
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
index 1686e4e..6a7c8bb 100644
--- a/block/copy-on-read.h
+++ b/block/copy-on-read.h
@@ -28,6 +28,7 @@
#include "block/block_int.h"
BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *base_name,
const char *filter_node_name,
Error **errp);
void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
--
1.8.3.1
[PATCH v8 1/7] copy-on-read: Support preadv/pwritev_part functions
Add support for the recently introduced functions
bdrv_co_preadv_part()
and
bdrv_co_pwritev_part()
to the COR-filter driver.
Signed-off-by: Andrey Shinkevich
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/copy-on-read.c | 28
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 2816e61..cb03e0f 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -74,21 +74,25 @@ static int64_t cor_getlength(BlockDriverState *bs)
}
-static int coroutine_fn cor_co_preadv(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset,
+ int flags)
{
-return bdrv_co_preadv(bs->file, offset, bytes, qiov,
- flags | BDRV_REQ_COPY_ON_READ);
+return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+ flags | BDRV_REQ_COPY_ON_READ);
}
-static int coroutine_fn cor_co_pwritev(BlockDriverState *bs,
- uint64_t offset, uint64_t bytes,
- QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_pwritev_part(BlockDriverState *bs,
+uint64_t offset,
+uint64_t bytes,
+QEMUIOVector *qiov,
+size_t qiov_offset, int flags)
{
-
-return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
+return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+flags);
}
@@ -137,8 +141,8 @@ static BlockDriver bdrv_copy_on_read = {
.bdrv_getlength = cor_getlength,
-.bdrv_co_preadv = cor_co_preadv,
-.bdrv_co_pwritev= cor_co_pwritev,
+.bdrv_co_preadv_part= cor_co_preadv_part,
+.bdrv_co_pwritev_part = cor_co_pwritev_part,
.bdrv_co_pwrite_zeroes = cor_co_pwrite_zeroes,
.bdrv_co_pdiscard = cor_co_pdiscard,
.bdrv_co_pwritev_compressed = cor_co_pwritev_compressed,
--
1.8.3.1
[PATCH v8 2/7] copy-on-read: add filter append/drop functions
Provide API for the COR-filter insertion/removal.
Also, drop the filter child permissions for an inactive state when the
filter node is being removed.
Signed-off-by: Andrey Shinkevich
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/copy-on-read.c | 104 +++
block/copy-on-read.h | 35 +
2 files changed, 139 insertions(+)
create mode 100644 block/copy-on-read.h
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index cb03e0f..0ede7aa 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -23,11 +23,21 @@
#include "qemu/osdep.h"
#include "block/block_int.h"
#include "qemu/module.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "block/copy-on-read.h"
+
+
+typedef struct BDRVStateCOR {
+bool active;
+} BDRVStateCOR;
static int cor_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
+BDRVStateCOR *state = bs->opaque;
+
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
false, errp);
@@ -42,6 +52,13 @@ static int cor_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
+state->active = true;
+
+/*
+ * We don't need to call bdrv_child_refresh_perms() now as the permissions
+ * will be updated later when the filter node gets its parent.
+ */
+
return 0;
}
@@ -57,6 +74,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild
*c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
{
+BDRVStateCOR *s = bs->opaque;
+
+if (!s->active) {
+/*
+ * While the filter is being removed
+ */
+*nperm = 0;
+*nshared = BLK_PERM_ALL;
+return;
+}
+
*nperm = perm & PERM_PASSTHROUGH;
*nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
@@ -135,6 +163,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool
locked)
static BlockDriver bdrv_copy_on_read = {
.format_name= "copy-on-read",
+.instance_size = sizeof(BDRVStateCOR),
.bdrv_open = cor_open,
.bdrv_child_perm= cor_child_perm,
@@ -159,4 +188,79 @@ static void bdrv_copy_on_read_init(void)
bdrv_register(_copy_on_read);
}
+
+static BlockDriverState *create_filter_node(BlockDriverState *bs,
+const char *filter_node_name,
+Error **errp)
+{
+QDict *opts = qdict_new();
+
+qdict_put_str(opts, "driver", "copy-on-read");
+qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+if (filter_node_name) {
+qdict_put_str(opts, "node-name", filter_node_name);
+}
+
+return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
+}
+
+
+BlockDriverState *bdrv_cor_filter_append(BlockDriverState *bs,
+ const char *filter_node_name,
+ Error **errp)
+{
+BlockDriverState *cor_filter_bs;
+Error *local_err = NULL;
+
+cor_filter_bs = create_filter_node(bs, filter_node_name, errp);
+if (cor_filter_bs == NULL) {
+error_prepend(errp, "Could not create filter node: ");
+return NULL;
+}
+
+if (!filter_node_name) {
+cor_filter_bs->implicit = true;
+}
+
+bdrv_drained_begin(bs);
+bdrv_replace_node(bs, cor_filter_bs, _err);
+bdrv_drained_end(bs);
+
+if (local_err) {
+bdrv_unref(cor_filter_bs);
+error_propagate(errp, local_err);
+return NULL;
+}
+
+return cor_filter_bs;
+}
+
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
+{
+BdrvChild *child;
+BlockDriverState *bs;
+BDRVStateCOR *s = cor_filter_bs->opaque;
+
+child = bdrv_filter_child(cor_filter_bs);
+if (!child) {
+return;
+}
+bs = child->bs;
+
+/* Retain the BDS until we complete the graph change. */
+bdrv_ref(bs);
+/* Hold a guest back from writing while permissions are being reset. */
+bdrv_drained_begin(bs);
+/* Drop permissions before the graph change. */
+s->active = false;
+bdrv_child_refresh_perms(cor_filter_bs, child, _abort);
+bdrv_replace_node(cor_filter_bs, bs, _abort);
+
+bdrv_drained_end(bs);
+bdrv_unref(bs);
+bdrv_unref(cor_filter_bs);
+}
+
+
block_init(bdrv_copy_on_read_init);
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
new file mode 100644
index 000..1686e4e
--- /dev/null
+++ b/block/copy-on-read.h
@@ -0,0 +1,35 @@
+/*
+ * Copy-on-read filter block driver
+ *
+ * The filter driver performs Copy-On-Read (COR) operations
+ *
+ * Copyright (c)
[PATCH v8 0/7] Apply COR-filter to the block-stream permanently
Note: this series is based on the another one "block: Deal with filters" by Max Reitz that could be found in the branches: https://git.xanclic.moe/XanClic/qemu child-access-functions-v6 https://github.com/XanClic/qemu child-access-functions-v6 v8: 03: qapi - version changed to 'Since: 5.2'. 04: New. 05: New. 06: New. 07: The extra bs variable and the enable_cor were dropped. The v7 Message-Id: <1598257914-887267-1-git-send-email-andrey.shinkev...@virtuozzo.com> Andrey Shinkevich (7): copy-on-read: Support preadv/pwritev_part functions copy-on-read: add filter append/drop functions qapi: add filter-node-name to block-stream copy-on-read: pass base file name to COR driver copy-on-read: limit guest writes to base in COR driver block-stream: freeze link to base node during stream job block: apply COR-filter to block-stream jobs block/copy-on-read.c | 218 ++--- block/copy-on-read.h | 36 +++ block/monitor/block-hmp-cmds.c | 4 +- block/stream.c | 105 ++-- blockdev.c | 4 +- include/block/block_int.h | 7 +- qapi/block-core.json | 6 ++ tests/qemu-iotests/030 | 60 +++- tests/qemu-iotests/030.out | 4 +- tests/qemu-iotests/245 | 21 ++-- tests/qemu-iotests/258 | 161 -- tests/qemu-iotests/258.out | 33 --- 12 files changed, 341 insertions(+), 318 deletions(-) create mode 100644 block/copy-on-read.h delete mode 100755 tests/qemu-iotests/258 delete mode 100644 tests/qemu-iotests/258.out -- 1.8.3.1
QEMU build error with --disable-qcow1
Hi, if I run "configure" with --disable-qcow1, I currently get a build error: Linking target qemu-nbd libblock.fa(block_qcow2-threads.c.o): In function `qcow2_zlib_compress': /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:101: undefined reference to `deflateInit2_' /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:116: undefined reference to `deflate' /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:123: undefined reference to `deflateEnd' libblock.fa(block_qcow2-threads.c.o): In function `qcow2_zlib_decompress': /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:152: undefined reference to `inflateInit2_' /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:157: undefined reference to `inflate' /tmp/qemu-test/../../home/thuth/devel/qemu/block/qcow2-threads.c:169: undefined reference to `inflateEnd' etc. Not sure whether this is due to the recent conversion to meson, or a recent change to that file ... anybody got a clue what's going on here? Thomas
Re: [PATCH v7 4/4] block: apply COR-filter to block-stream jobs
On 24.08.2020 14:30, Vladimir Sementsov-Ogievskiy wrote:
24.08.2020 11:31, Andrey Shinkevich wrote:
This patch completes the series with the COR-filter insertion for
block-stream operations. Adding the filter makes it possible for copied
regions to be discarded in backing files during the block-stream job,
what will reduce the disk overuse.
The COR-filter insertion incurs changes in the iotests case
245:test_block_stream_4 that reopens the backing chain during a
block-stream job. There are changes in the iotests #030 as well.
The iotests case 030:test_stream_parallel was deleted due to multiple
conflicts between the concurrent job operations over the same backing
chain. The base backing node for one job is the top node for another
job. It may change due to the filter node inserted into the backing
chain while both jobs are running. Another issue is that the parts of
the backing chain are being frozen by the running job and may not be
changed by the concurrent job when needed. The concept of the parallel
jobs with common nodes is considered vital no more.
Signed-off-by: Andrey Shinkevich
---
block/stream.c | 58
+++---
tests/qemu-iotests/030 | 50
---
tests/qemu-iotests/030.out | 4 ++--
tests/qemu-iotests/245 | 19 +++
4 files changed, 65 insertions(+), 66 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 8bf6b6d..e927fed 100644
--- a/block/stream.c
+++ b/block/stream.c
...
@@ -307,5 +332,10 @@ fail:
if (bs_read_only) {
bdrv_reopen_set_read_only(bs, true, NULL);
}
- bdrv_unfreeze_backing_chain(bs, above_base);
+ if () {
+ bdrv_unfreeze_backing_chain(cor_filter_bs, above_base);
+ bdrv_cor_filter_drop(cor_filter_bs);
+ } else {
+ bdrv_unfreeze_backing_chain(bs, above_base);
as I see, in this case chain is not yet frozen
When cor_filter_bs is NULL, only bs - above_base chain is frozen at this
point.
Andrey
+ }
}
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1cdd7e2..fec9d89 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
...
Re: [PATCH v7 1/8] Introduce yank feature
On Thu, 27 Aug 2020 14:37:00 +0200
Markus Armbruster wrote:
> I apologize for not reviewing this much earlier.
>
> Lukas Straub writes:
>
> > The yank feature allows to recover from hanging qemu by "yanking"
> > at various parts. Other qemu systems can register themselves and
> > multiple yank functions. Then all yank functions for selected
> > instances can be called by the 'yank' out-of-band qmp command.
> > Available instances can be queried by a 'query-yank' oob command.
> >
> > Signed-off-by: Lukas Straub
> > Acked-by: Stefan Hajnoczi
> > ---
> > include/qemu/yank.h | 80 +++
> > qapi/misc.json | 45 +++
> > util/Makefile.objs | 1 +
> > util/yank.c | 184
> > 4 files changed, 310 insertions(+)
> > create mode 100644 include/qemu/yank.h
> > create mode 100644 util/yank.c
> >
> > diff --git a/include/qemu/yank.h b/include/qemu/yank.h
> > new file mode 100644
> > index 00..cd184fcd05
> > --- /dev/null
> > +++ b/include/qemu/yank.h
> > @@ -0,0 +1,80 @@
> > +/*
> > + * QEMU yank feature
> > + *
> > + * Copyright (c) Lukas Straub
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef YANK_H
> > +#define YANK_H
> > +
> > +typedef void (YankFn) (void *opaque);
>
> No space before parameter lists, please.
Ok, I will fix this in the next version.
> > +
> > +/**
> > + * yank_register_instance: Register a new instance.
> > + *
> > + * This registers a new instance for yanking. Must be called before any
> > yank
> > + * function is registered for this instance.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The globally unique name of the instance.
> > + * @errp: ...
> > + */
> > +void yank_register_instance(const char *instance_name, Error **errp);
> > +
> > +/**
> > + * yank_unregister_instance: Unregister a instance.
> > + *
> > + * This unregisters a instance. Must be called only after every yank
> > function
> > + * of the instance has been unregistered.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance.
> > + */
> > +void yank_unregister_instance(const char *instance_name);
> > +
> > +/**
> > + * yank_register_function: Register a yank function
> > + *
> > + * This registers a yank function. All limitations of qmp oob commands
> > apply
> > + * to the yank function as well.
>
> The restrictions are documented in docs/devel/qapi-code-gen.txt under
> "An OOB-capable command handler must satisfy the following conditions".
> Let's point there,
I will fix this in the next version.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance
> > + * @func: The yank function
> > + * @opaque: Will be passed to the yank function
> > + */
> > +void yank_register_function(const char *instance_name,
> > +YankFn *func,
> > +void *opaque);
>
> Pardon my ignorance... can you explain to me why a yank instance may
> have multiple functions?
>
> > +
> > +/**
> > + * yank_unregister_function: Unregister a yank function
> > + *
> > + * This unregisters a yank function.
> > + *
> > + * This function is thread-safe.
> > + *
> > + * @instance_name: The name of the instance
> > + * @func: func that was passed to yank_register_function
> > + * @opaque: opaque that was passed to yank_register_function
> > + */
> > +void yank_unregister_function(const char *instance_name,
> > + YankFn *func,
> > + void *opaque);
> > +
> > +/**
> > + * yank_unregister_function: Generic yank function for iochannel
>
> Pasto, should be
>
> * yank_generic_iochannel: ...
I will fix this in the next version.
> > + *
> > + * This is a generic yank function which will call qio_channel_shutdown on
> > the
> > + * provided QIOChannel.
> > + *
> > + * @opaque: QIOChannel to shutdown
> > + */
> > +void yank_generic_iochannel(void *opaque);
> > +#endif
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index 9d32820dc1..0d6a8f20b7 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -1615,3 +1615,48 @@
> > ##
> > { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
> >
> > +##
> > +# @YankInstances:
> > +#
> > +# @instances: List of yank instances.
> > +#
> > +# Yank instances are named after the following schema:
> > +# "blockdev:", "chardev:" and "migration"
> > +#
> > +# Since: 5.1
> > +##
> > +{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } }
>
> I'm afraid this is a problematic QMP interface.
>
> By making YankInstances a struct, you keep the door open to adding more
> members, which is good.
>
> But by making its 'instances' member a ['str'], you close the door to
> using anything but a single string for the individual
[PATCH v2 5/7] qemu: Block migration when transient disk option is enabled
From: Masayoshi Mizuma
Block migration when transient disk option is enabled because migration
requires some blockjobs.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_migration.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0f2f92b211..6fcf5a3a07 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2949,6 +2949,22 @@ qemuMigrationDstPrepareDirect(virQEMUDriverPtr driver,
}
+static bool
+qemuMigrationTransientDiskExists(virDomainDefPtr def)
+{
+size_t i;
+
+for (i = 0; i < def->ndisks; i++) {
+ virDomainDiskDefPtr disk = def->disks[i];
+
+ if (disk->transient)
+return true;
+}
+
+return false;
+}
+
+
virDomainDefPtr
qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver,
virQEMUCapsPtr qemuCaps,
@@ -2971,6 +2987,12 @@ qemuMigrationAnyPrepareDef(virQEMUDriverPtr driver,
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
goto cleanup;
+/*
+ * transient disk option is a blocker for migration
+ */
+if (qemuMigrationTransientDiskExists(def))
+ goto cleanup;
+
if (dname) {
name = def->name;
def->name = g_strdup(dname);
--
2.27.0
[PATCH v2 6/7] qemu: Block disk hotplug when transient disk option is enabled
From: Masayoshi Mizuma
Block disk hotplug when transient disk option is enabled so far.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_hotplug.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2c6c30ce03..1c1b6c3acf 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1031,6 +1031,12 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriverPtr
driver,
return -1;
}
+if (disk->transient) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("transient disk hotplug isn't supported"));
+return -1;
+}
+
if (virDomainDiskTranslateSourcePool(disk) < 0)
goto cleanup;
--
2.27.0
[PATCH v2 7/7] qemu: Block blockjobs when transient disk option is enabled
From: Masayoshi Mizuma
Block blockjobs when transient disk option is enabled so far.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_domain.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index e28f704dba..98a52e5476 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10678,6 +10678,13 @@ qemuDomainDiskBlockJobIsSupported(virDomainObjPtr vm,
return false;
}
+if (disk->transient) {
+virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
+ _("block jobs are not supported on transient disk
'%s'"),
+ disk->dst);
+return false;
+}
+
return true;
}
--
2.27.0
[PATCH v2 1/7] qemuSnapshotDiskPrepareOne: Get available even if snapdisk is NULL
From: Masayoshi Mizuma Get available even if snapdisk argument is NULL at qemuSnapshotDiskPrepareOne() so that the caller can setup dd->src. Signed-off-by: Masayoshi Mizuma --- src/qemu/qemu_snapshot.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1e8ea80b22..d310e6ff02 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -953,8 +953,9 @@ qemuSnapshotDiskPrepareOne(virQEMUDriverPtr driver, if (qemuDomainStorageSourceValidateDepth(disk->src, 1, disk->dst) < 0) return -1; -if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) -return -1; +if (snapdisk) +if (!(dd->src = virStorageSourceCopy(snapdisk->src, false))) +return -1; if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0) return -1; -- 2.27.0
[PATCH v2 0/7] qemu: implementation of transient disk option
This patchset tries to implement transient option for qcow2 and raw format disk. This uses the snapshot cleanup codes: https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html It gets user available to set to the domain xml file like as: Any changes which the Guest does to the disk is dropped when the Guest is shutdowned. There are some limitations for transient disk option so far: - Supported disk format is qcow2 and raw - blockdev capability is required for qemu - Following features are blocked with transient disk option - blockjobs - Migration - Disk hotplug Masayoshi Mizuma (7): qemuSnapshotDiskPrepareOne: Get available even if snapdisk is NULL qemu: Introduce functions to handle transient disk qemu: Add transient disk handler to start and stop the guest qemu: Transient option gets avaiable for qcow2 and raw format disk qemu: Block blockjobs when transient disk option is enabled qemu: Block migration when transient disk option is enabled qemu: Block disk hotplug when transient disk option is enabled src/qemu/qemu_domain.c| 7 ++ src/qemu/qemu_hotplug.c | 6 ++ src/qemu/qemu_migration.c | 22 ++ src/qemu/qemu_process.c | 8 +++ src/qemu/qemu_snapshot.c | 139 +- src/qemu/qemu_snapshot.h | 8 +++ src/qemu/qemu_validate.c | 9 ++- src/util/virstoragefile.h | 2 + 8 files changed, 196 insertions(+), 5 deletions(-) -- 2.27.0
[PATCH v2 3/7] qemu: Add transient disk handler to start and stop the guest
From: Masayoshi Mizuma
The transient disk is attached before the guest starts.
Remove the transient disk when the guest does shutdown.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_process.c | 8
1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 126fabf5ef..5753258135 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -60,6 +60,7 @@
#include "qemu_firmware.h"
#include "qemu_backup.h"
#include "qemu_dbus.h"
+#include "qemu_snapshot.h"
#include "cpu/cpu.h"
#include "cpu/cpu_x86.h"
@@ -7000,6 +7001,10 @@ qemuProcessLaunch(virConnectPtr conn,
qemuProcessAutoDestroyAdd(driver, vm, conn) < 0)
goto cleanup;
+VIR_DEBUG("Setting up transient disk");
+if (qemuTransientCreatetDisk(driver, vm, asyncJob) < 0)
+goto cleanup;
+
ret = 0;
cleanup:
@@ -7636,6 +7641,9 @@ void qemuProcessStop(virQEMUDriverPtr driver,
}
qemuBlockRemoveImageMetadata(driver, vm, disk->dst, disk->src);
+
+if (disk->transient)
+qemuTransientRemoveDisk(disk);
}
}
--
2.27.0
[PATCH v2 2/7] qemu: Introduce functions to handle transient disk
From: Masayoshi Mizuma
Here is the implementation of transient option for qcow2 and raw format
disk. This gets available directive in domain xml file
like as:
When the qemu command line options are built, a new qcow2 image is
created with backing qcow2 by using blockdev-snapshot command.
The backing image is the qcow2 file which is set as .
The filename of the new qcow2 image is original-source-file.TRANSIENT.
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_snapshot.c | 134 ++
src/qemu/qemu_snapshot.h | 8 +++
src/util/virstoragefile.h | 2 +
3 files changed, 144 insertions(+)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index d310e6ff02..5c61d19f26 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -2265,3 +2265,137 @@ qemuSnapshotDelete(virDomainObjPtr vm,
cleanup:
return ret;
}
+
+static int
+qemuTransientDiskPrepareOne(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+qemuSnapshotDiskDataPtr data,
+virHashTablePtr blockNamedNodeData,
+int asyncJob,
+virJSONValuePtr actions)
+{
+int rc = -1;
+virStorageSourcePtr dest;
+virStorageSourcePtr src = data->disk->src;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver);
+
+if (!strlen(src->path))
+return rc;
+
+if (!(dest = virStorageSourceNew()))
+return rc;
+
+dest->path = g_strdup_printf("%s.TRANSIENT", src->path);
+
+if (virFileExists(dest->path)) {
+virReportError(VIR_ERR_INVALID_ARG,
+ _("Transient disk '%s' for '%s' exists"),
+ dest->path, src->path);
+goto cleanup;
+}
+
+dest->type = VIR_STORAGE_TYPE_FILE;
+dest->format = VIR_STORAGE_FILE_QCOW2;
+data->src = dest;
+
+if (qemuSnapshotDiskPrepareOne(driver, vm, cfg, data->disk,
+ NULL, data, blockNamedNodeData,
+ false, true, asyncJob, actions) < 0)
+goto cleanup;
+
+rc = 0;
+ cleanup:
+if (rc < 0)
+g_free(dest->path);
+
+return rc;
+}
+
+static int
+qemuWaitTransaction(virQEMUDriverPtr driver,
+virDomainObjPtr vm,
+int asyncJob,
+virJSONValuePtr *actions)
+{
+qemuDomainObjPrivatePtr priv = vm->privateData;
+
+if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
+return -1;
+
+if (qemuMonitorTransaction(priv->mon, actions) < 0)
+return -1;
+
+if (qemuDomainObjExitMonitor(driver, vm) < 0)
+return -1;
+
+return 0;
+}
+
+int
+qemuTransientCreatetDisk(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ int asyncJob)
+{
+size_t i;
+int rc = -1;
+size_t ndata = 0;
+qemuSnapshotDiskDataPtr data = NULL;
+g_autoptr(virJSONValue) actions = NULL;
+qemuDomainObjPrivatePtr priv = vm->privateData;
+g_autoptr(virHashTable) blockNamedNodeData = NULL;
+bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV);
+
+if (!blockdev)
+return rc;
+
+if (VIR_ALLOC_N(data, vm->def->ndisks) < 0)
+return rc;
+
+if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob)))
+goto cleanup;
+
+if (!(actions = virJSONValueNewArray()))
+goto cleanup;
+
+for (i = 0; i < vm->def->ndisks; i++) {
+virDomainDiskDefPtr disk = vm->def->disks[i];
+
+if (disk->src->readonly)
+continue;
+
+if (disk->transient) {
+data[ndata].disk = disk;
+if (qemuTransientDiskPrepareOne(driver, vm, [ndata],
blockNamedNodeData,
+ asyncJob, actions) < 0)
+goto cleanup;
+ndata++;
+}
+}
+
+if (qemuWaitTransaction(driver, vm, asyncJob, ) < 0)
+goto cleanup;
+
+for (i = 0; i < ndata; i++) {
+qemuSnapshotDiskDataPtr dd = [i];
+
+qemuSnapshotDiskUpdateSource(driver, vm, dd, blockdev);
+dd->disk->src->transientEstablished = true;
+}
+
+VIR_FREE(data);
+rc = 0;
+ cleanup:
+qemuSnapshotDiskCleanup(data, vm->def->ndisks, driver, vm, asyncJob);
+
+return rc;
+}
+
+void
+qemuTransientRemoveDisk(virDomainDiskDefPtr disk)
+{
+if (disk->src->transientEstablished) {
+VIR_DEBUG("unlink transient disk: %s", disk->src->path);
+unlink(disk->src->path);
+}
+}
diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h
index 8b3ebe87b1..aecb1762d2 100644
--- a/src/qemu/qemu_snapshot.h
+++ b/src/qemu/qemu_snapshot.h
@@ -53,3 +53,11 @@ int
qemuSnapshotDelete(virDomainObjPtr vm,
virDomainSnapshotPtr
[PATCH v2 4/7] qemu: Transient option gets avaiable for qcow2 and raw format disk
From: Masayoshi Mizuma
Signed-off-by: Masayoshi Mizuma
---
src/qemu/qemu_validate.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 488f258d00..82818a4fdc 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -2166,9 +2166,12 @@ qemuValidateDomainDeviceDefDiskFrontend(const
virDomainDiskDef *disk,
}
if (disk->transient) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("transient disks not supported yet"));
-return -1;
+if ((disk->src->format != VIR_STORAGE_FILE_QCOW2) &&
+(disk->src->format != VIR_STORAGE_FILE_RAW)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("transient disks not supported yet"));
+return -1;
+}
}
if (disk->iomode == VIR_DOMAIN_DISK_IO_NATIVE &&
--
2.27.0
Re: [PATCH] qcow2: Use macros for the L1, refcount and bitmap table entry sizes
Patchew URL: https://patchew.org/QEMU/20200828110828.13833-1-be...@igalia.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200828110828.13833-1-be...@igalia.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[PATCH] qcow2: Use macros for the L1, refcount and bitmap table entry sizes
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
---
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 996b3314f4..5b58720607 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);
Re: [PATCH v2 1/2] util/hexdump: Convert to take a void pointer argument
Philippe Mathieu-Daudé 于2020年8月23日周日 上午2:11写道：
>
> Most uses of qemu_hexdump() do not take an array of char
> as input, forcing use of cast. Since we can use this
> helper to dump any kind of buffer, use a pointer to void
> argument instead.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Li Qiang
> ---
> Since v1:
> - renamed argument 'bufptr' (Peter Maydell)
> ---
> include/qemu-common.h| 3 ++-
> hw/dma/xlnx_dpdma.c | 2 +-
> hw/net/fsl_etsec/etsec.c | 2 +-
> hw/sd/sd.c | 2 +-
> hw/usb/redirect.c| 2 +-
> net/colo-compare.c | 12 ++--
> net/net.c| 2 +-
> util/hexdump.c | 4 +++-
> 8 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index bb9496bd80f..6834883822f 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -138,7 +138,8 @@ int os_parse_cmd_args(int index, const char *optarg);
> * Hexdump a buffer to a file. An optional string prefix is added to every
> line
> */
>
> -void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t
> size);
> +void qemu_hexdump(const void *bufptr, FILE *fp,
> + const char *prefix, size_t size);
>
> /*
> * helper to parse debug environment variables
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index b40c897de2c..d75a8069426 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor
> *desc)
> {
> if (DEBUG_DPDMA) {
> qemu_log("DUMP DESCRIPTOR:\n");
> -qemu_hexdump((char *)desc, stdout, "", sizeof(DPDMADescriptor));
> +qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
> }
> }
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 7035cf4eb97..c817a28decd 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>
> #if defined(HEX_DUMP)
> fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
> -qemu_hexdump((void *)buf, stderr, "", size);
> +qemu_hexdump(buf, stderr, "", size);
> #endif
> /* Flush is unnecessary as are already in receiving path */
> etsec->need_flush = false;
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index fad9cf1ee7a..190e4cf1232 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1781,7 +1781,7 @@ send_response:
> }
>
> #ifdef DEBUG_SD
> -qemu_hexdump((const char *)response, stderr, "Response", rsplen);
> +qemu_hexdump(response, stderr, "Response", rsplen);
> #endif
>
> return rsplen;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 417a60a2e68..09edb0d81c0 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const
> char *desc,
> if (dev->debug < usbredirparser_debug_data) {
> return;
> }
> -qemu_hexdump((char *)data, stderr, desc, len);
> +qemu_hexdump(data, stderr, desc, len);
> }
>
> /*
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 2c20de1537d..550272b3baa 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -494,9 +494,9 @@ sec:
> g_queue_push_head(>secondary_list, spkt);
>
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr,
> +qemu_hexdump(ppkt->data, stderr,
> "colo-compare ppkt", ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr,
> +qemu_hexdump(spkt->data, stderr,
> "colo-compare spkt", spkt->size);
> }
>
> @@ -535,9 +535,9 @@ static int colo_packet_compare_udp(Packet *spkt, Packet
> *ppkt)
> trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> +qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
> ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> +qemu_hexdump(spkt->data, stderr, "colo-compare sec pkt",
> spkt->size);
> }
> return -1;
> @@ -578,9 +578,9 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet
> *ppkt)
> trace_colo_compare_icmp_miscompare("Secondary pkt size",
> spkt->size);
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> +qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
>
Re: [PATCH v2 2/2] util/hexdump: Reorder qemu_hexdump() arguments
On Sat, Aug 22, 2020 at 08:09:50PM +0200, Philippe Mathieu-Daudé wrote:
> qemu_hexdump()'s pointer to the buffer and length of the
> buffer are closely related arguments but are widely separated
> in the argument list order (also, the format of
> function prototypes is usually to have the FILE* argument
> coming first).
>
> Reorder the arguments as "fp, prefix, buf, size" which is
> more logical.
>
> Suggested-by: Peter Maydell
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Edgar E. Iglesias
> ---
> include/qemu-common.h| 4 ++--
> hw/dma/xlnx_dpdma.c | 2 +-
> hw/net/fsl_etsec/etsec.c | 2 +-
> hw/net/fsl_etsec/rings.c | 2 +-
> hw/sd/sd.c | 2 +-
> hw/usb/redirect.c| 2 +-
> net/colo-compare.c | 24
> net/net.c| 2 +-
> util/hexdump.c | 4 ++--
> util/iov.c | 2 +-
> 10 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 6834883822f..9cfd62669bf 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -138,8 +138,8 @@ int os_parse_cmd_args(int index, const char *optarg);
> * Hexdump a buffer to a file. An optional string prefix is added to every
> line
> */
>
> -void qemu_hexdump(const void *bufptr, FILE *fp,
> - const char *prefix, size_t size);
> +void qemu_hexdump(FILE *fp, const char *prefix,
> + const void *bufptr, size_t size);
>
> /*
> * helper to parse debug environment variables
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index d75a8069426..967548abd31 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor
> *desc)
> {
> if (DEBUG_DPDMA) {
> qemu_log("DUMP DESCRIPTOR:\n");
> -qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
> +qemu_hexdump(stdout, "", desc, sizeof(DPDMADescriptor));
> }
> }
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index c817a28decd..c5edb25dc9f 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>
> #if defined(HEX_DUMP)
> fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
> -qemu_hexdump(buf, stderr, "", size);
> +qemu_hexdump(stderr, "", buf, size);
> #endif
> /* Flush is unnecessary as are already in receiving path */
> etsec->need_flush = false;
> diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> index 337a55fc957..628648a9c37 100644
> --- a/hw/net/fsl_etsec/rings.c
> +++ b/hw/net/fsl_etsec/rings.c
> @@ -269,7 +269,7 @@ static void process_tx_bd(eTSEC *etsec,
>
> #if defined(HEX_DUMP)
> qemu_log("eTSEC Send packet size:%d\n", etsec->tx_buffer_len);
> -qemu_hexdump(etsec->tx_buffer, stderr, "", etsec->tx_buffer_len);
> +qemu_hexdump(stderr, "", etsec->tx_buffer, etsec->tx_buffer_len);
> #endif /* ETSEC_RING_DEBUG */
>
> if (etsec->first_bd.flags & BD_TX_TOEUN) {
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 190e4cf1232..6508939b1f4 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1781,7 +1781,7 @@ send_response:
> }
>
> #ifdef DEBUG_SD
> -qemu_hexdump(response, stderr, "Response", rsplen);
> +qemu_hexdump(stderr, "Response", response, rsplen);
> #endif
>
> return rsplen;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 09edb0d81c0..48f38d33912 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const
> char *desc,
> if (dev->debug < usbredirparser_debug_data) {
> return;
> }
> -qemu_hexdump(data, stderr, desc, len);
> +qemu_hexdump(stderr, desc, data, len);
> }
>
> /*
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 550272b3baa..4a5ed642e9a 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -494,10 +494,10 @@ sec:
> g_queue_push_head(>secondary_list, spkt);
>
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump(ppkt->data, stderr,
> -"colo-compare ppkt", ppkt->size);
> -qemu_hexdump(spkt->data, stderr,
> -"colo-compare spkt", spkt->size);
> +qemu_hexdump(stderr, "colo-compare ppkt",
> + ppkt->data, ppkt->size);
> +qemu_hexdump(stderr, "colo-compare spkt",
> + spkt->data, spkt->size);
> }
>
> colo_compare_inconsistency_notify(s);
> @@ -535,10 +535,10 @@ static int colo_packet_compare_udp(Packet *spkt, Packet
> *ppkt)
> trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> trace_colo_compare_udp_miscompare("Secondary pkt
Re: [PATCH v2 1/2] util/hexdump: Convert to take a void pointer argument
On Sat, Aug 22, 2020 at 08:09:49PM +0200, Philippe Mathieu-Daudé wrote:
> Most uses of qemu_hexdump() do not take an array of char
> as input, forcing use of cast. Since we can use this
> helper to dump any kind of buffer, use a pointer to void
> argument instead.
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Edgar E. Iglesias
> ---
> Since v1:
> - renamed argument 'bufptr' (Peter Maydell)
> ---
> include/qemu-common.h| 3 ++-
> hw/dma/xlnx_dpdma.c | 2 +-
> hw/net/fsl_etsec/etsec.c | 2 +-
> hw/sd/sd.c | 2 +-
> hw/usb/redirect.c| 2 +-
> net/colo-compare.c | 12 ++--
> net/net.c| 2 +-
> util/hexdump.c | 4 +++-
> 8 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index bb9496bd80f..6834883822f 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -138,7 +138,8 @@ int os_parse_cmd_args(int index, const char *optarg);
> * Hexdump a buffer to a file. An optional string prefix is added to every
> line
> */
>
> -void qemu_hexdump(const char *buf, FILE *fp, const char *prefix, size_t
> size);
> +void qemu_hexdump(const void *bufptr, FILE *fp,
> + const char *prefix, size_t size);
>
> /*
> * helper to parse debug environment variables
> diff --git a/hw/dma/xlnx_dpdma.c b/hw/dma/xlnx_dpdma.c
> index b40c897de2c..d75a8069426 100644
> --- a/hw/dma/xlnx_dpdma.c
> +++ b/hw/dma/xlnx_dpdma.c
> @@ -388,7 +388,7 @@ static void xlnx_dpdma_dump_descriptor(DPDMADescriptor
> *desc)
> {
> if (DEBUG_DPDMA) {
> qemu_log("DUMP DESCRIPTOR:\n");
> -qemu_hexdump((char *)desc, stdout, "", sizeof(DPDMADescriptor));
> +qemu_hexdump(desc, stdout, "", sizeof(DPDMADescriptor));
> }
> }
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 7035cf4eb97..c817a28decd 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -357,7 +357,7 @@ static ssize_t etsec_receive(NetClientState *nc,
>
> #if defined(HEX_DUMP)
> fprintf(stderr, "%s receive size:%zd\n", nc->name, size);
> -qemu_hexdump((void *)buf, stderr, "", size);
> +qemu_hexdump(buf, stderr, "", size);
> #endif
> /* Flush is unnecessary as are already in receiving path */
> etsec->need_flush = false;
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index fad9cf1ee7a..190e4cf1232 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1781,7 +1781,7 @@ send_response:
> }
>
> #ifdef DEBUG_SD
> -qemu_hexdump((const char *)response, stderr, "Response", rsplen);
> +qemu_hexdump(response, stderr, "Response", rsplen);
> #endif
>
> return rsplen;
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 417a60a2e68..09edb0d81c0 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -240,7 +240,7 @@ static void usbredir_log_data(USBRedirDevice *dev, const
> char *desc,
> if (dev->debug < usbredirparser_debug_data) {
> return;
> }
> -qemu_hexdump((char *)data, stderr, desc, len);
> +qemu_hexdump(data, stderr, desc, len);
> }
>
> /*
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 2c20de1537d..550272b3baa 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -494,9 +494,9 @@ sec:
> g_queue_push_head(>secondary_list, spkt);
>
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr,
> +qemu_hexdump(ppkt->data, stderr,
> "colo-compare ppkt", ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr,
> +qemu_hexdump(spkt->data, stderr,
> "colo-compare spkt", spkt->size);
> }
>
> @@ -535,9 +535,9 @@ static int colo_packet_compare_udp(Packet *spkt, Packet
> *ppkt)
> trace_colo_compare_udp_miscompare("primary pkt size", ppkt->size);
> trace_colo_compare_udp_miscompare("Secondary pkt size", spkt->size);
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> +qemu_hexdump(ppkt->data, stderr, "colo-compare pri pkt",
> ppkt->size);
> -qemu_hexdump((char *)spkt->data, stderr, "colo-compare sec pkt",
> +qemu_hexdump(spkt->data, stderr, "colo-compare sec pkt",
> spkt->size);
> }
> return -1;
> @@ -578,9 +578,9 @@ static int colo_packet_compare_icmp(Packet *spkt, Packet
> *ppkt)
> trace_colo_compare_icmp_miscompare("Secondary pkt size",
> spkt->size);
> if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> -qemu_hexdump((char *)ppkt->data, stderr, "colo-compare pri pkt",
> +qemu_hexdump(ppkt->data,
Re: [PATCH v2 0/2] util/hexdump: Cleanup qemu_hexdump()
On Sat, Aug 22, 2020 at 08:09:48PM +0200, Philippe Mathieu-Daudé wrote: > - Pass const void* buffer > - Reorder arguments > > Supersedes: <20200822150457.1322519-1-f4...@amsat.org> > > Philippe Mathieu-Daudé (2): > util/hexdump: Convert to take a void pointer argument > util/hexdump: Reorder qemu_hexdump() arguments > > include/qemu-common.h| 3 ++- > hw/dma/xlnx_dpdma.c | 2 +- > hw/net/fsl_etsec/etsec.c | 2 +- > hw/net/fsl_etsec/rings.c | 2 +- > hw/sd/sd.c | 2 +- > hw/usb/redirect.c| 2 +- > net/colo-compare.c | 24 > net/net.c| 2 +- > util/hexdump.c | 4 +++- > util/iov.c | 2 +- > 10 files changed, 24 insertions(+), 21 deletions(-) > > -- > 2.26.2 > > Reviewed-by: Stefano Garzarella
Re: [PATCH v2 (BROKEN) 0/6] migration: bring improved savevm/loadvm/delvm to QMP
Am 28.08.2020 um 08:20 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 27.08.2020 um 13:06 hat Markus Armbruster geschrieben: > >> Daniel P. Berrangé writes: > >> > >> > On Wed, Aug 26, 2020 at 07:28:24PM +0100, Daniel P. Berrangé wrote: > >> >> On Wed, Aug 26, 2020 at 05:52:06PM +0200, Markus Armbruster wrote: > >> >> > Open questions: > >> >> > > >> >> > * Do we want the QMP command to delete existing snapshots with > >> >> > conflicting tag / ID, like HMP savevm does? Or do we want it to > >> >> > fail > >> >> > the transaction? > >> >> > >> >> The intent is for the QMP commands to operate exclusively on > >> >> 'tags', and never consider "ID". > >> > > >> > I forgot that even HMP ignores "ID" now and works exclusively in terms > >> > of tags since: > >> > > >> > > >> > commit 6ca080453ea403959ccde661030ca16264acc181 > >> > Author: Daniel Henrique Barboza > >> > Date: Wed Nov 7 11:09:58 2018 -0200 > >> > > >> > block/snapshot.c: eliminate use of ID input in snapshot operations > >> > >> Almost a year after I sent the memo I quoted. It's an incompatible > >> change, but nobody complained, and I'm glad we got this issue out of the > >> way. > > > > FWIW, I would have ignored any complaint about incompatible changes in > > HMP. It's not supposed to be a stable API, but UI. > > The iffy part is actually the loss of ability to access snapshots that > lack a name. Complaints about that would have been valid, I think. > Fortunately, there have been none. 'loadvm ""' should do the trick for these. The same way as you have to use with 'savevm' to create them in non-prehistoric versions of QEMU. We stopped creating snapshots with empty names by default in 0.14, so they are probably not very relevant any more. (Versioned machine types go back "only" to 1.0, so good luck loading a snapshot from an older version. And I wouldn't bet money either on a 1.0 snapshot still working with that machine type...) Kevin
Re: [PATCH v2 (BROKEN) 0/6] migration: bring improved savevm/loadvm/delvm to QMP
Kevin Wolf writes: > Am 27.08.2020 um 13:06 hat Markus Armbruster geschrieben: >> Daniel P. Berrangé writes: >> >> > On Wed, Aug 26, 2020 at 07:28:24PM +0100, Daniel P. Berrangé wrote: >> >> On Wed, Aug 26, 2020 at 05:52:06PM +0200, Markus Armbruster wrote: >> >> > Open questions: >> >> > >> >> > * Do we want the QMP command to delete existing snapshots with >> >> > conflicting tag / ID, like HMP savevm does? Or do we want it to fail >> >> > the transaction? >> >> >> >> The intent is for the QMP commands to operate exclusively on >> >> 'tags', and never consider "ID". >> > >> > I forgot that even HMP ignores "ID" now and works exclusively in terms >> > of tags since: >> > >> > >> > commit 6ca080453ea403959ccde661030ca16264acc181 >> > Author: Daniel Henrique Barboza >> > Date: Wed Nov 7 11:09:58 2018 -0200 >> > >> > block/snapshot.c: eliminate use of ID input in snapshot operations >> >> Almost a year after I sent the memo I quoted. It's an incompatible >> change, but nobody complained, and I'm glad we got this issue out of the >> way. > > FWIW, I would have ignored any complaint about incompatible changes in > HMP. It's not supposed to be a stable API, but UI. The iffy part is actually the loss of ability to access snapshots that lack a name. Complaints about that would have been valid, I think. Fortunately, there have been none.
Re: [PATCH 4/6] block, migration: add bdrv_finalize_vmstate helper
Daniel P. Berrangé writes:
> On Thu, Aug 27, 2020 at 04:02:38PM +0300, Denis V. Lunev wrote:
>> On 8/27/20 3:58 PM, Daniel P. Berrangé wrote:
>> > On Thu, Jul 09, 2020 at 04:26:42PM +0300, Denis V. Lunev wrote:
>> >> Right now bdrv_fclose() is just calling bdrv_flush().
>> >>
>> >> The problem is that migration code is working inefficiently from block
>> >> layer terms and are frequently called for very small pieces of
>> >> unaligned data. Block layer is capable to work this way, but this is very
>> >> slow.
>> >>
>> >> This patch is a preparation for the introduction of the intermediate
>> >> buffer at block driver state. It would be beneficial to separate
>> >> conventional bdrv_flush() from closing QEMU file from migration code.
>> >>
>> >> The patch also forces bdrv_finalize_vmstate() operation inside
>> >> synchronous blk_save_vmstate() operation. This helper is used from
>> >> qemu-io only.
>> >>
>> >> Signed-off-by: Denis V. Lunev
>> >> Reviewed-by: Vladimir Sementsov-Ogievskiy
>> >> CC: Kevin Wolf
>> >> CC: Max Reitz
>> >> CC: Stefan Hajnoczi
>> >> CC: Fam Zheng
>> >> CC: Juan Quintela
>> >> CC: "Dr. David Alan Gilbert"
>> >> CC: Denis Plotnikov
>> >> ---
>> >> block/block-backend.c | 6 +-
>> >> block/io.c| 15 +++
>> >> include/block/block.h | 5 +
>> >> migration/savevm.c| 4
>> >> 4 files changed, 29 insertions(+), 1 deletion(-)
>> >> diff --git a/migration/savevm.c b/migration/savevm.c
>> >> index 45c9dd9d8a..d8a94e312c 100644
>> >> --- a/migration/savevm.c
>> >> +++ b/migration/savevm.c
>> >> @@ -150,6 +150,10 @@ static ssize_t block_get_buffer(void *opaque,
>> >> uint8_t *buf, int64_t pos,
>> >>
>> >> static int bdrv_fclose(void *opaque, Error **errp)
>> >> {
>> >> +int err = bdrv_finalize_vmstate(opaque);
>> >> +if (err < 0) {
>> >> +return err;
>> > This is returning an error without having populating 'errp' which means
>> > the caller will be missing error diagnosis
>>
>> but this behaves exactly like the branch below,
>> bdrv_flush() could return error too and errp
>> is not filled in the same way.
>
> Doh, it seems the only caller passes NULL for the errp too,
> so it is a redundant parameter. So nothing wrong with your
> patch after all.
Not setting an error on failure is plainly wrong.
If it works because all callers pass NULL, then the obvious fix is to
drop the @errp parameter.
I agree it's not this patch's fault. It needs fixing anyway. If you
have to respin for some other reason, including a fix would be nice.
