Re: [PATCH 02/22] hw/block: m25p80: Add various ISSI flash information
On Thu, Dec 31, 2020 at 3:32 AM Bin Meng wrote:
>
> From: Bin Meng
>
> This updates the flash information table to include various ISSI
> flashes that are supported by upstream U-Boot and Linux kernel.
>
> Signed-off-by: Bin Meng
Acked-by: Alistair Francis
Alistair
> ---
>
> hw/block/m25p80.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 8a62bc4bc4..e82deb41c6 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -209,6 +209,19 @@ static const FlashPartInfo known_devices[] = {
> { INFO("640s33b", 0x898913, 0, 64 << 10, 128, 0) },
> { INFO("n25q064", 0x20ba17, 0, 64 << 10, 128, 0) },
>
> +/* ISSI */
> +{ INFO("is25lq040b", 0x9d4013, 0, 64 << 10, 8, ER_4K) },
> +{ INFO("is25lp080d", 0x9d6014, 0, 64 << 10, 16, ER_4K) },
> +{ INFO("is25lp016d", 0x9d6015, 0, 64 << 10, 32, ER_4K) },
> +{ INFO("is25lp032", 0x9d6016, 0, 64 << 10, 64, ER_4K) },
> +{ INFO("is25lp064", 0x9d6017, 0, 64 << 10, 128, ER_4K) },
> +{ INFO("is25lp128", 0x9d6018, 0, 64 << 10, 256, ER_4K) },
> +{ INFO("is25lp256", 0x9d6019, 0, 64 << 10, 512, ER_4K) },
> +{ INFO("is25wp032", 0x9d7016, 0, 64 << 10, 64, ER_4K) },
> +{ INFO("is25wp064", 0x9d7017, 0, 64 << 10, 128, ER_4K) },
> +{ INFO("is25wp128", 0x9d7018, 0, 64 << 10, 256, ER_4K) },
> +{ INFO("is25wp256", 0x9d7019, 0, 64 << 10, 512, ER_4K) },
> +
> /* Macronix */
> { INFO("mx25l2005a", 0xc22012, 0, 64 << 10, 4, ER_4K) },
> { INFO("mx25l4005a", 0xc22013, 0, 64 << 10, 8, ER_4K) },
> --
> 2.25.1
>
>
Re: [PATCH v5 1/2] hw/block: m25p80: Don't write to flash if write is disabled
On Mon, Jan 4, 2021 at 7:50 PM Bin Meng wrote:
>
> On Wed, Dec 23, 2020 at 10:00 AM Bin Meng wrote:
> >
> > From: Bin Meng
> >
> > When write is disabled, the write to flash should be avoided
> > in flash_write8().
> >
> > Fixes: 82a2499011a7 ("m25p80: Initial implementation of SPI flash device")
> > Signed-off-by: Bin Meng
> > Reviewed-by: Philippe Mathieu-Daudé
> > Reviewed-by: Francisco Iglesias
> >
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> > - new patch: honor write enable flag in flash write
> >
> > hw/block/m25p80.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
>
> Ping?
Thanks!
Applied to riscv-to-apply.next
Alistair
>
Re: [PATCH] meson: Propagate gnutls dependency
On 05/01/21 15:37, Roman Bolshakov wrote:
Does it work if you do:
crypto_ss.add(authz, qom)
libcrypto = static_library('crypto', crypto_ss.sources() + genh,
dependencies: crypto_ss.dependencies(),
...)
crypto = declare_dependency(link_whole: libcrypto,
dependencies: crypto_ss.dependencies())
I tried that approach before I sent the patch in the subject. It
produces duplicate symbols:
duplicate symbol '_qauthz_pam_new' in:
libcrypto.fa(authz_pamacct.c.o)
libauthz.fa(authz_pamacct.c.o)
[...]
duplicate symbol '_object_property_set_qobject' in:
libcrypto.fa(qom_qom-qobject.c.o)
libqom.fa(qom_qom-qobject.c.o)
My impression that it links in every static library that's mentioned in
dependencies of static_library, so they grow like a snow ball. Patch
below:
Okay, I'll look more into it.
Paolo
diff --git a/block/meson.build b/block/meson.build
index 7595d86c41..7eaf48c6dc 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -40,7 +40,7 @@ block_ss.add(files(
'vmdk.c',
'vpc.c',
'write-threshold.c',
-), zstd, zlib)
+), crypto, zstd, zlib)
softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('blkreplay.c'))
diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build
index fd2951a860..1f2ed013b2 100644
--- a/hw/nvram/meson.build
+++ b/hw/nvram/meson.build
@@ -1,6 +1,3 @@
-# QOM interfaces must be available anytime QOM is used.
-qom_ss.add(files('fw_cfg-interface.c'))
-
softmmu_ss.add(files('fw_cfg.c'))
softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c'))
softmmu_ss.add(when: 'CONFIG_DS1225Y', if_true: files('ds1225y.c'))
diff --git a/io/meson.build b/io/meson.build
index bcd8b1e737..a844271b17 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -12,4 +12,4 @@ io_ss.add(files(
'dns-resolver.c',
'net-listener.c',
'task.c',
-))
+), crypto)
diff --git a/meson.build b/meson.build
index 372576f82c..1a8c653067 100644
--- a/meson.build
+++ b/meson.build
@@ -1538,6 +1538,34 @@ libqemuutil = static_library('qemuutil',
qemuutil = declare_dependency(link_with: libqemuutil,
sources: genh + version_res)
+# QOM interfaces must be available anytime QOM is used.
+qom_ss.add(files('hw/nvram/fw_cfg-interface.c'))
+qom_ss = qom_ss.apply(config_host, strict: false)
+libqom = static_library('qom', qom_ss.sources() + genh,
+dependencies: [qom_ss.dependencies()],
+name_suffix: 'fa')
+
+qom = declare_dependency(link_whole: libqom)
+
+authz_ss = authz_ss.apply(config_host, strict: false)
+libauthz = static_library('authz', authz_ss.sources() + genh,
+ dependencies: [authz_ss.dependencies()],
+ name_suffix: 'fa',
+ build_by_default: false)
+
+authz = declare_dependency(link_whole: libauthz,
+ dependencies: qom)
+
+crypto_ss.add(authz)
+crypto_ss = crypto_ss.apply(config_host, strict: false)
+libcrypto = static_library('crypto', crypto_ss.sources() + genh,
+ dependencies: crypto_ss.dependencies(),
+ name_suffix: 'fa',
+ build_by_default: false)
+
+crypto = declare_dependency(link_whole: libcrypto,
+dependencies: crypto_ss.dependencies())
+
decodetree = generator(find_program('scripts/decodetree.py'),
output: 'decode-@basen...@.c.inc',
arguments: ['@INPUT@', '@EXTRA_ARGS@', '-o',
'@OUTPUT@'])
@@ -1652,31 +1680,6 @@ qemu_syms = custom_target('qemu.syms', output:
'qemu.syms',
capture: true,
command: [undefsym, nm, '@INPUT@'])
-qom_ss = qom_ss.apply(config_host, strict: false)
-libqom = static_library('qom', qom_ss.sources() + genh,
-dependencies: [qom_ss.dependencies()],
-name_suffix: 'fa')
-
-qom = declare_dependency(link_whole: libqom)
-
-authz_ss = authz_ss.apply(config_host, strict: false)
-libauthz = static_library('authz', authz_ss.sources() + genh,
- dependencies: [authz_ss.dependencies()],
- name_suffix: 'fa',
- build_by_default: false)
-
-authz = declare_dependency(link_whole: libauthz,
- dependencies: qom)
-
-crypto_ss = crypto_ss.apply(config_host, strict: false)
-libcrypto = static_library('crypto', crypto_ss.sources() + genh,
- dependencies: [crypto_ss.dependencies()],
- name_suffix: 'fa',
-
Re: [PULL for-5.2 2/2] scripts/tracetool: silence SystemTap dtrace(1) long long warnings
On Mon, Jan 04, 2021 at 09:31:19PM +0100, Laurent Vivier wrote:
> On 11/11/2020 16:56, Stefan Hajnoczi wrote:
> > SystemTap's dtrace(1) prints the following warning when it encounters
> > long long arguments:
> >
> > Warning: /usr/bin/dtrace:trace/trace-dtrace-hw_virtio.dtrace:76: syntax
> > error near:
> > probe vhost_vdpa_dev_start
> >
> > Warning: Proceeding as if --no-pyparsing was given.
> >
> > Use the uint64_t and int64_t types, respectively. This works with all
> > host CPU 32- and 64-bit data models (ILP32, LP64, and LLP64) that QEMU
> > supports.
> >
> > Reported-by: Markus Armbruster
> > Signed-off-by: Stefan Hajnoczi
> > Reviewed-by: Daniel P. Berrangé
> > Reviewed-by: Philippe Mathieu-Daudé
> > Message-id: 20201020094043.159935-1-stefa...@redhat.com
> > Suggested-by: Daniel P. Berrangé
> > Signed-off-by: Stefan Hajnoczi
> > ---
> > scripts/tracetool/format/d.py | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/scripts/tracetool/format/d.py b/scripts/tracetool/format/d.py
> > index 353722f89c..ebfb714200 100644
> > --- a/scripts/tracetool/format/d.py
> > +++ b/scripts/tracetool/format/d.py
> > @@ -57,6 +57,12 @@ def generate(events, backend, group):
> > # Avoid it by changing probe type to signed char *
> > beforehand.
> > if type_ == 'int8_t *':
> > type_ = 'signed char *'
> > +
> > +# SystemTap dtrace(1) emits a warning when long long is used
> > +type_ = type_.replace('unsigned long long', 'uint64_t')
> > +type_ = type_.replace('signed long long', 'int64_t')
> > +type_ = type_.replace('long long', 'int64_t')
> > +
> > if name in RESERVED_WORDS:
> > name += '_'
> > args.append(type_ + ' ' + name)
> >
>
> This patch fixes the warning with "d" format, but we have the same kind of
> problem with
> log-stap format:
>
> $ sudo stap -e 'probe begin{printf ("BEGIN")}' -I .
> parse error: invalid or missing conversion specifier
> saw: operator ',' at ./qemu-system-x86_64-log.stp:15118:101
>source: printf("%d@%d vhost_vdpa_set_log_base dev: %p base: 0x%x
> size: %llu
> refcnt: %d fd: %d log: %p\n", pid(), gettimeofday_ns(), dev, base, size,
> refcnt, fd, log)
>
>^
>
> 1 parse error.
> WARNING: tapset "./qemu-system-x86_64-log.stp" has errors, and will be
> skipped
> BEGIN
>
> This happens because of the "%llu" in the format string.
>
> I'm wondering if we need to fix all the stap based format or simply replace
> the "unsigned
> long long" by "uint64_t" in hw/virtio/trace-events?
The problem isn't really the data type, but rather the format string.
systemtap format strings are not quite the same as C format strings.
So we need to re-write %llu into %lu I expect. We already do some
rewriting in log_stap.py but we obviously need a bit more.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] tests/iotests: drop test 312 from auto group
On 05.01.21 11:04, Alex Bennée wrote: The "auto" documentation states: That means they should run with every QEMU binary (also non-x86) which is not the case as the check-system-fedora build which only includes a rag tag group of rare and deprecated targets doesn't support the virtio device required. Signed-off-by: Alex Bennée --- tests/qemu-iotests/group | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Re: [PULL 0/5] Tracing patches
On Mon, 4 Jan 2021 at 14:32, Stefan Hajnoczi wrote: > > The following changes since commit 41192db338588051f21501abc13743e62b0a5605: > > Merge remote-tracking branch > 'remotes/ehabkost-gl/tags/machine-next-pull-request' into staging (2021-01-01 > 22:57:15 +) > > are available in the Git repository at: > > https://gitlab.com/stefanha/qemu.git tags/tracing-pull-request > > for you to fetch changes up to 7fb48c0ee1bbf5cc4c905e900b054096250e9f39: > > tracetool: show trace-events filename/lineno in fmt string errors > (2021-01-04 14:24:58 +) > > > Pull request > > Show trace-events filename/lineno in fmt string errors and send -d trace:help > output to stdout for consistency. > > > > Doug Evans (1): > trace: Send "-d trace:help" output to stdout > > Stefan Hajnoczi (4): > tracetool: add output filename command-line argument > tracetool: add out_lineno and out_next_lineno to out() > tracetool: add input filename and line number to Event > tracetool: show trace-events filename/lineno in fmt string errors Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0 for any user-visible changes. -- PMM
Re: [PATCH v15 00/13] Apply COR-filter to the block-stream permanently
On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote: Hi all! Here is a new version of cor-filter in block-stream series. Main change is freezing the chain in cor-filter itself. Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
From: Andrey Shinkevich
This patch completes the series with the COR-filter applied to
block-stream operations.
Adding the filter makes it possible in future implement discarding
copied regions in backing files during the block-stream job, to reduce
the disk overuse (we need control on permissions).
Also, the filter now is smart enough to do copy-on-read with specified
base, so we have benefit on guest reads even when doing block-stream of
the part of the backing chain.
Several iotests are slightly modified due to filter insertion.
Signed-off-by: Andrey Shinkevich
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/stream.c | 105 ++---
tests/qemu-iotests/030 | 8 +--
tests/qemu-iotests/141.out | 2 +-
tests/qemu-iotests/245 | 20 ---
4 files changed, 80 insertions(+), 55 deletions(-)
[...]
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 08e0aecd65..028a16f365 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -99,7 +99,7 @@ wrote 1048576/1048576 bytes at offset 0
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data":
{"status": "created", "id": "job0"}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data":
{"status": "running", "id": "job0"}}
{'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
-{"error": {"class": "GenericError", "desc": "Node drv0 is in use"}}
+{"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is
in use by block job: stream"}}
{'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data":
{"status": "aborting", "id": "job0"}}
Amusing side note: This context matches two places in 141.out. With
0e720781282 (which requires two contextual whitespace tweaks), the file
gets longer, so the line number doesn’t match anymore. git then tries
to apply the context to the first match (which is closer to line 99),
which is wrong.
First time I had something like that happen, actually.
Max
Re: [PATCH] meson: Propagate gnutls dependency
On Mon, Jan 04, 2021 at 09:50:32PM +0100, Paolo Bonzini wrote:
> On 04/01/21 18:24, Roman Bolshakov wrote:
> > Hi Paolo,
> >
> > I'm sorry I didn't reply earlier. As I showed in an example to Peter
> > (https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg00085.html):
> > https://github.com/mesonbuild/meson/commit/ff5dc65ef841857dd306694dff1fb1cd2bf801e4
> >
> > The approach doesn't propogate dependencies of crypto beyond libcrypto.
> > i.e. if you specify crypto somewhere else as depedency, it won't pull
> > CFLAGS needed for gnutls.
>
> Hi Roman,
>
> After writing the meson patch in fact I noticed that get_dependencies() is
> used only for linker flags. I got a very quick reply from the Meson
> maintainer (https://github.com/mesonbuild/meson/pull/8151):
>
Thanks for providing a PR! I'll try if it works for QEMU with previous
proposal of fixing it (where we specify dependency in source set only
once and don't duplicate in declare_dependency).
I wonder if we should add a source set method like public_add to allow
the behavior we want and permit propogation of a dependency beyond
static_library without breaking all other users of meson out there?
>The fact that header flags are not passed transitively but libraries
>are (in some cases) is intentional. Otherwise compiler flag counts
>explode in deep hierarchies. Because of this include paths must be
>exported manually, typically by adding the appropriate bits to a
>declare_dependency.
>
>Libs are a bit stupid, because you need to add direct dependencies
>if, for example, you link to a static library.
>
> Does it work if you do:
>
> crypto_ss.add(authz, qom)
> libcrypto = static_library('crypto', crypto_ss.sources() + genh,
>dependencies: crypto_ss.dependencies(),
>...)
> crypto = declare_dependency(link_whole: libcrypto,
> dependencies: crypto_ss.dependencies())
>
I tried that approach before I sent the patch in the subject. It
produces duplicate symbols:
duplicate symbol '_qauthz_pam_new' in:
libcrypto.fa(authz_pamacct.c.o)
libauthz.fa(authz_pamacct.c.o)
[...]
duplicate symbol '_object_property_set_qobject' in:
libcrypto.fa(qom_qom-qobject.c.o)
libqom.fa(qom_qom-qobject.c.o)
My impression that it links in every static library that's mentioned in
dependencies of static_library, so they grow like a snow ball. Patch
below:
diff --git a/block/meson.build b/block/meson.build
index 7595d86c41..7eaf48c6dc 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -40,7 +40,7 @@ block_ss.add(files(
'vmdk.c',
'vpc.c',
'write-threshold.c',
-), zstd, zlib)
+), crypto, zstd, zlib)
softmmu_ss.add(when: 'CONFIG_TCG', if_true: files('blkreplay.c'))
diff --git a/hw/nvram/meson.build b/hw/nvram/meson.build
index fd2951a860..1f2ed013b2 100644
--- a/hw/nvram/meson.build
+++ b/hw/nvram/meson.build
@@ -1,6 +1,3 @@
-# QOM interfaces must be available anytime QOM is used.
-qom_ss.add(files('fw_cfg-interface.c'))
-
softmmu_ss.add(files('fw_cfg.c'))
softmmu_ss.add(when: 'CONFIG_CHRP_NVRAM', if_true: files('chrp_nvram.c'))
softmmu_ss.add(when: 'CONFIG_DS1225Y', if_true: files('ds1225y.c'))
diff --git a/io/meson.build b/io/meson.build
index bcd8b1e737..a844271b17 100644
--- a/io/meson.build
+++ b/io/meson.build
@@ -12,4 +12,4 @@ io_ss.add(files(
'dns-resolver.c',
'net-listener.c',
'task.c',
-))
+), crypto)
diff --git a/meson.build b/meson.build
index 372576f82c..1a8c653067 100644
--- a/meson.build
+++ b/meson.build
@@ -1538,6 +1538,34 @@ libqemuutil = static_library('qemuutil',
qemuutil = declare_dependency(link_with: libqemuutil,
sources: genh + version_res)
+# QOM interfaces must be available anytime QOM is used.
+qom_ss.add(files('hw/nvram/fw_cfg-interface.c'))
+qom_ss = qom_ss.apply(config_host, strict: false)
+libqom = static_library('qom', qom_ss.sources() + genh,
+dependencies: [qom_ss.dependencies()],
+name_suffix: 'fa')
+
+qom = declare_dependency(link_whole: libqom)
+
+authz_ss = authz_ss.apply(config_host, strict: false)
+libauthz = static_library('authz', authz_ss.sources() + genh,
+ dependencies: [authz_ss.dependencies()],
+ name_suffix: 'fa',
+ build_by_default: false)
+
+authz = declare_dependency(link_whole: libauthz,
+ dependencies: qom)
+
+crypto_ss.add(authz)
+crypto_ss = crypto_ss.apply(config_host, strict: false)
+libcrypto = static_library('crypto', crypto_ss.sources() + genh,
+ dependencies: crypto_ss.dependencies(),
+ name_suffix:
Re: To start multiple KVM guests from one qcow2 image with transient disk option
On Mon, Jan 04, 2021 at 15:30:19 -0500, Masayoshi Mizuma wrote:
> On Sat, Dec 19, 2020 at 11:30:39PM -0500, Masayoshi Mizuma wrote:
[...]
> I think following qemu command line options and QMP commands work for sharing
> the qcow2 disks. The following uses disk hotplug instead of snapshot overlay.
> Does that make sense for libvirt...?
>
> qemu command line options:
So you are proposing to ...
>
> qemu-system-x86_64 \
> -M q35,accel=kvm,usb=off,vmport=off,smm=on,dump-guest-core=off \
> -smp 1 \
> -m 4096 \
> -blockdev
> '{"driver":"file","filename":"/home/mmizuma/debug/guest.qcow2","node-name":"storage1","auto-read-only":true,"discard":"unmap"}'
> \
> -blockdev
> '{"node-name":"format1","read-only":true,"driver":"qcow2","file":"storage1","backing":null}'
> \
... start with the disk already in 'read-only' mode _and_ skip addition
of the disk ...
> -nographic \
> -nodefaults \
> -no-user-config \
> -serial telnet::1,server,nowait \
> -qmp tcp::10001,server,nowait \
> -S \
> -device pcie-root-port,id=pci.1
>
> QMP commands:
>
> {"execute":"qmp_capabilities"}
>
> {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/guest.TRANSIENT","node-name":"storage2","auto-read-only":true,"discard":"unmap"}}
>
> {"execute":"blockdev-create","arguments":{"job-id":"create","options":{"driver":"qcow2","file":"storage2","size":4294967296,"cluster-size":65536,"backing-file":"/var/lib/libvirt/images/guest.TRANSIENT","backing-fmt":"qcow2"}}}
>
> {"execute":"blockdev-add","arguments":{"node-name":"format2","read-only":false,"driver":"qcow2","file":"storage2"}}
... and then add a writable overlay ...
>
> {"execute":"device_add","arguments":{"driver":"virtio-blk-pci","drive":"format2","id":"transient-disk","bootindex":1,"bus":"pci.1","addr":0}}
... and hotplug the disk.
> {"execute":"cont"}
So that is a no-go. Some disk bus-es such as IDE don't support hotplug:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_hotplug.c#L1074
You could try to just instantiate the backend of the disk as read-only,
and then create a writable overlay. You just need to make sure that the
disk will be writable and that it works even for IDE/SATA which doesn't
support read-only:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_validate.c#L2634
Re: [PATCH v15 13/13] block: apply COR-filter to block-stream jobs
On 22.12.20 19:07, Vladimir Sementsov-Ogievskiy wrote:
22.12.2020 19:20, Max Reitz wrote:
On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote:
From: Andrey Shinkevich
This patch completes the series with the COR-filter applied to
block-stream operations.
Adding the filter makes it possible in future implement discarding
copied regions in backing files during the block-stream job, to reduce
the disk overuse (we need control on permissions).
Also, the filter now is smart enough to do copy-on-read with specified
base, so we have benefit on guest reads even when doing block-stream of
the part of the backing chain.
Several iotests are slightly modified due to filter insertion.
Signed-off-by: Andrey Shinkevich
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
block/stream.c | 105 ++---
tests/qemu-iotests/030 | 8 +--
tests/qemu-iotests/141.out | 2 +-
tests/qemu-iotests/245 | 20 ---
4 files changed, 80 insertions(+), 55 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 626dfa2b22..1fa742b0db 100644
--- a/block/stream.c
+++ b/block/stream.c
[...]
@@ -266,30 +251,62 @@ void stream_start(const char *job_id,
BlockDriverState *bs,
[...]
/* Make sure that the image is opened in read-write mode */
bs_read_only = bdrv_is_read_only(bs);
if (bs_read_only) {
- if (bdrv_reopen_set_read_only(bs, false, errp) != 0) {
- bs_read_only = false;
- goto fail;
+ int ret;
+ /* Hold the chain during reopen */
+ if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
+ return;
+ }
+
+ ret = bdrv_reopen_set_read_only(bs, false, errp);
+
+ /* failure, or cor-filter will hold the chain */
+ bdrv_unfreeze_backing_chain(bs, above_base);
+
+ if (ret < 0) {
Shouldn’t we keep the “bs_read_only = false;” here?
No, as we don't goto fail.
Ah, right, then it won’t do anything.
(pre-patch, we goto fail here, and don't want
fail: code path to reopend back to RW (as reopening to RO is failed
anyway (and we hope it's transactional enough)))
That’s why we had bs_read_only = false; pre-patch, so the reopen back to
RW is skipped.
And with this patch, we don’t need anything else from the “fail” path
(freezing is done by the filter, and the filter doesn’t exist yet), so
it’s correct to condense the “bs_read_only = false; goto fail;” into a
plain “return”.
Reviewed-by: Max Reitz
Re: [PATCH v15 10/13] qapi: block-stream: add "bottom" argument
On 22.12.20 19:00, Vladimir Sementsov-Ogievskiy wrote: 22.12.2020 19:07, Max Reitz wrote: On 16.12.20 07:17, Vladimir Sementsov-Ogievskiy wrote: The code already don't freeze base node and we try to make it prepared for the situation when base node is changed during the operation. In other words, block-stream doesn't own base node. Let's introduce a new interface which should replace the current one, which will in better relations with the code. Specifying bottom node instead of base, and requiring it to be non-filter gives us the following benefits: - drop difference between above_base and base_overlay, which will be renamed to just bottom, when old interface dropped - clean way to work with parallel streams/commits on the same backing chain, which otherwise become a problem when we introduce a filter for stream job - cleaner interface. Nobody will surprised the fact that base node may disappear during block-stream, when there is no word about "base" in the interface. Signed-off-by: Vladimir Sementsov-Ogievskiy --- qapi/block-core.json | 12 --- include/block/block_int.h | 1 + block/monitor/block-hmp-cmds.c | 3 +- block/stream.c | 50 +++- blockdev.c | 59 -- 5 files changed, 94 insertions(+), 31 deletions(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index b8094a5ec7..cb0066fd5c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2517,10 +2517,14 @@ # @device: the device or node name of the top image # # @base: the common backing file name. -# It cannot be set if @base-node is also set. +# It cannot be set if @base-node or @bottom is also set. # # @base-node: the node name of the backing file. -# It cannot be set if @base is also set. (Since 2.8) +# It cannot be set if @base or @bottom is also set. (Since 2.8) +# +# @bottom: the last node in the chain that should be streamed into +# top. It cannot be set if @base or @base-node is also set. +# It cannot be filter node. (Since 6.0) As far as I can make out, one of the results of our discussion on v14 was that when using backing-file + bottom, we want to require the user to specify backing-fmt as well. Now, backing-fmt isn’t present yet. Doesn’t that mean we have to make bottom + backing-file an error until we have backing-fmt (like it was in v14)? See my answer on 09. I just have some doubts around backing-fmt and decided to keep it as is. I don't think that we really need backing-fmt. We shouldn't have use-cases when backing-fmt is set to something another than final base node. Therefore, using format_name of final base node is a correct thing to do. So, I don't see the reason now for introducing new option. Yup, yup, all good. Reviewed-by: Max Reitz
Re: [PATCH v15 09/13] stream: rework backing-file changing
On 22.12.20 18:53, Vladimir Sementsov-Ogievskiy wrote:
22.12.2020 18:59, Max Reitz wrote:
On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
From: Andrey Shinkevich
Stream in stream_prepare calls bdrv_change_backing_file() to change
backing-file in the metadata of bs.
It may use either backing-file parameter given by user or just take
filename of base on job start.
Backing file format is determined by base on job finish.
There are some problems with this design, we solve only two by this
patch:
1. Consider scenario with backing-file unset. Current concept of stream
supports changing of the base during the job (we don't freeze link to
the base). So, we should not save base filename at job start,
- let's determine name of the base on job finish.
2. Using direct base to determine filename and format is not very good:
base node may be a filter, so its filename may be JSON, and format_name
is not good for storing into qcow2 metadata as backing file format.
- let's use unfiltered_base
Signed-off-by: Andrey Shinkevich
Signed-off-by: Vladimir Sementsov-Ogievskiy
[vsementsov: change commit subject, change logic in stream_prepare]
---
block/stream.c | 9 +
blockdev.c | 8 +---
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index 6e281c71ac..6a525a5edf 100644
--- a/block/stream.c
+++ b/block/stream.c
[...]
@@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
- if (base) {
- base_id = s->backing_file_str;
- if (base->drv) {
- base_fmt = base->drv->format_name;
+ if (unfiltered_base) {
+ base_id = s->backing_file_str ?: unfiltered_base->filename;
+ if (unfiltered_base->drv) {
+ base_fmt = unfiltered_base->drv->format_name;
}
}
bdrv_set_backing_hd(unfiltered_bs, base, _err);
I think I preferred the v14 behavior of not setting a backing file
format if backing_file_str is nowhere to be found in the current
backing chain. (I just noticed, I had a typo in my reply to v14,
though; the “continuing on with setting a backing_fmt” should have
read “continuing on *without* setting a backing_fmt”...)
Anyway, this is still an improvement on the pre-patch behavior, so:
Reviewed-by: Max Reitz
(And as we discussed, the best would be for the user to specify a
backing format through a yet-to-be-added option.)
We discussed that the original aim of backing_file_str arg is something
like fd-passing, when qemu doesn't know real name. In that way what was
done in v14 is a degradation: we'll never find such name in a backing
chain. And acutally, using format of backing file is a correct thing.
It was my understanding that this was an example use case; other users
might want to use backing-file for something else entirely. (I imagined
using e.g. a completely different file in a different format.)
OTOH, considering that “something else entirely” simply doesn’t work
(because the driver has to be something from the backing chain), my
imagination was just too wild. If anyone should ever want to follow up
on it, I expect them to complain, and we’ll worry about it then.
So, as I understand now:
We set backing file to the node which is the new backing-bs (maybe,
skipping some filters). Nobody should set backing in qcow2 metadata to
something absolutely different. So, using format_name of backing bs
(skipping filters) is a correct thing.
We want to support cases when qemu doens't know real file-names. So,
trying to check filename, or search it in a backing chain is wrong idea..
Hmm, or when we search backing name, we really track what was written in
backing_file field of some qcow2 image in a chain, so it should be
something correct?
If it does something else than what people want it to do, they’ll
complain. :)
(It isn’t like this patch is breaking anything that would work right now.)
So I agree that we don’t need backing-fmt now. (Or maybe ever.)
Max
Max
Re: [PATCH] tests/iotests: drop test 312 from auto group
On 1/5/21 11:12 AM, Philippe Mathieu-Daudé wrote:
> On 1/5/21 11:04 AM, Alex Bennée wrote:
>> The "auto" documentation states:
>>
>> That means they should run with every QEMU binary (also non-x86)
>>
>> which is not the case as the check-system-fedora build which only
>> includes a rag tag group of rare and deprecated targets doesn't
>> support the virtio device required.
>>
>
> Fixes: ef9bba1484b ("quorum: Implement bdrv_co_block_status()")
> Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
Re: [PATCH] tests/iotests: drop test 312 from auto group
On 1/5/21 11:04 AM, Alex Bennée wrote:
> The "auto" documentation states:
>
> That means they should run with every QEMU binary (also non-x86)
>
> which is not the case as the check-system-fedora build which only
> includes a rag tag group of rare and deprecated targets doesn't
> support the virtio device required.
>
Fixes: ef9bba1484b ("quorum: Implement bdrv_co_block_status()")
Reviewed-by: Philippe Mathieu-Daudé
> Signed-off-by: Alex Bennée
> ---
> tests/qemu-iotests/group | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index e4fb6327ae..bc5bc324fe 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -318,4 +318,4 @@
> 307 rw quick export
> 308 rw
> 309 rw auto quick
> -312 rw auto quick
> +312 rw quick
>
[PATCH] tests/iotests: drop test 312 from auto group
The "auto" documentation states: That means they should run with every QEMU binary (also non-x86) which is not the case as the check-system-fedora build which only includes a rag tag group of rare and deprecated targets doesn't support the virtio device required. Signed-off-by: Alex Bennée --- tests/qemu-iotests/group | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index e4fb6327ae..bc5bc324fe 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -318,4 +318,4 @@ 307 rw quick export 308 rw 309 rw auto quick -312 rw auto quick +312 rw quick -- 2.20.1
Ping: [PATCH v4 0/7] block: Add retry for werror=/rerror= mechanism
Hi Kevin, What do you think of these patches? Thanks, Jiahui On 2020/12/15 20:30, Jiahui Cen wrote: > A VM in the cloud environment may use a virutal disk as the backend storage, > and there are usually filesystems on the virtual block device. When backend > storage is temporarily down, any I/O issued to the virtual block device > will cause an error. For example, an error occurred in ext4 filesystem would > make the filesystem readonly. In production environment, a cloud backend > storage can be soon recovered. For example, an IP-SAN may be down due to > network failure and will be online soon after network is recovered. However, > the error in the filesystem may not be recovered unless a device reattach > or system restart. Thus an I/O retry mechanism is in need to implement a > self-healing system. > > This patch series propose to extend the werror=/rerror= mechanism to add > a 'retry' feature. It can automatically retry failed I/O requests on error > without sending error back to guest, and guest can get back running smoothly > when I/O is recovred. > > v3->v4: > * Adapt to werror=/rerror= mechanism. > > v2->v3: > * Add a doc to describe I/O hang. > > v1->v2: > * Rebase to fix compile problems. > * Fix incorrect remove of rehandle list. > * Provide rehandle pause interface. > > REF: https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg06560.html > > Signed-off-by: Jiahui Cen > Signed-off-by: Ying Fang > > Jiahui Cen (7): > qapi/block-core: Add retry option for error action > block-backend: Introduce retry timer > block-backend: Add device specific retry callback > block-backend: Enable retry action on errors > block-backend: Add timeout support for retry > block: Add error retry param setting > virtio_blk: Add support for retry on errors > > block/block-backend.c | 66 > blockdev.c | 52 +++ > hw/block/block.c | 10 +++ > hw/block/virtio-blk.c | 19 +- > include/hw/block/block.h | 7 ++- > include/sysemu/block-backend.h | 10 +++ > qapi/block-core.json | 4 +- > 7 files changed, 162 insertions(+), 6 deletions(-) >
