Re: [PATCH v2 0/2] fix crash if try to migrate during backup
On 11.09.21 14:00, Vladimir Sementsov-Ogievskiy wrote: v2: 01: check that migration fails 02: Add Hanna's r-b Vladimir Sementsov-Ogievskiy (2): tests: add migrate-during-backup block: bdrv_inactivate_recurse(): check for permissions and fix crash block.c | 8 ++ .../qemu-iotests/tests/migrate-during-backup | 97 +++ .../tests/migrate-during-backup.out | 5 + 3 files changed, 110 insertions(+) create mode 100755 tests/qemu-iotests/tests/migrate-during-backup create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out Thanks, applied to my block branch: https://github.com/XanClic/qemu/commits/block Hanna
Re: [PATCH v2 02/17] iotests.py: qemu_img*("create"): support IMGOPTS='compression_type=zstd'
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote:
Adding support of IMGOPTS (like in bash tests) allows user to pass a
lot of different options. Still, some may require additional logic.
Now we want compression_type option, so add some smart logic around it:
ignore compression_type=zstd in IMGOPTS, if test want qcow2 in
compatibility mode. As well, ignore compression_type for non-qcow2
formats.
Note that we may instead add support only to qemu_img_create(), but
that works bad:
1. We'll have to update a lot of tests to use qemu_img_create instead
of qemu_img('create'). (still, we may want do it anyway, but no
reason to create a dependancy between task of supporting IMGOPTS and
updating a lot of tests)
2. Some tests use qemu_img_pipe('create', ..) - even more work on
updating
3. Even if we update all tests to go through qemu_img_create, we'll
need a way to avoid creating new tests using qemu_img*('create') -
add assertions.. That doesn't seem good.
So, let's add support of IMGOPTS to most generic
qemu_img_pipe_and_status().
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
Reviewed-by: Hanna Reitz
Re: [PATCH v2 03/17] iotests: drop qemu_img_verbose() helper
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: qemu_img_verbose() has a drawback of not going through generic qemu_img_pipe_and_status(). qemu_img_verbose() is not very popular, so update the only two users to qemu_img_log() and drop qemu_img_verbose() at all. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/044| 5 +++-- tests/qemu-iotests/044.out| 1 + tests/qemu-iotests/209| 7 --- tests/qemu-iotests/209.out| 2 ++ tests/qemu-iotests/iotests.py | 8 5 files changed, 10 insertions(+), 13 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v2 04/17] iotests.py: rewrite default luks support in qemu_img
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote:
Move the logic to more generic qemu_img_pipe_and_status(). Also behave
better when we have several -o options. And reuse argument parser of
course.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 36 +--
1 file changed, 17 insertions(+), 19 deletions(-)
Reviewed-by: Hanna Reitz
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index def6ae2475..484f616270 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -128,9 +128,13 @@ def qemu_img_create_prepare_args(args: List[str]) ->
List[str]:
args = args[1:]
p = argparse.ArgumentParser(allow_abbrev=False)
+# -o option may be specified several times
+p.add_argument('-o', action='append', default=[])
p.add_argument('-f')
parsed, remaining = p.parse_known_args(args)
+opts_list = parsed.o
+
result = ['create']
if parsed.f is not None:
result += ['-f', parsed.f]
@@ -139,8 +143,18 @@ def qemu_img_create_prepare_args(args: List[str]) ->
List[str]:
# like extended_l2 or compression_type for qcow2. Test may want to create
# additional images in other formats that doesn't support these options.
# So, use IMGOPTS only for images created in imgfmt format.
-if parsed.f == imgfmt and 'IMGOPTS' in os.environ:
-result += ['-o', os.environ['IMGOPTS']]
+imgopts = os.environ.get('IMGOPTS')
+if imgopts and parsed.f == imgfmt:
+opts_list.insert(0, imgopts)
Hm. Yes, IMGOPTS should come first, so it has lower priority. That
means that patch 2 should have inserted IMGOPTS at the beginning of the
parameter list, though, right?
Hanna
+
+# default luks support
+if parsed.f == 'luks' and \
+all('key-secret' not in opts for opts in opts_list):
+result += ['--object', luks_default_secret_object]
+opts_list.append(luks_default_key_secret_opt)
+
+for opts in opts_list:
+result += ['-o', opts]
result += remaining
Re: [PATCH v2 06/17] iotest 065: explicit compression type
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: The test checks different options. It of course fails if set IMGOPTS='compression_type=zstd'. So, let's be explicit in what compression type we want and independent of IMGOPTS. Test both existing compression types. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/065 | 16 1 file changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v2] include/block.h: remove outdated comment
Am 03.09.2021 um 13:38 hat Emanuele Giuseppe Esposito geschrieben: > There are a couple of errors in bdrv_drained_begin header comment: > - block_job_pause does not exist anymore, it has been replaced > with job_pause in b15de82867 > - job_pause is automatically invoked as a .drained_begin callback > (child_job_drained_begin) by the child_job BdrvChildClass struct > in blockjob.c. So no additional pause should be required. > > Signed-off-by: Emanuele Giuseppe Esposito Thanks, applied to the block branch. Kevin
Re: [PATCH v2 07/17] iotests.py: filter out successful output of qemu-img crate
Subject: s/crate/create/ On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: The only "feature" of this "Formatting ..." line is that we have to update it every time we add new option. Let's drop it. Sounds good to me. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/255.out| 4 tests/qemu-iotests/274.out| 29 - tests/qemu-iotests/280.out| 1 - tests/qemu-iotests/iotests.py | 10 -- 4 files changed, 8 insertions(+), 36 deletions(-) Grepping like so: $ (for f in $(ag -l 'Formatting' | grep '\.out' | sed -e 's/\.out.*//'); do \ echo -n "$f "; \ head -n 1 $f; \ done) | grep python yields also 149, 237, and 296 as tests whose reference output needs to be adjusted. (Although 149 just fails for me altogether, seemingly for the same reason that makes 210 fail.) Hanna
Re: [PATCH v2 08/17] iotests.py: filter compression type out
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: We want iotests pass with both the default zlib compression and with IMGOPTS='compression_type=zstd'. Actually the only test that is interested in real compression type in test output is 287 (test for qcow2 compression type) and it's in bash. So for now we can safely filter out compression type in all qcow2 tests. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/206.out| 10 +- tests/qemu-iotests/242.out| 10 +- tests/qemu-iotests/274.out| 10 +- tests/qemu-iotests/iotests.py | 2 ++ 4 files changed, 17 insertions(+), 15 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v2 14/17] iotests: bash tests: filter compression type
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: We want iotests pass with both the default zlib compression and with IMGOPTS='compression_type=zstd'. Actually the only test that is interested in real compression type in test output is 287 (test for qcow2 compression type), so implement specific option for it. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/060.out | 2 +- tests/qemu-iotests/061.out | 12 ++-- tests/qemu-iotests/082.out | 14 +++--- tests/qemu-iotests/198.out | 4 ++-- tests/qemu-iotests/287 | 8 tests/qemu-iotests/common.filter | 8 tests/qemu-iotests/common.rc | 14 +- 7 files changed, 41 insertions(+), 21 deletions(-) Reviewed-by: Hanna Reitz
[PATCH v2] vmdk: allow specification of tools version
VMDK files support an attribute that represents the version of the guest
tools that are installed on the disk.
This attribute is used by vSphere before a machine has been started to
determine if the VM has the guest tools installed.
This is important when configuring "Operating system customizations" in
vSphere, as it checks for the presence of the guest tools before
allowing those customizations.
Thus when the VM has not yet booted normally it would be impossible to
customize it, therefore preventing a customized first-boot.
The attribute should not hurt on disks that do not have the guest tools
installed and indeed the VMware tools also unconditionally add this
attribute.
(Defaulting to the value "2147483647", as is done in this patch)
Signed-off-by: Thomas Weißschuh
---
v1:
https://lore.kernel.org/qemu-devel/20210908174250.12946-1-thomas.weissschuh@zeiss.com/
v1 -> v2:
* Expand QAPI docs (Eric Blake)
block/vmdk.c | 24
qapi/block-core.json | 3 +++
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 4499f136bd..93ef6426b0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -60,6 +60,7 @@
#define VMDK_ZEROED (-3)
#define BLOCK_OPT_ZEROED_GRAIN "zeroed_grain"
+#define BLOCK_OPT_TOOLSVERSION "toolsversion"
typedef struct {
uint32_t version;
@@ -2344,6 +2345,7 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
BlockdevVmdkAdapterType adapter_type,
const char *backing_file,
const char *hw_version,
+ const char *toolsversion,
bool compat6,
bool zeroed_grain,
vmdk_create_extent_fn extent_fn,
@@ -2384,7 +2386,8 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
"ddb.geometry.cylinders = \"%" PRId64 "\"\n"
"ddb.geometry.heads = \"%" PRIu32 "\"\n"
"ddb.geometry.sectors = \"63\"\n"
-"ddb.adapterType = \"%s\"\n";
+"ddb.adapterType = \"%s\"\n"
+"ddb.toolsVersion = \"%s\"\n";
ext_desc_lines = g_string_new(NULL);
@@ -2401,6 +2404,9 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
if (!hw_version) {
hw_version = "4";
}
+if (!toolsversion) {
+toolsversion = "2147483647";
+}
if (adapter_type != BLOCKDEV_VMDK_ADAPTER_TYPE_IDE) {
/* that's the number of heads with which vmware operates when
@@ -2525,7 +2531,8 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
size /
(int64_t)(63 * number_heads * BDRV_SECTOR_SIZE),
number_heads,
- BlockdevVmdkAdapterType_str(adapter_type));
+ BlockdevVmdkAdapterType_str(adapter_type),
+ toolsversion);
desc_len = strlen(desc);
/* the descriptor offset = 0x200 */
if (!split && !flat) {
@@ -2617,6 +2624,7 @@ static int coroutine_fn vmdk_co_create_opts(BlockDriver
*drv,
BlockdevVmdkAdapterType adapter_type_enum;
char *backing_file = NULL;
char *hw_version = NULL;
+char *toolsversion = NULL;
char *fmt = NULL;
BlockdevVmdkSubformat subformat;
int ret = 0;
@@ -2649,6 +2657,7 @@ static int coroutine_fn vmdk_co_create_opts(BlockDriver
*drv,
adapter_type = qemu_opt_get_del(opts, BLOCK_OPT_ADAPTER_TYPE);
backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE);
hw_version = qemu_opt_get_del(opts, BLOCK_OPT_HWVERSION);
+toolsversion = qemu_opt_get_del(opts, BLOCK_OPT_TOOLSVERSION);
compat6 = qemu_opt_get_bool_del(opts, BLOCK_OPT_COMPAT6, false);
if (strcmp(hw_version, "undefined") == 0) {
g_free(hw_version);
@@ -2692,14 +2701,15 @@ static int coroutine_fn vmdk_co_create_opts(BlockDriver
*drv,
.opts = opts,
};
ret = vmdk_co_do_create(total_size, subformat, adapter_type_enum,
-backing_file, hw_version, compat6, zeroed_grain,
-vmdk_co_create_opts_cb, &data, errp);
+backing_file, hw_version, toolsversion, compat6,
+zeroed_grain, vmdk_co_create_opts_cb, &data, errp);
exit:
g_free(backing_fmt);
g_free(adapter_type);
g_free(backing_file);
g_free(hw_version);
+g_free(toolsversion);
g_free(fmt);
g_free(desc);
g_free(path);
@@ -2782,6 +2792,7 @@ static int coroutine_fn
vmdk_co_create(BlockdevCreateOptions *create_options,
opts->adapter_type,
opts->backing_file,
opts->hwversion,
+opts->toolsversion,
false,
Re: [RFC PATCH 3/4] include/sysemu/block-backend: split header into I/O and graph API
On Wed, Sep 08, 2021 at 09:10:20AM -0400, Emanuele Giuseppe Esposito wrote: > diff --git a/include/sysemu/block-backend-graph.h > b/include/sysemu/block-backend-graph.h > new file mode 100644 > index 00..3310987b16 > --- /dev/null > +++ b/include/sysemu/block-backend-graph.h > @@ -0,0 +1,132 @@ > +/* > + * QEMU Block backends > + * > + * Copyright (C) 2014-2016 Red Hat, Inc. > + * > + * Authors: > + * Markus Armbruster , > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 > + * or later. See the COPYING.LIB file in the top-level directory. > + */ > + > +#ifndef BLOCK_BACKEND_GRAPH_H > +#define BLOCK_BACKEND_GRAPH_H > + > +#include "block-backend-common.h" > + > +/* > + * Graph API. These functions run under the BQL lock. > + * > + * If a function modifies the graph, it uses drain and/or > + * aio_context_acquire/release to be sure it has unique access. It's not obvious why additional locking is needed if the BQL is already held. Please refer to the thread-safe I/O API functions that can be running concurrently without the BQL. > + * > + * All functions in this header must use this assertion: > + * g_assert(qemu_in_main_thread()); > + * to be sure they belong here. I suggest "to catch when they are accidentally called without the BQL". It explains the rationale whereas "to be sure they belong here" doesn't explain anything. > diff --git a/include/sysemu/block-backend-io.h > b/include/sysemu/block-backend-io.h > new file mode 100644 > index 00..66a7bed9f0 > --- /dev/null > +++ b/include/sysemu/block-backend-io.h > @@ -0,0 +1,123 @@ > +/* > + * QEMU Block backends > + * > + * Copyright (C) 2014-2016 Red Hat, Inc. > + * > + * Authors: > + * Markus Armbruster , > + * > + * This work is licensed under the terms of the GNU LGPL, version 2.1 > + * or later. See the COPYING.LIB file in the top-level directory. > + */ > + > +#ifndef BLOCK_BACKEND_IO_H > +#define BLOCK_BACKEND_IO_H > + > +#include "block-backend-common.h" > + > +/* > + * I/O API functions. These functions are thread-safe, and therefore > + * can run in any AioContext. "can run in any AioContext" makes me wonder what the exact requirements are. Can they run in any *thread* (regardless of whether an AioContext even exists for that thread) or do they need to run in a thread that has called qemu_set_current_aio_context()? signature.asc Description: PGP signature
[PATCH] qemu-img: Add -F shorthand to convert
Although we have long supported 'qemu-img convert -o
backing_file=foo,backing_fmt=bar', the fact that we have a shortcut -B
for backing_file but none for backing_fmt has made it more likely that
users accidentally run into:
qemu-img: warning: Deprecated use of backing file without explicit backing
format
when using -B instead of -o. For similarity with other qemu-img
commands, such as create and compare, add '-F $fmt' as the shorthand
for '-o backing_fmt=$fmt'. Update iotest 122 for coverage of both
spellings.
Signed-off-by: Eric Blake
---
This stemmed from an IRC conversation; I'd add a Reported-by: line if
I can figure out how to credit more than just the nick bparker_.
docs/tools/qemu-img.rst | 4 ++--
qemu-img.c | 10 +++---
qemu-img-cmds.hx| 2 +-
tests/qemu-iotests/122 | 2 +-
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index b7d602a28804..e02880c1c5dc 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -414,7 +414,7 @@ Command description:
4
Error on reading data
-.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts]
[--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q]
[-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE] [-o
OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT] [-m
NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts]
[--target-is-zero] [--bitmaps [--skip-broken-bitmaps]] [-U] [-C] [-c] [-p] [-q]
[-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O OUTPUT_FMT] [-B BACKING_FILE [-F
backing_fmt]] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] [-r RATE_LIMIT]
[-m NUM_COROUTINES] [-W] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
Convert the disk image *FILENAME* or a snapshot *SNAPSHOT_PARAM*
to disk image *OUTPUT_FILENAME* using format *OUTPUT_FMT*. It can
@@ -438,7 +438,7 @@ Command description:
You can use the *BACKING_FILE* option to force the output image to be
created as a copy on write image of the specified base image; the
*BACKING_FILE* should have the same content as the input's base image,
- however the path, image format, etc may differ.
+ however the path, image format (as given by *BACKING_FMT*), etc may differ.
If a relative path name is given, the backing file is looked up relative to
the directory containing *OUTPUT_FILENAME*.
diff --git a/qemu-img.c b/qemu-img.c
index d77f3e76a9b6..290368da7d6d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2183,7 +2183,8 @@ static int img_convert(int argc, char **argv)
int c, bs_i, flags, src_flags = BDRV_O_NO_SHARE;
const char *fmt = NULL, *out_fmt = NULL, *cache = "unsafe",
*src_cache = BDRV_DEFAULT_CACHE, *out_baseimg = NULL,
- *out_filename, *out_baseimg_param, *snapshot_name = NULL;
+ *out_filename, *out_baseimg_param, *snapshot_name = NULL,
+ *backing_fmt = NULL;
BlockDriver *drv = NULL, *proto_drv = NULL;
BlockDriverInfo bdi;
BlockDriverState *out_bs;
@@ -2223,7 +2224,7 @@ static int img_convert(int argc, char **argv)
{"skip-broken-bitmaps", no_argument, 0, OPTION_SKIP_BROKEN},
{0, 0, 0, 0}
};
-c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WUr:",
+c = getopt_long(argc, argv, ":hf:O:B:CcF:o:l:S:pt:T:qnm:WUr:",
long_options, NULL);
if (c == -1) {
break;
@@ -2253,6 +2254,9 @@ static int img_convert(int argc, char **argv)
case 'c':
s.compressed = true;
break;
+case 'F':
+backing_fmt = optarg;
+break;
case 'o':
if (accumulate_options(&options, optarg) < 0) {
goto fail_getopt;
@@ -2521,7 +2525,7 @@ static int img_convert(int argc, char **argv)
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);
+ret = add_old_style_options(out_fmt, opts, out_baseimg, backing_fmt);
if (ret < 0) {
goto out;
}
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index b3620f29e50c..4c4d94ab2267 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -46,7 +46,7 @@ SRST
ERST
DEF("convert", img_convert,
-"convert [--object objectdef] [--image-opts] [--target-image-opts]
[--target-is-zero] [--bitmaps] [-U] [-C] [-c] [-p] [-q] [-n] [-f fmt] [-t
cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-l
snapshot_param] [-S sparse_size] [-r rate_limit] [-m num_coroutines] [-W]
[--salvage] filename [filename2 [...]] output_filename")
+"convert [--object objectdef] [--image-opts] [--target-image-opts]
[--target-is-zero] [--bitmaps] [-U]
Re: [PATCH v2 17/17] iotests: declare lack of support for compresion_type in IMGOPTS
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: compression_type can't be used if we want to create image with compat=0.10. So, skip these tests, not many of them. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/031 | 5 +++-- tests/qemu-iotests/051 | 5 +++-- tests/qemu-iotests/061 | 6 +- tests/qemu-iotests/112 | 3 ++- tests/qemu-iotests/290 | 2 +- 5 files changed, 14 insertions(+), 7 deletions(-) Reviewed-by: Hanna Reitz
Re: [RFC PATCH 3/4] include/sysemu/block-backend: split header into I/O and graph API
On Wed, Sep 08, 2021 at 09:10:20AM -0400, Emanuele Giuseppe Esposito wrote: > +/* > + * Graph API. These functions run under the BQL lock. > + * > + * If a function modifies the graph, it uses drain and/or > + * aio_context_acquire/release to be sure it has unique access. > + * > + * All functions in this header must use this assertion: > + * g_assert(qemu_in_main_thread()); > + * to be sure they belong here. > + */ It's important to note that not all of these functions are necessarily limited to running under the BQL, but they would require additional auditing and may small thread-safety changes to move them into the I/O API. Often it's not worth doing that work since the APIs are only used with the BQL held at the moment, so they have been placed in the graph API (for now). Stefan signature.asc Description: PGP signature
Re: [RFC PATCH 0/4] block layer: split block APIs in graph and I/O
On Wed, Sep 08, 2021 at 09:10:17AM -0400, Emanuele Giuseppe Esposito wrote: > Currently, block layer APIs like block-backend.h contain a mix of > functions that are either running in the main loop and under the > BQL, or are thread-safe functions and run in iothreads performing I/O. > The functions running under BQL also take care of modifying the > block graph, by using drain and/or aio_context_acquire/release. > This makes it very confusing to understand where each function > runs, and what assumptions it provided with regards to thread > safety. > > We call the functions running under BQL "graph API", and > distinguish them from the thread-safe "I/O API". Maybe "BQL" is clearer than "graph" because not all functions classified as "graph" need to traverse/modify the graph. Stefan signature.asc Description: PGP signature
Re: [PATCH v2 04/17] iotests.py: rewrite default luks support in qemu_img
13.09.2021 14:16, Hanna Reitz wrote:
On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote:
Move the logic to more generic qemu_img_pipe_and_status(). Also behave
better when we have several -o options. And reuse argument parser of
course.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/iotests.py | 36 +--
1 file changed, 17 insertions(+), 19 deletions(-)
Reviewed-by: Hanna Reitz
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index def6ae2475..484f616270 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -128,9 +128,13 @@ def qemu_img_create_prepare_args(args: List[str]) ->
List[str]:
args = args[1:]
p = argparse.ArgumentParser(allow_abbrev=False)
+ # -o option may be specified several times
+ p.add_argument('-o', action='append', default=[])
p.add_argument('-f')
parsed, remaining = p.parse_known_args(args)
+ opts_list = parsed.o
+
result = ['create']
if parsed.f is not None:
result += ['-f', parsed.f]
@@ -139,8 +143,18 @@ def qemu_img_create_prepare_args(args: List[str]) ->
List[str]:
# like extended_l2 or compression_type for qcow2. Test may want to create
# additional images in other formats that doesn't support these options.
# So, use IMGOPTS only for images created in imgfmt format.
- if parsed.f == imgfmt and 'IMGOPTS' in os.environ:
- result += ['-o', os.environ['IMGOPTS']]
+ imgopts = os.environ.get('IMGOPTS')
+ if imgopts and parsed.f == imgfmt:
+ opts_list.insert(0, imgopts)
Hm. Yes, IMGOPTS should come first, so it has lower priority. That means that
patch 2 should have inserted IMGOPTS at the beginning of the parameter list,
though, right?
Agree
+
+ # default luks support
+ if parsed.f == 'luks' and \
+ all('key-secret' not in opts for opts in opts_list):
+ result += ['--object', luks_default_secret_object]
+ opts_list.append(luks_default_key_secret_opt)
+
+ for opts in opts_list:
+ result += ['-o', opts]
result += remaining
--
Best regards,
Vladimir
Re: [RFC PATCH 1/4] main-loop.h: introduce qemu_in_main_thread()
On Wed, Sep 08, 2021 at 09:10:18AM -0400, Emanuele Giuseppe Esposito wrote: > When invoked from the main loop, this function is the same > as qemu_mutex_iothread_locked, and returns true if the BQL is held. > When invoked from iothreads or tests, it returns true only > if the current AioContext is the Main Loop. > > This essentially just extends qemu_mutex_iothread_locked to work > also in unit tests or other users like storage-daemon, that run > in the Main Loop but end up using the implementation in > stubs/iothread-lock.c. > > Using qemu_mutex_iothread_locked in unit tests defaults to false > because they use the implementation in stubs/iothread-lock, > making all assertions added in next patches fail despite the > AioContext is still the main loop. > > Signed-off-by: Emanuele Giuseppe Esposito > --- > include/qemu/main-loop.h | 13 + > softmmu/cpus.c | 5 + > stubs/iothread-lock.c| 5 + > 3 files changed, 23 insertions(+) > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index 8dbc6fcb89..c6547207f7 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -245,6 +245,19 @@ AioContext *iohandler_get_aio_context(void); > */ > bool qemu_mutex_iothread_locked(void); > > +/** > + * qemu_in_main_thread: Return true if the function runs with BQL > + * or in the main loop AioContext. > + * > + * This function falls back to qemu_mutex_iothread_locked() if > + * called from the main loop, otherwise it checks if the current > + * AioContext is the main loop. This is useful to check that the BQL > + * is held, and not make it return false when invoked by unit > + * tests or other users like storage-daemon that end up using > + * stubs/iothread-lock.c implementation. > + */ > +bool qemu_in_main_thread(void); This description doesn't match the behavior because the "or in the main loop AioContext" part only applies to non softmmu builds (e.g. tests). Please phrase it so it's clear that this function is the same as qemu_mutex_iothread_locked() when softmmu/cpus.c is linked into the program. Otherwise it checks that the current AioContext is the global AioContext. signature.asc Description: PGP signature
Re: [RFC PATCH 2/4] migration: block-dirty-bitmap: add missing qemu_mutex_lock_iothread
On Wed, Sep 08, 2021 at 09:10:19AM -0400, Emanuele Giuseppe Esposito wrote: > init_dirty_bitmap_migration assumes the iothread lock (BQL) > to be held, but instead it isn't. > > Instead of adding the lock to qemu_savevm_state_setup(), > follow the same pattern as the other ->save_setup callbacks > and lock+unlock inside dirty_bitmap_save_setup(). > > Signed-off-by: Emanuele Giuseppe Esposito > --- > migration/block-dirty-bitmap.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [RFC PATCH 0/4] block layer: split block APIs in graph and I/O
Thanks, marking block layers APIs that require the BQL and those that don't is an important step. I have posted comments. Stefan signature.asc Description: PGP signature
Re: [RFC PATCH 4/4] block/block-backend.c: assertions for block-backend
On Wed, Sep 08, 2021 at 09:10:21AM -0400, Emanuele Giuseppe Esposito wrote:
> @@ -1767,12 +1817,14 @@ void blk_drain_all(void)
> void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
>BlockdevOnError on_write_error)
> {
> +g_assert(qemu_in_main_thread());
> blk->on_read_error = on_read_error;
> blk->on_write_error = on_write_error;
> }
>
> BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read)
> {
> +g_assert(qemu_in_main_thread());
> return is_read ? blk->on_read_error : blk->on_write_error;
> }
>
> @@ -1780,6 +1832,7 @@ BlockErrorAction blk_get_error_action(BlockBackend
> *blk, bool is_read,
>int error)
> {
> BlockdevOnError on_err = blk_get_on_error(blk, is_read);
> +g_assert(qemu_in_main_thread());
>
> switch (on_err) {
> case BLOCKDEV_ON_ERROR_ENOSPC:
> @@ -1819,6 +1872,7 @@ void blk_error_action(BlockBackend *blk,
> BlockErrorAction action,
>bool is_read, int error)
> {
> assert(error >= 0);
> +g_assert(qemu_in_main_thread());
>
> if (action == BLOCK_ERROR_ACTION_STOP) {
> /* First set the iostatus, so that "info block" returns an iostatus
The error action APIs are called from the I/O code path. For example,
hw/block/virtio-blk.c:virtio_blk_handle_rw_error() is called from an
IOThread with -device virtio-blk-pci,iothread=... with the AioContext
held.
> @@ -1983,11 +2045,13 @@ uint32_t blk_get_max_transfer(BlockBackend *blk)
>
> int blk_get_max_iov(BlockBackend *blk)
> {
> +g_assert(qemu_in_main_thread());
> return blk->root->bs->bl.max_iov;
> }
This is called by hw/block/virtio-blk.c:virtio_blk_submit_multireq()
from an IOThread with the AioContext held.
signature.asc
Description: PGP signature
Re: [PATCH 2/4] hw/sd: add nuvoton MMC
On Wed, 8 Sept 2021 at 00:26, Hao Wu wrote:
>
> From: Shengtan Mao
>
> Signed-off-by: Shengtan Mao
> Reviewed-by: Hao Wu
> Reviewed-by: Chris Rauer
> Reviewed-by: Tyrone Ting
> ---
> hw/arm/npcm7xx.c | 12 +++-
> hw/sd/meson.build | 1 +
> hw/sd/npcm7xx_sdhci.c | 131 ++
> include/hw/arm/npcm7xx.h | 2 +
> include/hw/sd/npcm7xx_sdhci.h | 65 +
> 5 files changed, 210 insertions(+), 1 deletion(-)
> create mode 100644 hw/sd/npcm7xx_sdhci.c
> create mode 100644 include/hw/sd/npcm7xx_sdhci.h
>
> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
> index 2ab0080e0b..878c2208e0 100644
> --- a/hw/arm/npcm7xx.c
> +++ b/hw/arm/npcm7xx.c
> @@ -63,6 +63,8 @@
> #define NPCM7XX_ROM_BA (0x)
> #define NPCM7XX_ROM_SZ (64 * KiB)
>
> +/* SDHCI Modules */
> +#define NPCM7XX_MMC_BA (0xf0842000)
>
> /* Clock configuration values to be fixed up when bypassing bootloader */
>
> @@ -83,6 +85,7 @@ enum NPCM7xxInterrupt {
> NPCM7XX_UART3_IRQ,
> NPCM7XX_EMC1RX_IRQ = 15,
> NPCM7XX_EMC1TX_IRQ,
> +NPCM7XX_MMC_IRQ = 26,
> NPCM7XX_TIMER0_IRQ = 32, /* Timer Module 0 */
> NPCM7XX_TIMER1_IRQ,
> NPCM7XX_TIMER2_IRQ,
> @@ -443,6 +446,8 @@ static void npcm7xx_init(Object *obj)
> for (i = 0; i < ARRAY_SIZE(s->emc); i++) {
> object_initialize_child(obj, "emc[*]", &s->emc[i], TYPE_NPCM7XX_EMC);
> }
> +
> +object_initialize_child(obj, "mmc", &s->mmc, TYPE_NPCM7XX_SDHCI);
> }
>
> static void npcm7xx_realize(DeviceState *dev, Error **errp)
> @@ -707,6 +712,12 @@ static void npcm7xx_realize(DeviceState *dev, Error
> **errp)
> &error_abort);
> memory_region_add_subregion(get_system_memory(), NPCM7XX_ROM_BA,
> &s->irom);
>
> +/* SDHCI */
> +sysbus_realize(SYS_BUS_DEVICE(&s->mmc), &error_abort);
> +sysbus_mmio_map(SYS_BUS_DEVICE(&s->mmc), 0, NPCM7XX_MMC_BA);
> +sysbus_connect_irq(SYS_BUS_DEVICE(&s->mmc), 0,
> +npcm7xx_irq(s, NPCM7XX_MMC_IRQ));
> +
> create_unimplemented_device("npcm7xx.shm", 0xc0001000, 4 *
> KiB);
> create_unimplemented_device("npcm7xx.vdmx", 0xe080, 4 *
> KiB);
> create_unimplemented_device("npcm7xx.pcierc", 0xe100, 64 *
> KiB);
> @@ -736,7 +747,6 @@ static void npcm7xx_realize(DeviceState *dev, Error
> **errp)
> create_unimplemented_device("npcm7xx.usbd[8]", 0xf0838000, 4 *
> KiB);
> create_unimplemented_device("npcm7xx.usbd[9]", 0xf0839000, 4 *
> KiB);
> create_unimplemented_device("npcm7xx.sd", 0xf084, 8 *
> KiB);
> -create_unimplemented_device("npcm7xx.mmc", 0xf0842000, 8 *
> KiB);
> create_unimplemented_device("npcm7xx.pcimbx", 0xf0848000, 512 *
> KiB);
> create_unimplemented_device("npcm7xx.aes", 0xf0858000, 4 *
> KiB);
> create_unimplemented_device("npcm7xx.des", 0xf0859000, 4 *
> KiB);
The "add to board" code should be a separate patch from
"implement this new device".
> diff --git a/hw/sd/meson.build b/hw/sd/meson.build
> index f1ce357a3b..807ca07b7c 100644
> --- a/hw/sd/meson.build
> +++ b/hw/sd/meson.build
> @@ -9,4 +9,5 @@ softmmu_ss.add(when: 'CONFIG_PXA2XX', if_true:
> files('pxa2xx_mmci.c'))
> softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files('bcm2835_sdhost.c'))
> softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_sdhci.c'))
> softmmu_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true:
> files('allwinner-sdhost.c'))
> +softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx_sdhci.c'))
> softmmu_ss.add(when: 'CONFIG_CADENCE_SDHCI', if_true:
> files('cadence_sdhci.c'))
> diff --git a/hw/sd/npcm7xx_sdhci.c b/hw/sd/npcm7xx_sdhci.c
> new file mode 100644
> index 00..85cccdc485
> --- /dev/null
> +++ b/hw/sd/npcm7xx_sdhci.c
> @@ -0,0 +1,131 @@
> +/*
> + * NPCM7xx SD-3.0 / eMMC-4.51 Host Controller
> + *
> + * Copyright (c) 2021 Google LLC
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/sd/npcm7xx_sdhci.h"
> +#include "sdhci-internal.h"
> +
> +static uint64_t npcm7xx_sdhci_read(void *opaque, hwaddr addr, unsigned int
> size)
> +{
> +NPCM7xxSDHCIState *s = opaque;
> +uint64_t val = 0;
> +
> +switch (addr) {
> +case NPCM7XX_PRSTVALS_0:
> +case NPCM7XX_PRSTVALS_1:
> +case NPCM7XX_PRSTVALS_2:
Re: [PATCH 3/4] hw/arm: Attach MMC to quanta-gbs-bmc
On Wed, 8 Sept 2021 at 00:26, Hao Wu wrote: > > From: Shengtan Mao > > Signed-off-by: Shengtan Mao > Reviewed-by: Hao Wu > Reviewed-by: Tyrone Ting > --- > hw/arm/npcm7xx_boards.c | 21 + > 1 file changed, 21 insertions(+) > > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c > index e5a3243995..7205483280 100644 > --- a/hw/arm/npcm7xx_boards.c > +++ b/hw/arm/npcm7xx_boards.c > @@ -27,6 +27,10 @@ > #include "qemu-common.h" > #include "qemu/datadir.h" > #include "qemu/units.h" > +#include "sysemu/blockdev.h" > +#include "sysemu/sysemu.h" > +#include "sysemu/block-backend.h" > + > Stray extra blank line. Otherwise Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v2 1/3] simplebench: add img_bench_templater.py
On 24.08.21 12:15, Vladimir Sementsov-Ogievskiy wrote: Add simple grammar-parsing template benchmark. New tool consume test template written in bash with some special grammar injections and produces multiple tests, run them and finally print a performance comparison table of different tests produced from one template. Signed-off-by: Vladimir Sementsov-Ogievskiy --- scripts/simplebench/img_bench_templater.py | 95 ++ scripts/simplebench/table_templater.py | 62 ++ 2 files changed, 157 insertions(+) create mode 100755 scripts/simplebench/img_bench_templater.py create mode 100644 scripts/simplebench/table_templater.py Reviewed-by: Hanna Reitz
Re: [PATCH 4/4] tests/qtest: add qtests for npcm7xx sdhci
On Wed, 8 Sept 2021 at 00:26, Hao Wu wrote:
>
> From: Shengtan Mao
>
> Signed-off-by: Shengtan Mao
> Reviewed-by: Hao Wu
> Reviewed-by: Chris Rauer
> Reviewed-by: Tyrone Ting
> ---
> tests/qtest/meson.build | 1 +
> tests/qtest/npcm7xx_sdhci-test.c | 201 +++
> 2 files changed, 202 insertions(+)
> create mode 100644 tests/qtest/npcm7xx_sdhci-test.c
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 757bb8499a..ef9c904779 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -157,6 +157,7 @@ qtests_npcm7xx = \
> 'npcm7xx_gpio-test',
> 'npcm7xx_pwm-test',
> 'npcm7xx_rng-test',
> + 'npcm7xx_sdhci-test',
> 'npcm7xx_smbus-test',
> 'npcm7xx_timer-test',
> 'npcm7xx_watchdog_timer-test'] + \
> diff --git a/tests/qtest/npcm7xx_sdhci-test.c
> b/tests/qtest/npcm7xx_sdhci-test.c
> new file mode 100644
> index 00..5c4e78fda4
> --- /dev/null
> +++ b/tests/qtest/npcm7xx_sdhci-test.c
> @@ -0,0 +1,201 @@
> +/*
> + * QTests for NPCM7xx SD-3.0 / MMC-4.51 Host Controller
> + *
> + * Copyright (c) 2021 Google LLC
> + *
> + * 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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sd/npcm7xx_sdhci.h"
> +
> +#include "libqos/libqtest.h"
> +#include "libqtest-single.h"
> +#include "libqos/sdhci-cmd.h"
> +
> +#define NPCM7XX_MMC_BA 0xF0842000
> +#define NPCM7XX_BLK_SIZE 512
> +#define NPCM7XX_TEST_IMAGE_SIZE (1 << 30)
> +
> +char *sd_path;
should be "static".
> +
> +static QTestState *setup_sd_card(void)
> +{
> +QTestState *qts = qtest_initf(
> +"-machine quanta-gbs-bmc "
> +"-device sd-card,drive=drive0 "
> +"-drive id=drive0,if=none,file=%s,format=raw,auto-read-only=off",
> +sd_path);
> +
> +qtest_writew(qts, NPCM7XX_MMC_BA + SDHC_SWRST, SDHC_RESET_ALL);
> +qtest_writew(qts, NPCM7XX_MMC_BA + SDHC_CLKCON,
> + SDHC_CLOCK_SDCLK_EN | SDHC_CLOCK_INT_STABLE |
> + SDHC_CLOCK_INT_EN);
inconsistent indent
> +sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_APP_CMD);
> +sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4120, 0, (41 << 8));
> +sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
> +sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
> +sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4567, 0,
> + SDHC_SELECT_DESELECT_CARD);
> +
> +return qts;
> +}
> +
> +static void write_sdread(QTestState *qts, const char *msg)
> +{
> +size_t len = strlen(msg);
> +char *rmsg = g_malloc(len);
> +
> +/* write message to sd */
> +int fd = open(sd_path, O_WRONLY);
> +int ret = write(fd, msg, len);
> +close(fd);
You should check the return value from open() and close()
(same again in code below)
> +g_assert(ret == len);
> +
> +/* read message using sdhci */
> +ret = sdhci_read_cmd(qts, NPCM7XX_MMC_BA, rmsg, len);
> +g_assert(ret == len);
> +g_assert(!strcmp(rmsg, msg));
> +
> +free(rmsg);
> +}
> +static void drive_create(void)
> +{
> +int fd, ret;
> +sd_path = g_strdup("/tmp/qtest.XX");
Please use a template string that gives an indication of
which test created the file. This helps subsequent debugging
of "where did this junk in my tmp directory come from?".
> +
> +/* Create a temporary raw image */
> +fd = mkstemp(sd_path);
> +g_assert_cmpint(fd, >=, 0);
> +ret = ftruncate(fd, NPCM7XX_TEST_IMAGE_SIZE);
> +g_assert_cmpint(ret, ==, 0);
> +g_message("%s", sd_path);
> +close(fd);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +drive_create();
> +
> +g_test_init(&argc, &argv, NULL);
> +
> +qtest_add_func("npcm7xx_sdhci/reset", test_reset);
> +qtest_add_func("npcm7xx_sdhci/write_sd", test_write_sd);
> +qtest_add_func("npcm7xx_sdhci/read_sd", test_read_sd);
> +
> +int ret = g_test_run();
> +drive_destroy();
> +return ret;
> +}
thanks
-- PMM
Re: [PATCH 1/4] tests/qtest/libqos: add SDHCI commands
On Wed, 8 Sept 2021 at 00:26, Hao Wu wrote: > > From: Shengtan Mao > > Signed-off-by: Shengtan Mao > Reviewed-by: Hao Wu > Reviewed-by: Chris Rauer > Reviewed-by: Tyrone Ting I think this patch would make more sense placed later in the series, just before the current patch 4 which is the user of the functions added here. -- PMM
Re: [PATCH v2 3/3] qcow2: handle_dependencies(): relax conflict detection
On 24.08.21 12:15, Vladimir Sementsov-Ogievskiy wrote:
There is no conflict and no dependency if we have parallel writes to
different subclusters of one cluster when the cluster itself is already
allocated. So, relax extra dependency.
Measure performance:
First, prepare build/qemu-img-old and build/qemu-img-new images.
cd scripts/simplebench
./img_bench_templater.py
Paste the following to stdin of running script:
qemu_img=../../build/qemu-img-{old|new}
$qemu_img create -f qcow2 -o extended_l2=on /ssd/x.qcow2 1G
$qemu_img bench -c 10 -d 8 [-s 2K|-s 2K -o 512|-s $((1024*2+512))] \
-w -t none -n /ssd/x.qcow2
The result:
All results are in seconds
-- - -
oldnew
-s 2K 6.7 ± 15% 6.2 ± 12%
-7%
-s 2K -o 51213 ± 3%11 ± 5%
-16%
-s $((1024*2+512)) 9.5 ± 4% 8.4
-12%
-- - -
So small writes are more independent now and that helps to keep deeper
io queue which improves performance.
271 iotest output becomes racy for three allocation in one cluster.
Second and third writes may finish in different order. Second and
third requests don't depend on each other any more. Still they both
depend on first request anyway. Filter out second and third write
offsets to cover both possible outputs.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/qcow2-cluster.c | 11 +++
tests/qemu-iotests/271 | 5 -
tests/qemu-iotests/271.out | 4 ++--
3 files changed, 17 insertions(+), 3 deletions(-)
[...]
diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
index 599b849cc6..d9d391955e 100755
--- a/tests/qemu-iotests/271
+++ b/tests/qemu-iotests/271
@@ -893,7 +893,10 @@ EOF
}
_make_test_img -o extended_l2=on 1M
-_concurrent_io | $QEMU_IO | _filter_qemu_io
+# Second an third writes in _concurrent_io() are independent and may finish in
s/ an / and /
With that fixed:
Reviewed-by: Hanna Reitz
+# different order. So, filter offset out to match both possible variants.
+_concurrent_io | $QEMU_IO | _filter_qemu_io | \
+$SED -e 's/\(20480\|40960\)/OFFSET/'
_concurrent_verify | $QEMU_IO | _filter_qemu_io
# success, all done
Re: [PATCH v2 0/3] qcow2: relax subclusters allocation dependencies
On 24.08.21 12:15, Vladimir Sementsov-Ogievskiy wrote: Hi all! v2: 01: improve documentation 02: add Hanna's and Eric's r-bs, add tiny grammar fix 03: fix test by filtering instead of reducing number of writes Thanks, I’ve fixed the typo in patch 3 and applied the series to my block branch: https://github.com/XanClic/qemu/commits/block Hanna
Re: [PATCH] nbd/server: Suppress Broken pipe errors on abrupt disconnection
Am 23.07.2021 um 17:49 hat Eric Blake geschrieben:
> On Thu, Jul 22, 2021 at 11:45:52AM +0100, Richard W.M. Jones wrote:
> > $ rm -f /tmp/sock /tmp/pid
> > $ qemu-img create -f qcow2 /tmp/disk.qcow2 1M
> > $ qemu-nbd -t --format=qcow2 --socket=/tmp/sock --pid-file=/tmp/pid
> > /tmp/disk.qcow2 &
> > $ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()'
> > qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write
> > to socket: Broken pipe
> > $ killall qemu-nbd
> >
> > nbdsh is abruptly dropping the NBD connection here which is a valid
> > way to close the connection. It seems unnecessary to print an error
> > in this case so this commit suppresses it.
> >
> > Note that if you call the nbdsh h.shutdown() method then the message
> > was not printed:
> >
> > $ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()' -c
> > 'h.shutdown()'
>
> A client not shutting down cleanly might cause the server to leave the
> disk in an unspecified state prior to the next client (more
> concretely, a client that just disconnects instead of waiting for a
> flush to land may result in data loss from the point of view of that
> client when it reconnects, although the server was never in the
> wrong).
I think in such cases, clients must assume that all in-flight requests
have failed. Request failure means that the state is undefined. You
could have the old content, you could have the new content, or you could
have some random corruption.
> But for your _specific_ example here of a client that only performs
> read actions and does not modify the disk, there is obviously no data
> loss possible.
>
> But you are also correct that a client that disconnects abruptly
> instead of cleanly is a common enough event that warning about it can
> just feel noisy. Is this the sort of thing that users would want a
> command-line knob to opt in or out of those warnings (and what default
> should that knob take), or should this be something we just always
> ignore? Or maybe we make the warning conditional on whether the
> client attempted any modification to the image, being silent on
> default to a client that merely reads, and only noisy for a client
> that attempted at least one write but disconnected before we could
> reply that the write or subsequent flush was complete.
>
> qemu-storage-daemon has to answer the same question, so I'd like
> Kevin's take on the matter to make sure we pick an answer we are
> consistently happy with.
So I don't think I would make a difference between read-only and
read-write clients. The consideration whether we should print an error
message or not feels more like something that becomes relevant when
debugging a bug that we can't reproduce and just get a bunch of logs. I
feel that abrupt disconnects could in some cases be useful information
to have there.
Essentially it's something that you would configure with log levels, but
we don't really have that (and even if we had it, in practice management
tools would use one default setting). So I feel we have to decide for
one thing or the other. Since bugs involving NBD are probably something
you'll have to debug, maybe you should pick. I don't really mind either
way.
> >
> > Signed-off-by: Richard W.M. Jones
> > ---
> > nbd/server.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/nbd/server.c b/nbd/server.c
> > index b60ebc3ab6..0f86535b88 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -2668,7 +2668,11 @@ static coroutine_fn void nbd_trip(void *opaque)
> > ret = nbd_handle_request(client, &request, req->data, &local_err);
> > }
> > if (ret < 0) {
> > -error_prepend(&local_err, "Failed to send reply: ");
> > +if (errno != EPIPE) {
> > +error_prepend(&local_err, "Failed to send reply: ");
> > +} else {
> > +local_err = NULL;
>
> This line should be error_free(local_err) to avoid a memleak.
Actually, you want both error_free(local_err) and local_err = NULL.
> > +}
> > goto disconnect;
> > }
Kevin
Re: [PATCH v3 01/10] qcow2-refcount: improve style of check_refcounts_l2()
On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote: - don't use same name for size in bytes and in entries - use g_autofree for l2_table - add whitespace - fix block comment style Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/qcow2-refcount.c | 47 +- 1 file changed, 24 insertions(+), 23 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v3 02/10] qcow2: compressed read: simplify cluster descriptor passing
On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote: Let's pass the whole L2 entry and not bother with L2E_COMPRESSED_OFFSET_SIZE_MASK. It also helps further refactoring that adds generic qcow2_parse_compressed_l2_entry() helper. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Reviewed-by: Alberto Garcia --- block/qcow2.h | 1 - block/qcow2-cluster.c | 5 ++--- block/qcow2.c | 12 +++- 3 files changed, 9 insertions(+), 9 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH] nbd/server: Suppress Broken pipe errors on abrupt disconnection
On Mon, Sep 13, 2021 at 05:07:12PM +0200, Kevin Wolf wrote:
> > > if (ret < 0) {
> > > -error_prepend(&local_err, "Failed to send reply: ");
> > > +if (errno != EPIPE) {
> > > +error_prepend(&local_err, "Failed to send reply: ");
> > > +} else {
> > > +local_err = NULL;
> >
> > This line should be error_free(local_err) to avoid a memleak.
>
> Actually, you want both error_free(local_err) and local_err = NULL.
Give me a few mins to test an post a new version that at least
fixes this bug ...
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
[PATCH v2] nbd/server: Suppress Broken pipe errors on abrupt disconnection
$ rm -f /tmp/sock /tmp/pid
$ qemu-img create -f qcow2 /tmp/disk.qcow2 1M
$ qemu-nbd -t --format=qcow2 --socket=/tmp/sock --pid-file=/tmp/pid
/tmp/disk.qcow2 &
$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()'
qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write to
socket: Broken pipe
$ killall qemu-nbd
nbdsh is abruptly dropping the NBD connection here which is a valid
way to close the connection. It seems unnecessary to print an error
in this case so this commit suppresses it.
Note that if you call the nbdsh h.shutdown() method then the message
was not printed:
$ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()' -c 'h.shutdown()'
Signed-off-by: Richard W.M. Jones
---
nbd/server.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/nbd/server.c b/nbd/server.c
index 3927f7789d..e0a43802b2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2669,7 +2669,12 @@ static coroutine_fn void nbd_trip(void *opaque)
ret = nbd_handle_request(client, &request, req->data, &local_err);
}
if (ret < 0) {
-error_prepend(&local_err, "Failed to send reply: ");
+if (errno != EPIPE) {
+error_prepend(&local_err, "Failed to send reply: ");
+} else {
+error_free(local_err);
+local_err = NULL;
+}
goto disconnect;
}
--
2.32.0
Re: [PATCH v3 03/10] qcow2: introduce qcow2_parse_compressed_l2_entry() helper
On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote: Add helper to parse compressed l2_entry and use it everywhere instead of open-coding. Note, that in most places we move to precise coffset/csize instead of sector-aligned. Still it should work good enough for updating refcounts. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/qcow2.h | 3 ++- block/qcow2-cluster.c | 15 +++ block/qcow2-refcount.c | 36 +--- block/qcow2.c | 9 ++--- 4 files changed, 36 insertions(+), 27 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v3 04/10] qcow2-refcount: introduce fix_l2_entry_by_zero()
On 24.05.21 16:20, Vladimir Sementsov-Ogievskiy wrote: Split fix_l2_entry_by_zero() out of check_refcounts_l2() to be reused in further patch. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake --- block/qcow2-refcount.c | 87 +- 1 file changed, 60 insertions(+), 27 deletions(-) Reviewed-by: Hanna Reitz
Re: [PATCH v2 17/17] iotests: declare lack of support for compresion_type in IMGOPTS
13.09.2021 15:43, Hanna Reitz wrote: On 20.07.21 13:38, Vladimir Sementsov-Ogievskiy wrote: compression_type can't be used if we want to create image with compat=0.10. So, skip these tests, not many of them. Signed-off-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/031 | 5 +++-- tests/qemu-iotests/051 | 5 +++-- tests/qemu-iotests/061 | 6 +- tests/qemu-iotests/112 | 3 ++- tests/qemu-iotests/290 | 2 +- 5 files changed, 14 insertions(+), 7 deletions(-) Reviewed-by: Hanna Reitz Thanks a lot for reviewing! I'll resend tomorrow. -- Best regards, Vladimir
