Re: [PATCH v2 1/2] hw: Replace anti-social QOM type names
On 04/03/2021 14:02, Markus Armbruster wrote:
Several QOM type names contain ',':
ARM,bitband-memory
etraxfs,pic
etraxfs,serial
etraxfs,timer
fsl,imx25
fsl,imx31
fsl,imx6
fsl,imx6ul
fsl,imx7
grlib,ahbpnp
grlib,apbpnp
grlib,apbuart
grlib,gptimer
grlib,irqmp
qemu,register
SUNW,bpp
SUNW,CS4231
SUNW,DBRI
SUNW,DBRI.prom
SUNW,fdtwo
SUNW,sx
SUNW,tcx
xilinx,zynq_slcr
xlnx,zynqmp
xlnx,zynqmp-pmu-soc
xlnx,zynq-xadc
These are all device types. They can't be plugged with -device /
device_add, except for xlnx,zynqmp-pmu-soc, and I doubt that one
actually works.
They *can* be used with -device / device_add to request help.
Usability is poor, though: you have to double the comma, like this:
$ qemu-system-x86_64 -device SUNW,,fdtwo,help
Trap for the unwary. The fact that this was broken in
device-introspect-test for more than six years until commit e27bd49876
fixed it demonstrates that "the unwary" includes seasoned developers.
One QOM type name contains ' ': "ICH9 SMB". Because having to
remember just one way to quote would be too easy.
Rename the "SUNW,FOO types to "sun-FOO". Summarily replace ',' and '
' by '-' in the other type names.
Signed-off-by: Markus Armbruster
---
include/hw/arm/armv7m.h | 2 +-
include/hw/arm/fsl-imx25.h | 2 +-
include/hw/arm/fsl-imx31.h | 2 +-
include/hw/arm/fsl-imx6.h| 2 +-
include/hw/arm/fsl-imx6ul.h | 2 +-
include/hw/arm/fsl-imx7.h| 2 +-
include/hw/arm/xlnx-zynqmp.h | 2 +-
include/hw/cris/etraxfs.h| 2 +-
include/hw/i386/ich9.h | 2 +-
include/hw/misc/grlib_ahb_apb_pnp.h | 4 ++--
include/hw/misc/zynq-xadc.h | 2 +-
include/hw/register.h| 2 +-
include/hw/sparc/grlib.h | 6 +++---
hw/arm/xilinx_zynq.c | 2 +-
hw/audio/cs4231.c| 2 +-
hw/block/fdc.c | 4 ++--
hw/char/etraxfs_ser.c| 2 +-
hw/cris/axis_dev88.c | 6 +++---
hw/display/tcx.c | 2 +-
hw/intc/etraxfs_pic.c| 2 +-
hw/microblaze/xlnx-zynqmp-pmu.c | 2 +-
hw/misc/zynq_slcr.c | 2 +-
hw/sparc/sun4m.c | 12 ++--
hw/timer/etraxfs_timer.c | 2 +-
softmmu/vl.c | 2 +-
tests/vmstate-static-checker-data/dump1.json | 4 ++--
tests/vmstate-static-checker-data/dump2.json | 4 ++--
27 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index 0791dcb68a..189b23a8ce 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -15,7 +15,7 @@
#include "target/arm/idau.h"
#include "qom/object.h"
-#define TYPE_BITBAND "ARM,bitband-memory"
+#define TYPE_BITBAND "ARM-bitband-memory"
OBJECT_DECLARE_SIMPLE_TYPE(BitBandState, BITBAND)
struct BitBandState {
diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
index c1603b2ac2..1b1086e945 100644
--- a/include/hw/arm/fsl-imx25.h
+++ b/include/hw/arm/fsl-imx25.h
@@ -34,7 +34,7 @@
#include "target/arm/cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX25 "fsl,imx25"
+#define TYPE_FSL_IMX25 "fsl-imx25"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX25State, FSL_IMX25)
#define FSL_IMX25_NUM_UARTS 5
diff --git a/include/hw/arm/fsl-imx31.h b/include/hw/arm/fsl-imx31.h
index b9792d58ae..c116a73e0b 100644
--- a/include/hw/arm/fsl-imx31.h
+++ b/include/hw/arm/fsl-imx31.h
@@ -30,7 +30,7 @@
#include "target/arm/cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX31 "fsl,imx31"
+#define TYPE_FSL_IMX31 "fsl-imx31"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX31State, FSL_IMX31)
#define FSL_IMX31_NUM_UARTS 2
diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 29cc425acc..83291457cf 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -36,7 +36,7 @@
#include "cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX6 "fsl,imx6"
+#define TYPE_FSL_IMX6 "fsl-imx6"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX6State, FSL_IMX6)
#define FSL_IMX6_NUM_CPUS 4
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index f8ebfba4f9..7812e516a5 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -40,7 +40,7 @@
#include "cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX6UL "fsl,imx6ul"
+#define TYPE_FSL_IMX6UL "fsl-imx6ul"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX6ULState, FSL_IMX6UL)
enum FslIMX6ULConfiguration {
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 161
Re: [PATCH v2 1/3] fdc: Drop deprecated floppy configuration
Peter Krempa writes:
> On Thu, Mar 04, 2021 at 17:23:05 +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé writes:
>>
>> > On Thu, Mar 04, 2021 at 03:26:55PM +0100, Markus Armbruster wrote:
>> >> Daniel P. Berrangé writes:
>> >>
>> >> > On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote:
>> >> >> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate
>> >> >> configuring floppies with -global isa-fdc" (v5.1.0).
>> >> >>
>> >> >> Signed-off-by: Markus Armbruster
>> >> >> ---
>> >> >> docs/system/deprecated.rst | 26 --
>> >> >> docs/system/removed-features.rst | 26 ++
>> >> >> hw/block/fdc.c | 54 +--
>> >> >> tests/qemu-iotests/172 | 31 +-
>> >> >> tests/qemu-iotests/172.out | 562 +--
>> >> >> 5 files changed, 30 insertions(+), 669 deletions(-)
>
> [...]
>
>> >>
>> >> Correct.
>> >>
>> >> This was deprecated in commit 4a27a638e7 "fdc: Deprecate configuring
>> >> floppies with -global isa-fdc" (v5.1.0). Since then, its use triggers a
>> >> warning:
>> >>
>> >> $ qemu-system-x86_64 -nodefaults -M q35 -display none -drive
>> >> if=none,id=drive-fdc0-0-0 -device
>> >> isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1
>> >> qemu-system-x86_64: -device
>> >> isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1: warning: warning: property
>> >> isa-fdc.driveA is deprecated
>> >> Use -device floppy,unit=0,drive=... instead.
>> >>
>> >> Note the -M q35. Needed because the default machine type has an onboard
>> >> isa-fdc, which cannot be configured this way.
>> >>
>> >> Sadly, the commit's update of docs/system/deprecated.rst neglects to
>> >> cover this use. Looks the series overtaxed my capacity to juggle
>> >> details; my apologies.
>> >>
>> >> Is libvirt still using these properties?
>> >
>> > Unfortunately yes, but it seems like it ought to be fairly easy to
>> > change the syntax. Just need to figure out what the right way to
>> > detect the availability of the new syntax is. Presumably just look
>> > for existance of the 'floppy' device type ?
>>
>> Yes. The device type was added in merge commit fd209e4a7, v2.8.0.
>>
>> > Can you confirm that switching from -global to the new -device floppy
>> > does /not/ have any live migration impact ?
>>
>> Yes, it must not affect migration.
>>
>> When Kevin split the floppy device type off the floppy controller, he
>> had to add some moderately ugly hackery to keep the old qdev properties
>> working. Think propagate property values to floppy from controller,
>> which otherwise ignores them.
>>
>> The way you get the values into the floppy device cannot affect the
>> migration data. Only different values can.
>>
>> This patch removes a deprecated way.
>
> Note that when QEMU_CAPS_BLOCKDEV is asserted we format floppies as:
>
> -blockdev '{"driver":"file","filename":"/tmp/firmware.img",\
> "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
> -blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\
> "file":"libvirt-2-storage"}' \
> -device floppy,unit=0,drive=libvirt-2-format,id=fdc0-0-0 \
> -blockdev '{"driver":"file","filename":"/tmp/data.img",\
> "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
> -blockdev
> '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2",\
> "file":"libvirt-1-storage"}' \
> -device floppy,unit=1,drive=libvirt-1-format,id=fdc0-0-1 \
>
> as visible in the test file:
>
> tests/qemuxml2argvdata/disk-floppy-q35-2_11.x86_64-latest.args
>
> So libvirt should be in the clear. isa-fdc with driveA/driveB is
> formatted only when the blockdev capability isn't present.
Even better: in the clear for some time already, which means a wider
range of libvirt versions keeps working with the latest QEMU. Nice,
because keeping the coupling reasonably lose makes upgrading easier.
Thanks!
Re: [RESEND][BUG FIX HELP] QEMU main thread endlessly hangs in __ppoll()
Hi John,
Thanks for your comment.
On 2021/3/5 7:53, John Snow wrote:
On 2/28/21 9:39 PM, Like Xu wrote:
Hi Genius,
I am a user of QEMU v4.2.0 and stuck in an interesting bug, which may
still exist in the mainline.
Thanks in advance to heroes who can take a look and share understanding.
Do you have a test case that reproduces on 5.2? It'd be nice to know if it
was still a problem in the latest source tree or not.
We narrowed down the source of the bug, which basically came from
the following qmp usage:
{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
'drive_del replication0' } }
One of the test cases is the COLO usage (docs/colo-proxy.txt).
This issue is sporadic,the probability may be 1/15 for a io-heavy guest.
I believe it's reproducible on 5.2 and the latest tree.
--js
The qemu main thread endlessly hangs in the handle of the qmp statement:
{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
'drive_del replication0' } }
and we have the call trace looks like:
#0 0x7f3c22045bf6 in __ppoll (fds=0x555611328410, nfds=1,
timeout=, timeout@entry=0x7ffc56c66db0,
sigmask=sigmask@entry=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:44
#1 0x55561021f415 in ppoll (__ss=0x0, __timeout=0x7ffc56c66db0,
__nfds=, __fds=)
at /usr/include/x86_64-linux-gnu/bits/poll2.h:77
#2 qemu_poll_ns (fds=, nfds=,
timeout=) at util/qemu-timer.c:348
#3 0x555610221430 in aio_poll (ctx=ctx@entry=0x5556113010f0,
blocking=blocking@entry=true) at util/aio-posix.c:669
#4 0x55561019268d in bdrv_do_drained_begin (poll=true,
ignore_bds_parents=false, parent=0x0, recursive=false,
bs=0x55561138b0a0) at block/io.c:430
#5 bdrv_do_drained_begin (bs=0x55561138b0a0, recursive=,
parent=0x0, ignore_bds_parents=,
poll=) at block/io.c:396
#6 0x55561017b60b in quorum_del_child (bs=0x55561138b0a0,
child=0x7f36dc0ce380, errp=)
at block/quorum.c:1063
#7 0x55560ff5836b in qmp_x_blockdev_change (parent=0x555612373120
"colo-disk0", has_child=,
child=0x5556112df3e0 "children.1", has_node=, node=0x0,
errp=0x7ffc56c66f98) at blockdev.c:4494
#8 0x5556100f8f57 in qmp_marshal_x_blockdev_change (args=out>, ret=, errp=0x7ffc56c67018)
at qapi/qapi-commands-block-core.c:1538
#9 0x5556101d8290 in do_qmp_dispatch (errp=0x7ffc56c67010,
allow_oob=, request=,
cmds=0x5556109c69a0 ) at qapi/qmp-dispatch.c:132
#10 qmp_dispatch (cmds=0x5556109c69a0 , request=out>, allow_oob=)
at qapi/qmp-dispatch.c:175
#11 0x5556100d4c4d in monitor_qmp_dispatch (mon=0x5556113a6f40,
req=) at monitor/qmp.c:145
#12 0x5556100d5437 in monitor_qmp_bh_dispatcher (data=out>) at monitor/qmp.c:234
#13 0x55561021dbec in aio_bh_call (bh=0x5556112164bGrateful0) at
util/async.c:117
#14 aio_bh_poll (ctx=ctx@entry=0x5556112151b0) at util/async.c:117
#15 0x5556102212c4 in aio_dispatch (ctx=0x5556112151b0) at
util/aio-posix.c:459
#16 0x55561021dab2 in aio_ctx_dispatch (source=,
callback=, user_data=)
at util/async.c:260
#17 0x7f3c22302fbd in g_main_context_dispatch () from
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x555610220358 in glib_pollfds_poll () at util/main-loop.c:219
#19 os_host_main_loop_wait (timeout=) at util/main-loop.c:242
#20 main_loop_wait (nonblocking=) at util/main-loop.c:518
#21 0x55560ff600fe in main_loop () at vl.c:1814
#22 0x55560fddbce9 in main (argc=, argv=out>, envp=) at vl.c:4503
We found that we're doing endless check in the line of
block/io.c:bdrv_do_drained_begin():
BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent));
and it turns out that the bdrv_drain_poll() always get true from:
- bdrv_parent_drained_poll(bs, ignore_parent, ignore_bds_parents)
- AND atomic_read(&bs->in_flight)
I personally think this is a deadlock issue in the a QEMU block layer
(as we know, we have some #FIXME comments in related codes, such as block
permisson update).
Any comments are welcome and appreciated.
---
thx,likexu
Re: [PATCH] hw/sd/sdhci: Report error when guest access protected registers
On Fri, Feb 19, 2021 at 1:56 AM Philippe Mathieu-Daudé wrote: > > The SDHC_SYSAD and SDHC_BLKSIZE can not be accessed while a > transaction is in progress, see 'SD Host Controller Simplified > Specification' > > 1.5) SD Command Generation > > The Host Driver should not read the SDMA System Address, Block > Size and Block Count registers during a data transaction unless > the transfer is stopped because the value is changing and not > stable. > > Report guest intents as errors. > > Signed-off-by: Philippe Mathieu-Daudé > --- > Based-on: <1613447214-81951-1-git-send-email-bmeng...@gmail.com> > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/sd/sdhci.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) > Reviewed-by: Bin Meng
Re: [PATCH v2 4/8] simplebench/bench-backup: add target-cache argument
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote: Allow benchmark with different kinds of target cache. Signed-off-by: Vladimir Sementsov-Ogievskiy --- scripts/simplebench/bench-backup.py| 33 -- scripts/simplebench/bench_block_job.py | 10 +--- 2 files changed, 33 insertions(+), 10 deletions(-) Admittedly just skimming a bit on this one, it looks OK. Reviewed-by: John Snow
Re: [PATCH v2 5/8] simplebench/bench_block_job: handle error in BLOCK_JOB_COMPLETED
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
We should not report success if there is an error in final event.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench_block_job.py | 4
1 file changed, 4 insertions(+)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 8f8385ccce..71d2e489c8 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -70,6 +70,10 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.shutdown()
return {'error': 'block-job failed: ' + str(e),
'vm-log': vm.get_log()}
+if 'error' in e['data']:
+vm.shutdown()
+return {'error': 'block-job failed: ' + e['data']['error'],
+'vm-log': vm.get_log()}
end_ms = e['timestamp']['seconds'] * 100 + \
e['timestamp']['microseconds']
finally:
"Yes, the block job completed -- but not how you wanted it to."
Reviewed-by: John Snow
Re: [PATCH v2 6/8] simplebench/bench-backup: support qcow2 source files
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
Add support for qcow2 source. New option says to use test-source.qcow2
instead of test-source. Of course, test-source.qcow2 should be
precreated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench-backup.py| 5 +
scripts/simplebench/bench_block_job.py | 7 ++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/scripts/simplebench/bench-backup.py
b/scripts/simplebench/bench-backup.py
index fbc85f266f..a2120fcbf0 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -58,6 +58,8 @@ def bench(args):
if src == 'nbd':
source = nbd_drv
+elif args.qcow2_sources:
+source = drv_qcow2(drv_file(dirs[src] + '/test-source.qcow2'))
else:
source = drv_file(dirs[src] + '/test-source')
@@ -199,6 +201,9 @@ def __call__(self, parser, namespace, values, option_string=None):
Use compressed backup. It automatically means
automatically creating qcow2 target with
lazy_refcounts for each test run''', action='store_true')
+p.add_argument('--qcow2-sources', help='''\
+Use test-source.qcow2 images as sources instead of
+test-source raw images''', action='store_true')
p.add_argument('--target-cache', help='''\
Setup cache for target nodes. Options:
direct: default, use O_DIRECT and aio=native
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 71d2e489c8..4f03c12169 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -88,6 +88,11 @@ def get_image_size(path):
return json.loads(out)['virtual-size']
+def get_blockdev_size(obj):
+img = obj['filename'] if 'filename' in obj else obj['file']['filename']
+return get_image_size(img)
+
Well, as long as it works :)
+
# Bench backup or mirror
def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
"""Helper to run bench_block_job() for mirror or backup"""
@@ -101,7 +106,7 @@ def bench_block_copy(qemu_binary, cmd, cmd_options, source,
target):
subprocess.run(['qemu-img', 'create', '-f', 'qcow2',
target['file']['filename'],
-str(get_image_size(source['filename']))],
+str(get_blockdev_size(source))],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL, check=True)
Reviewed-by: John Snow
Re: [PATCH v2 7/8] simplebench/bench-backup: add --count and --no-initial-run
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
Add arguments to set number of test runs per table cell and to disable
initial run that is not counted in results.
It's convenient to set --count 1 --no-initial-run to fast run test
onece, and to set --count to some large enough number for good
precision of the results.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench-backup.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/simplebench/bench-backup.py
b/scripts/simplebench/bench-backup.py
index a2120fcbf0..519a985a7f 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -155,7 +155,9 @@ def bench(args):
'qemu-binary': path
})
-result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
+result = simplebench.bench(bench_func, test_envs, test_cases,
+ count=args.count,
+ initial_run = not args.no_initial_run)
The double negative feels odd; "initial_run = args.initial_run" would
read better and avoid changing behavior, but maybe that's intentional.
with open('results.json', 'w') as f:
json.dump(result, f, indent=4)
print(results_to_text(result))
@@ -211,4 +213,10 @@ def __call__(self, parser, namespace, values,
option_string=None):
both: generate two test cases for each src:dst pair''',
default='direct', choices=('direct', 'cached', 'both'))
+p.add_argument('--count', type=int, default=3, help='''\
+Number of test runs per table cell''')
+
+p.add_argument('--no-initial-run', action='store_true', help='''\
+Don't do initial run of test for each cell which doesn't count''')
+
bench(p.parse_args())
Re: [PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
It probably may improve reliability of results when testing in cached
mode.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench_block_job.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
+subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
+ check=True)
res = vm.qmp(cmd, **cmd_args)
if res != {'return': {}}:
vm.shutdown()
Worth adding a conditional to allow "hot" or "cold" runs? nah?
Re: [PATCH v2 2/8] simplebench: bench_one(): support count=1
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote: statistics.stdev raises if sequence length is less than two. Support that case by hand. Signed-off-by: Vladimir Sementsov-Ogievskiy --- scripts/simplebench/simplebench.py | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py index b153cae274..712e1f845b 100644 --- a/scripts/simplebench/simplebench.py +++ b/scripts/simplebench/simplebench.py @@ -92,7 +92,10 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True, dim = 'seconds' result['dimension'] = dim result['average'] = statistics.mean(r[dim] for r in succeeded) -result['stdev'] = statistics.stdev(r[dim] for r in succeeded) +if len(succeeded) == 1: +result['stdev'] = 0 +else: +result['stdev'] = statistics.stdev(r[dim] for r in succeeded) if len(succeeded) < count: result['n-failed'] = count - len(succeeded) Omitted from patch context is that we have "if succeeded: ..." so we know it's at least 1 here. Reviewed-by: John Snow
Re: [PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument
On 3/4/21 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
Sometimes one of cells in a testing table runs too slow. And we really
don't want to wait so long. Limit number of runs in this case.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/simplebench.py | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/simplebench/simplebench.py
b/scripts/simplebench/simplebench.py
index f61513af90..b153cae274 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -19,9 +19,11 @@
#
import statistics
+import time
-def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
+def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
+ slow_limit=100):
"""Benchmark one test-case
test_func -- benchmarking function with prototype
@@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, count=5,
initial_run=True):
test_case -- test case - opaque second argument for test_func
count -- how many times to call test_func, to calculate average
initial_run -- do initial run of test_func, which don't get into result
+slow_limit -- reduce test runs to 2, if current run exceedes the limit
+ (in seconds)
s/exceedes/exceeds, and you need to mention that if the initial run
exceeds the limit, it will change the behavior to count that result.
It is also possible (conceivably) that the initial run exceeds the
limit, but subsequent runs don't, so it might be hard to predict how
many tests it'll actually run.
If you're OK with that behavior, maybe:
"Consider a test run 'slow' once it exceeds this limit, in seconds.
Stop early once there are two 'slow' runs, including the initial run.
Slow initial runs will be included in the results."
Lastly, this will change existing behavior -- do we care? Should it
default to None instead? Should we be able to pass None or 0 to disable
this behavior?
Returns dict with the following fields:
'runs': list of test_func results
@@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, count=5,
initial_run=True):
'n-failed': number of failed runs (exists only if at least one run
failed)
"""
+runs = []
+i = 0
if initial_run:
+t = time.time()
+
print(' #initial run:')
-print(' ', test_func(test_env, test_case))
+res = test_func(test_env, test_case)
+print(' ', res)
+
+if time.time() - t > slow_limit:
+print('- initial run is too slow, so it counts')
+runs.append(res)
+i = 1
+
+for i in range(i, count):
+t = time.time()
-runs = []
-for i in range(count):
print(' #run {}'.format(i+1))
res = test_func(test_env, test_case)
print(' ', res)
runs.append(res)
+if time.time() - t > slow_limit and len(runs) >= 2:
+print('- run is too slow, and we have enough runs, stop here')
+break
+
+count = len(runs)
+
result = {'runs': runs}
succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]
Re: [RESEND][BUG FIX HELP] QEMU main thread endlessly hangs in __ppoll()
On 2/28/21 9:39 PM, Like Xu wrote:
Hi Genius,
I am a user of QEMU v4.2.0 and stuck in an interesting bug, which may
still exist in the mainline.
Thanks in advance to heroes who can take a look and share understanding.
Do you have a test case that reproduces on 5.2? It'd be nice to know if
it was still a problem in the latest source tree or not.
--js
The qemu main thread endlessly hangs in the handle of the qmp statement:
{'execute': 'human-monitor-command', 'arguments':{ 'command-line':
'drive_del replication0' } }
and we have the call trace looks like:
#0 0x7f3c22045bf6 in __ppoll (fds=0x555611328410, nfds=1,
timeout=, timeout@entry=0x7ffc56c66db0,
sigmask=sigmask@entry=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:44
#1 0x55561021f415 in ppoll (__ss=0x0, __timeout=0x7ffc56c66db0,
__nfds=, __fds=)
at /usr/include/x86_64-linux-gnu/bits/poll2.h:77
#2 qemu_poll_ns (fds=, nfds=,
timeout=) at util/qemu-timer.c:348
#3 0x555610221430 in aio_poll (ctx=ctx@entry=0x5556113010f0,
blocking=blocking@entry=true) at util/aio-posix.c:669
#4 0x55561019268d in bdrv_do_drained_begin (poll=true,
ignore_bds_parents=false, parent=0x0, recursive=false,
bs=0x55561138b0a0) at block/io.c:430
#5 bdrv_do_drained_begin (bs=0x55561138b0a0, recursive=,
parent=0x0, ignore_bds_parents=,
poll=) at block/io.c:396
#6 0x55561017b60b in quorum_del_child (bs=0x55561138b0a0,
child=0x7f36dc0ce380, errp=)
at block/quorum.c:1063
#7 0x55560ff5836b in qmp_x_blockdev_change (parent=0x555612373120
"colo-disk0", has_child=,
child=0x5556112df3e0 "children.1", has_node=, node=0x0,
errp=0x7ffc56c66f98) at blockdev.c:4494
#8 0x5556100f8f57 in qmp_marshal_x_blockdev_change (args=out>, ret=, errp=0x7ffc56c67018)
at qapi/qapi-commands-block-core.c:1538
#9 0x5556101d8290 in do_qmp_dispatch (errp=0x7ffc56c67010,
allow_oob=, request=,
cmds=0x5556109c69a0 ) at qapi/qmp-dispatch.c:132
#10 qmp_dispatch (cmds=0x5556109c69a0 , request=out>, allow_oob=)
at qapi/qmp-dispatch.c:175
#11 0x5556100d4c4d in monitor_qmp_dispatch (mon=0x5556113a6f40,
req=) at monitor/qmp.c:145
#12 0x5556100d5437 in monitor_qmp_bh_dispatcher (data=out>) at monitor/qmp.c:234
#13 0x55561021dbec in aio_bh_call (bh=0x5556112164bGrateful0) at
util/async.c:117
#14 aio_bh_poll (ctx=ctx@entry=0x5556112151b0) at util/async.c:117
#15 0x5556102212c4 in aio_dispatch (ctx=0x5556112151b0) at
util/aio-posix.c:459
#16 0x55561021dab2 in aio_ctx_dispatch (source=,
callback=, user_data=)
at util/async.c:260
#17 0x7f3c22302fbd in g_main_context_dispatch () from
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x555610220358 in glib_pollfds_poll () at util/main-loop.c:219
#19 os_host_main_loop_wait (timeout=) at
util/main-loop.c:242
#20 main_loop_wait (nonblocking=) at util/main-loop.c:518
#21 0x55560ff600fe in main_loop () at vl.c:1814
#22 0x55560fddbce9 in main (argc=, argv=out>, envp=) at vl.c:4503
We found that we're doing endless check in the line of
block/io.c:bdrv_do_drained_begin():
BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent));
and it turns out that the bdrv_drain_poll() always get true from:
- bdrv_parent_drained_poll(bs, ignore_parent, ignore_bds_parents)
- AND atomic_read(&bs->in_flight)
I personally think this is a deadlock issue in the a QEMU block layer
(as we know, we have some #FIXME comments in related codes, such as
block permisson update).
Any comments are welcome and appreciated.
---
thx,likexu
Re: [PATCH] hw/sd/sdhci: Report error when guest access protected registers
On 2/18/21 12:56 PM, Philippe Mathieu-Daudé wrote:
The SDHC_SYSAD and SDHC_BLKSIZE can not be accessed while a
transaction is in progress, see 'SD Host Controller Simplified
Specification'
1.5) SD Command Generation
The Host Driver should not read the SDMA System Address, Block
Size and Block Count registers during a data transaction unless
the transfer is stopped because the value is changing and not
stable.
Naive question: Is this an RFC2119 "SHOULD NOT"? (i.e., does it have a
defined behavior that you are simply encouraged to avoid?)
Is it really an error?
Report guest intents as errors.
Signed-off-by: Philippe Mathieu-Daudé
---
Based-on: <1613447214-81951-1-git-send-email-bmeng...@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé
---
hw/sd/sdhci.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 5b8678110b0..98928c18542 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1136,6 +1136,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
unsigned size)
sdhci_sdma_transfer_single_block(s);
}
}
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Transfer already in progress,"
+ " can not update SYSAD", __func__);
}
break;
case SDHC_BLKSIZE:
@@ -1163,8 +1167,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val,
unsigned size)
if (blksize != s->blksize) {
s->data_count = 0;
}
+} else {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Transfer already in progress,"
+ " can not update BLKSIZE", __func__);
}
-
break;
case SDHC_ARGUMENT:
MASKED_WRITE(s->argument, mask, value);
Re: [PATCH v2 2/2] memory: Drop "qemu:" prefix from QOM memory region type names
On Thu, Mar 4, 2021 at 9:03 AM Markus Armbruster wrote: > > Almost all QOM type names consist only of letters, digits, '-', '_', > and '.'. Just two contain ':': "qemu:memory-region" and > "qemu:iommu-memory-region". Neither can be plugged with -object. > Rename them to "memory-region" and "iommu-memory-region". > > Signed-off-by: Markus Armbruster Reviewed-by: Alistair Francis Alistair > --- > include/exec/memory.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index c6fb714e49..3c95d7831a 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -33,11 +33,11 @@ > #define MAX_PHYS_ADDR_SPACE_BITS 62 > #define MAX_PHYS_ADDR(((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - > 1) > > -#define TYPE_MEMORY_REGION "qemu:memory-region" > +#define TYPE_MEMORY_REGION "memory-region" > DECLARE_INSTANCE_CHECKER(MemoryRegion, MEMORY_REGION, > TYPE_MEMORY_REGION) > > -#define TYPE_IOMMU_MEMORY_REGION "qemu:iommu-memory-region" > +#define TYPE_IOMMU_MEMORY_REGION "iommu-memory-region" > typedef struct IOMMUMemoryRegionClass IOMMUMemoryRegionClass; > DECLARE_OBJ_CHECKERS(IOMMUMemoryRegion, IOMMUMemoryRegionClass, > IOMMU_MEMORY_REGION, TYPE_IOMMU_MEMORY_REGION) > -- > 2.26.2 > >
Re: [PATCH V4 0/8] hw/block/nvme: support namespace attachment
On Tue, Mar 02, 2021 at 10:26:09PM +0900, Minwoo Im wrote: > Hello, > > This series supports namespace attachment: attach and detach. This is > the fourth version of series with replacing changed namespace list to > bitmap to indicate changed namespace IDs. > > Please review. Looks good to me. Reviewed-by: Keith Busch
Re: [PATCH v6 03/11] target/arm: Restrict ARMv4 cpus to TCG accel
On 3/4/21 12:55 PM, Claudio Fontana wrote: > Hi, > > I am trying to take these patches, > in the hope that they help with some of the test issues I am having with the > kvm-only build, > > but they fail with: > > target/arm/Kconfig: does not exist in index > > so I guess I need the "target/arm/Kconfig" series right, how can I find that > one? See the Based-on in the cover ;) https://www.mail-archive.com/qemu-block@nongnu.org/msg79924.html Regards, Phil.
Re: QEMU RBD is slow with QCOW2 images
On Thu, Mar 04, 2021 at 03:59:17PM +0100, Kevin Wolf wrote:
Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben:
On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote:
> Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben:
> > Hi Jason,
> > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD
> > writing data is very slow compared to a raw file.
> >
> > Comparing raw vs QCOW2 image creation with RBD I found that we use a
> > different object size, for the raw file I see '4 MiB objects', for
> > QCOW2 I
> > see '64 KiB objects' as reported on comment 14 [2].
> > This should be the main issue of slowness, indeed forcing in the code 4 MiB
> > object size also for QCOW2 increased the speed a lot.
> >
> > Looking better I discovered that for raw files, we call rbd_create() with
> > obj_order = 0 (if 'cluster_size' options is not defined), so the default
> > object size is used.
> > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size'
> > defined for QCOW2, is 64 KiB.
>
> Hm, the QemuOpts-based image creation is messy, but why does the rbd
> driver even see the cluster_size option?
>
> The first thing qcow2_co_create_opts() does is splitting the passed
> QemuOpts into options it will process on the qcow2 layer and options
> that are passed to the protocol layer. So if you pass a cluster_size
> option, qcow2 should take it for itself and not pass it to rbd.
>
> If it is passed to rbd, I think that's a bug in the qcow2 driver.
IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts,
BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned.
Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find()
properly returns a NULL pointer, but then we call find_default_by_name()
that returns the default value of qcow2 format (64k).
Ugh, I see why. We're passing the protocol driver a QemuOpts that was
created for a QemuOptsList with the qcow2 default, not for its own
QemuOptsList. This is wrong.
Note that the QemuOptsList is not qcow2_create_opts itself, but a list
that is created with qemu_opts_append() to combine qcow2 and rbd options
into a new QemuOptsList. For overlapping options, the format wins.
I don't think you can change the QemuOptsList of an existing QemuOpts,
nor is there a clone operation that could just copy all options into a
new QemuOpts created for the rbd QemuOptsList, so maybe the easiest
hack^Wsolution would be converting to QDict and back...
Do you mean something like this? (I'll send a proper patch when
everything is a little clearer to me :-)
diff --git a/block.c b/block.c
index a1f3cecd75..74b02b32dc 100644
--- a/block.c
+++ b/block.c
@@ -671,13 +671,33 @@ out:
int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
{
BlockDriver *drv;
+QemuOpts *new_opts;
+QDict *qdict;
+int ret;
drv = bdrv_find_protocol(filename, true, errp);
if (drv == NULL) {
return -ENOENT;
}
-return bdrv_create(drv, filename, opts, errp);
+if (!drv->create_opts) {
+error_setg(errp, "Driver '%s' does not support image creation",
+ drv->format_name);
+return -ENOTSUP;
+}
+
+qdict = qemu_opts_to_qdict(opts, NULL);
+new_opts = qemu_opts_from_qdict(drv->create_opts, qdict, errp);
+if (new_opts == NULL) {
+ret = -EINVAL;
+goto out;
+}
+
+ret = bdrv_create(drv, filename, new_opts, errp);
+out:
+qemu_opts_del(new_opts);
+qobject_unref(qdict);
+return ret;
}
> > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster
> > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from
> > QemuOpts calling qemu_opts_to_qdict_filtered().
> > For some reason that I have yet to understand, after this deletion, however
> > remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB),
> > that it's used in qemu_rbd_co_create_opts()
>
> So it seems you came to a similar conclusion. We need to find out where
> the 64k come from and just fix that so that rbd uses its default.
Yes, I tried debugging above, but I'm not sure how to fix it.
Maybe a new parameter in qemu_opt_get_size_helper() to prevent it from
looking for the default value.
Or we should prevent the default value from being added to the
opts->list->desc, but that part is still not very clear to me.
opts->list is already wrong, I think this is what we need to fix.
> > At this point my doubts are:
> > Does it make sense to use the same cluster_size as qcow2 as object_size in
> > RBD?
> > If we want to keep the 2 options separated, how can it be done?
> > Should we
> > rename the option in block/rbd.c?
>
> My lazy answer is that you could just use QMP blockdev-create, where you
> create layer by layer separately.
>
> What could possibly be done for the QemuOpts is using the dotted syntax
> like for opening, so you could specify file.cluster_size=... for the
> pro
Re: [PATCH v2 1/3] fdc: Drop deprecated floppy configuration
On Thu, Mar 04, 2021 at 05:52:50PM +0100, Peter Krempa wrote:
> On Thu, Mar 04, 2021 at 17:23:05 +0100, Markus Armbruster wrote:
> > Daniel P. Berrangé writes:
> >
> > > On Thu, Mar 04, 2021 at 03:26:55PM +0100, Markus Armbruster wrote:
> > >> Daniel P. Berrangé writes:
> > >>
> > >> > On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote:
> > >> >> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate
> > >> >> configuring floppies with -global isa-fdc" (v5.1.0).
> > >> >>
> > >> >> Signed-off-by: Markus Armbruster
> > >> >> ---
> > >> >> docs/system/deprecated.rst | 26 --
> > >> >> docs/system/removed-features.rst | 26 ++
> > >> >> hw/block/fdc.c | 54 +--
> > >> >> tests/qemu-iotests/172 | 31 +-
> > >> >> tests/qemu-iotests/172.out | 562
> > >> >> +--
> > >> >> 5 files changed, 30 insertions(+), 669 deletions(-)
>
> [...]
>
> > >>
> > >> Correct.
> > >>
> > >> This was deprecated in commit 4a27a638e7 "fdc: Deprecate configuring
> > >> floppies with -global isa-fdc" (v5.1.0). Since then, its use triggers a
> > >> warning:
> > >>
> > >> $ qemu-system-x86_64 -nodefaults -M q35 -display none -drive
> > >> if=none,id=drive-fdc0-0-0 -device
> > >> isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1
> > >> qemu-system-x86_64: -device
> > >> isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1: warning: warning: property
> > >> isa-fdc.driveA is deprecated
> > >> Use -device floppy,unit=0,drive=... instead.
> > >>
> > >> Note the -M q35. Needed because the default machine type has an onboard
> > >> isa-fdc, which cannot be configured this way.
> > >>
> > >> Sadly, the commit's update of docs/system/deprecated.rst neglects to
> > >> cover this use. Looks the series overtaxed my capacity to juggle
> > >> details; my apologies.
> > >>
> > >> Is libvirt still using these properties?
> > >
> > > Unfortunately yes, but it seems like it ought to be fairly easy to
> > > change the syntax. Just need to figure out what the right way to
> > > detect the availability of the new syntax is. Presumably just look
> > > for existance of the 'floppy' device type ?
> >
> > Yes. The device type was added in merge commit fd209e4a7, v2.8.0.
> >
> > > Can you confirm that switching from -global to the new -device floppy
> > > does /not/ have any live migration impact ?
> >
> > Yes, it must not affect migration.
> >
> > When Kevin split the floppy device type off the floppy controller, he
> > had to add some moderately ugly hackery to keep the old qdev properties
> > working. Think propagate property values to floppy from controller,
> > which otherwise ignores them.
> >
> > The way you get the values into the floppy device cannot affect the
> > migration data. Only different values can.
> >
> > This patch removes a deprecated way.
>
> Note that when QEMU_CAPS_BLOCKDEV is asserted we format floppies as:
>
> -blockdev '{"driver":"file","filename":"/tmp/firmware.img",\
> "node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
> -blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\
> "file":"libvirt-2-storage"}' \
> -device floppy,unit=0,drive=libvirt-2-format,id=fdc0-0-0 \
> -blockdev '{"driver":"file","filename":"/tmp/data.img",\
> "node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
> -blockdev
> '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2",\
> "file":"libvirt-1-storage"}' \
> -device floppy,unit=1,drive=libvirt-1-format,id=fdc0-0-1 \
>
> as visible in the test file:
>
> tests/qemuxml2argvdata/disk-floppy-q35-2_11.x86_64-latest.args
>
> So libvirt should be in the clear. isa-fdc with driveA/driveB is
> formatted only when the blockdev capability isn't present.
Ahh, excellant, I missed that detailed and worried that we still had
work todo.
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 v2 1/3] fdc: Drop deprecated floppy configuration
On Thu, Mar 04, 2021 at 17:23:05 +0100, Markus Armbruster wrote:
> Daniel P. Berrangé writes:
>
> > On Thu, Mar 04, 2021 at 03:26:55PM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé writes:
> >>
> >> > On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote:
> >> >> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate
> >> >> configuring floppies with -global isa-fdc" (v5.1.0).
> >> >>
> >> >> Signed-off-by: Markus Armbruster
> >> >> ---
> >> >> docs/system/deprecated.rst | 26 --
> >> >> docs/system/removed-features.rst | 26 ++
> >> >> hw/block/fdc.c | 54 +--
> >> >> tests/qemu-iotests/172 | 31 +-
> >> >> tests/qemu-iotests/172.out | 562 +--
> >> >> 5 files changed, 30 insertions(+), 669 deletions(-)
[...]
> >>
> >> Correct.
> >>
> >> This was deprecated in commit 4a27a638e7 "fdc: Deprecate configuring
> >> floppies with -global isa-fdc" (v5.1.0). Since then, its use triggers a
> >> warning:
> >>
> >> $ qemu-system-x86_64 -nodefaults -M q35 -display none -drive
> >> if=none,id=drive-fdc0-0-0 -device
> >> isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1
> >> qemu-system-x86_64: -device
> >> isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1: warning: warning: property
> >> isa-fdc.driveA is deprecated
> >> Use -device floppy,unit=0,drive=... instead.
> >>
> >> Note the -M q35. Needed because the default machine type has an onboard
> >> isa-fdc, which cannot be configured this way.
> >>
> >> Sadly, the commit's update of docs/system/deprecated.rst neglects to
> >> cover this use. Looks the series overtaxed my capacity to juggle
> >> details; my apologies.
> >>
> >> Is libvirt still using these properties?
> >
> > Unfortunately yes, but it seems like it ought to be fairly easy to
> > change the syntax. Just need to figure out what the right way to
> > detect the availability of the new syntax is. Presumably just look
> > for existance of the 'floppy' device type ?
>
> Yes. The device type was added in merge commit fd209e4a7, v2.8.0.
>
> > Can you confirm that switching from -global to the new -device floppy
> > does /not/ have any live migration impact ?
>
> Yes, it must not affect migration.
>
> When Kevin split the floppy device type off the floppy controller, he
> had to add some moderately ugly hackery to keep the old qdev properties
> working. Think propagate property values to floppy from controller,
> which otherwise ignores them.
>
> The way you get the values into the floppy device cannot affect the
> migration data. Only different values can.
>
> This patch removes a deprecated way.
Note that when QEMU_CAPS_BLOCKDEV is asserted we format floppies as:
-blockdev '{"driver":"file","filename":"/tmp/firmware.img",\
"node-name":"libvirt-2-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-2-format","read-only":false,"driver":"raw",\
"file":"libvirt-2-storage"}' \
-device floppy,unit=0,drive=libvirt-2-format,id=fdc0-0-0 \
-blockdev '{"driver":"file","filename":"/tmp/data.img",\
"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}' \
-blockdev '{"node-name":"libvirt-1-format","read-only":false,"driver":"qcow2",\
"file":"libvirt-1-storage"}' \
-device floppy,unit=1,drive=libvirt-1-format,id=fdc0-0-1 \
as visible in the test file:
tests/qemuxml2argvdata/disk-floppy-q35-2_11.x86_64-latest.args
So libvirt should be in the clear. isa-fdc with driveA/driveB is
formatted only when the blockdev capability isn't present.
Re: [PATCH v2 1/3] fdc: Drop deprecated floppy configuration
Daniel P. Berrangé writes: > On Thu, Mar 04, 2021 at 03:26:55PM +0100, Markus Armbruster wrote: >> Daniel P. Berrangé writes: >> >> > On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote: >> >> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate >> >> configuring floppies with -global isa-fdc" (v5.1.0). >> >> >> >> Signed-off-by: Markus Armbruster >> >> --- >> >> docs/system/deprecated.rst | 26 -- >> >> docs/system/removed-features.rst | 26 ++ >> >> hw/block/fdc.c | 54 +-- >> >> tests/qemu-iotests/172 | 31 +- >> >> tests/qemu-iotests/172.out | 562 +-- >> >> 5 files changed, 30 insertions(+), 669 deletions(-) >> >> >> >> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst >> >> index 2fcac7861e..6a22bc07e2 100644 >> >> --- a/docs/system/deprecated.rst >> >> +++ b/docs/system/deprecated.rst >> >> @@ -94,32 +94,6 @@ QEMU 5.1 has three options: >> >>to the user to load all the images they need. >> >> 3. ``-bios `` - Tells QEMU to load the specified file as the >> >> firmwrae. >> >> >> >> -``Configuring floppies with ``-global`` >> >> -''' >> >> - >> >> -Use ``-device floppy,...`` instead: >> >> -:: >> >> - >> >> --global isa-fdc.driveA=... >> >> --global sysbus-fdc.driveA=... >> >> --global SUNW,fdtwo.drive=... >> >> - >> >> -become >> >> -:: >> >> - >> >> --device floppy,unit=0,drive=... >> >> - >> >> -and >> >> -:: >> >> - >> >> --global isa-fdc.driveB=... >> >> --global sysbus-fdc.driveB=... >> >> - >> >> -become >> >> -:: >> >> - >> >> --device floppy,unit=1,drive=... >> >> - >> >> ``-drive`` with bogus interface type >> >> >> >> >> >> diff --git a/docs/system/removed-features.rst >> >> b/docs/system/removed-features.rst >> >> index c8481cafbd..b0e7350408 100644 >> >> --- a/docs/system/removed-features.rst >> >> +++ b/docs/system/removed-features.rst >> >> @@ -38,6 +38,32 @@ or ``-display default,show-cursor=on`` instead. >> >> QEMU 5.0 introduced an alternative syntax to specify the size of the >> >> translation >> >> block cache, ``-accel tcg,tb-size=``. >> >> >> >> +``Configuring floppies with ``-global`` (removed in 6.0) >> >> + >> >> + >> >> +Use ``-device floppy,...`` instead: >> >> +:: >> >> + >> >> +-global isa-fdc.driveA=... >> >> +-global sysbus-fdc.driveA=... >> >> +-global SUNW,fdtwo.drive=... >> > >> > It looks like we're not actually removing the use of -global, rather >> > we're removing the driveA= and driveB= properties entirely, which >> > simply means there's nothing to be set via -global. The distinction >> > is important, because IIUC, it means that libvirt's use of these >> > properties via -device is also impacted eg >> > >> > -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1,driveB=drive-fdc0-0-1 >> > >> > will no longer work too ? >> >> Correct. >> >> This was deprecated in commit 4a27a638e7 "fdc: Deprecate configuring >> floppies with -global isa-fdc" (v5.1.0). Since then, its use triggers a >> warning: >> >> $ qemu-system-x86_64 -nodefaults -M q35 -display none -drive >> if=none,id=drive-fdc0-0-0 -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1 >> qemu-system-x86_64: -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1: >> warning: warning: property isa-fdc.driveA is deprecated >> Use -device floppy,unit=0,drive=... instead. >> >> Note the -M q35. Needed because the default machine type has an onboard >> isa-fdc, which cannot be configured this way. >> >> Sadly, the commit's update of docs/system/deprecated.rst neglects to >> cover this use. Looks the series overtaxed my capacity to juggle >> details; my apologies. >> >> Is libvirt still using these properties? > > Unfortunately yes, but it seems like it ought to be fairly easy to > change the syntax. Just need to figure out what the right way to > detect the availability of the new syntax is. Presumably just look > for existance of the 'floppy' device type ? Yes. The device type was added in merge commit fd209e4a7, v2.8.0. > Can you confirm that switching from -global to the new -device floppy > does /not/ have any live migration impact ? Yes, it must not affect migration. When Kevin split the floppy device type off the floppy controller, he had to add some moderately ugly hackery to keep the old qdev properties working. Think propagate property values to floppy from controller, which otherwise ignores them. The way you get the values into the floppy device cannot affect the migration data. Only different values can. This patch removes a deprecated way.
Re: Potential regression in 'qemu-img convert' to LVM
On 07/01/2021 21:03, Nir Soffer wrote: On Tue, Sep 15, 2020 at 2:51 PM Stefan Reiter wrote: On 9/15/20 11:08 AM, Nir Soffer wrote: On Mon, Sep 14, 2020 at 3:25 PM Stefan Reiter wrote: Hi list, following command fails since 5.1 (tested on kernel 5.4.60): # qemu-img convert -p -f raw -O raw /dev/zvol/pool/disk-1 /dev/vg/disk-1 qemu-img: error while writing at byte 2157968896: Device or resource busy (source is ZFS here, but doesn't matter in practice, it always fails the same; offset changes slightly but consistently hovers around 2^31) strace shows the following: fallocate(13, FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE, 2157968896, 4608) = -1 EBUSY (Device or resource busy) What is the size of the LV? Same as the source, 5GB in my test case. Created with: # lvcreate -ay --size 5242880k --name disk-1 vg Does it happen if you change sparse minimum size (-S)? For example: -S 64k qemu-img convert -p -f raw -O raw -S 64k /dev/zvol/pool/disk-1 /dev/vg/disk-1 Tried a few different values, always the same result: EBUSY at byte 2157968896. Other fallocate calls leading up to this work fine. This happens since commit edafc70c0c "qemu-img convert: Don't pre-zero images", before that all fallocates happened at the start. Reverting the commit and calling qemu-img exactly the same way on the same data works fine. But slowly, doing up to 100% more work for fully allocated images. Of course, I'm not saying the patch is wrong, reverting it just avoids triggering the bug. Simply retrying the syscall on EBUSY (like EINTR) does *not* work, once it fails it keeps failing with the same error. I couldn't find anything related to EBUSY on fallocate, and it only happens on LVM targets... Any idea or pointers where to look? Is this thin LV? No, regular LV. See command above. This works for us using regular LVs. Which kernel? which distro? Reproducible on: * PVE w/ kernel 5.4.60 (Ubuntu based) * Manjaro w/ kernel 5.8.6 I found that it does not happen with all images, I suppose there must be a certain number of smaller holes for it to happen. I am using a VM image with a bare-bones Alpine Linux installation, but it's not an isolated case, we've had two people report the issue on our bug tracker: https://bugzilla.proxmox.com/show_bug.cgi?id=3002 I think that this issue may be fixed by https://lists.nongnu.org/archive/html/qemu-block/2020-11/msg00358.html Nir Sorry for the late reply, but yes, I can confirm this fixes the issue. ~
Re: QEMU RBD is slow with QCOW2 images
Am 04.03.2021 um 15:08 hat Stefano Garzarella geschrieben: > On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote: > > Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben: > > > Hi Jason, > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD > > > writing data is very slow compared to a raw file. > > > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a > > > different object size, for the raw file I see '4 MiB objects', for > > > QCOW2 I > > > see '64 KiB objects' as reported on comment 14 [2]. > > > This should be the main issue of slowness, indeed forcing in the code 4 > > > MiB > > > object size also for QCOW2 increased the speed a lot. > > > > > > Looking better I discovered that for raw files, we call rbd_create() with > > > obj_order = 0 (if 'cluster_size' options is not defined), so the default > > > object size is used. > > > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size' > > > defined for QCOW2, is 64 KiB. > > > > Hm, the QemuOpts-based image creation is messy, but why does the rbd > > driver even see the cluster_size option? > > > > The first thing qcow2_co_create_opts() does is splitting the passed > > QemuOpts into options it will process on the qcow2 layer and options > > that are passed to the protocol layer. So if you pass a cluster_size > > option, qcow2 should take it for itself and not pass it to rbd. > > > > If it is passed to rbd, I think that's a bug in the qcow2 driver. > > IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts, > BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned. > > Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find() > properly returns a NULL pointer, but then we call find_default_by_name() > that returns the default value of qcow2 format (64k). Ugh, I see why. We're passing the protocol driver a QemuOpts that was created for a QemuOptsList with the qcow2 default, not for its own QemuOptsList. This is wrong. Note that the QemuOptsList is not qcow2_create_opts itself, but a list that is created with qemu_opts_append() to combine qcow2 and rbd options into a new QemuOptsList. For overlapping options, the format wins. I don't think you can change the QemuOptsList of an existing QemuOpts, nor is there a clone operation that could just copy all options into a new QemuOpts created for the rbd QemuOptsList, so maybe the easiest hack^Wsolution would be converting to QDict and back... > > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster > > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from > > > QemuOpts calling qemu_opts_to_qdict_filtered(). > > > For some reason that I have yet to understand, after this deletion, > > > however > > > remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 > > > KiB), > > > that it's used in qemu_rbd_co_create_opts() > > > > So it seems you came to a similar conclusion. We need to find out where > > the 64k come from and just fix that so that rbd uses its default. > > Yes, I tried debugging above, but I'm not sure how to fix it. > > Maybe a new parameter in qemu_opt_get_size_helper() to prevent it from > looking for the default value. > Or we should prevent the default value from being added to the > opts->list->desc, but that part is still not very clear to me. opts->list is already wrong, I think this is what we need to fix. > > > At this point my doubts are: > > > Does it make sense to use the same cluster_size as qcow2 as object_size in > > > RBD? > > > If we want to keep the 2 options separated, how can it be done? Should we > > > rename the option in block/rbd.c? > > > > My lazy answer is that you could just use QMP blockdev-create, where you > > create layer by layer separately. > > > > What could possibly be done for the QemuOpts is using the dotted syntax > > like for opening, so you could specify file.cluster_size=... for the > > protocol layer (or data_file.cluster_size=... for the external data > > file etc.) > > This would be cool :-) I'm almost sure that compatibility will make this more complicated than it sounds, but we could have a try. Kevin
Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field
On 3/4/21 5:24 PM, Kevin Wolf wrote:
> Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> We are going to use it in more places, calculating
>> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict
>> *options, int flags,
>> ret = -EFBIG;
>> goto fail;
>> }
>> +s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>>
>> s->bat_size = le32_to_cpu(ph.bat_entries);
>> if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> Checking the context, I saw this a few lines above:
>
> if (s->tracks > INT32_MAX/513) {
>
> Is the 513 intentional?
>
> Kevin
>
I can not remember why I have written this at that time,
but original comment for the commit was
commit d25d59802021a747812472780d80a0e792078f40
Author: Denis V. Lunev
Date: Mon Jul 28 20:23:55 2014 +0400
parallels: 2TB+ parallels images support
Parallels has released in the recent updates of Parallels Server 5/6
new addition to his image format. Images with signature WithouFreSpacExt
have offsets in the catalog coded not as offsets in sectors (multiple
of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
In this case all 64 bits of header->nb_sectors are used for image size.
This patch implements support of this for qemu-img and also adds
specific
check for an incorrect image. Images with block size greater than
INT_MAX/513 are not supported. The biggest available Parallels image
cluster size in the field is 1 Mb. Thus this limit will not hurt
anyone.
Signed-off-by: Denis V. Lunev
CC: Jeff Cody
CC: Kevin Wolf
CC: Stefan Hajnoczi
Reviewed-by: Jeff Cody
Signed-off-by: Stefan Hajnoczi
Thus I believe that this is intentional.
Den
Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field
Am 04.03.2021 um 15:57 hat Denis V. Lunev geschrieben:
> On 3/4/21 5:24 PM, Kevin Wolf wrote:
> > Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> We are going to use it in more places, calculating
> >> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy
> >> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict
> >> *options, int flags,
> >> ret = -EFBIG;
> >> goto fail;
> >> }
> >> +s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
> >>
> >> s->bat_size = le32_to_cpu(ph.bat_entries);
> >> if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
> > Checking the context, I saw this a few lines above:
> >
> > if (s->tracks > INT32_MAX/513) {
> >
> > Is the 513 intentional?
> >
> > Kevin
> >
> I can not remember why I have written this at that time,
> but original comment for the commit was
>
> commit d25d59802021a747812472780d80a0e792078f40
> Author: Denis V. Lunev
> Date: Mon Jul 28 20:23:55 2014 +0400
>
> parallels: 2TB+ parallels images support
>
> Parallels has released in the recent updates of Parallels Server 5/6
> new addition to his image format. Images with signature WithouFreSpacExt
> have offsets in the catalog coded not as offsets in sectors (multiple
> of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
>
> In this case all 64 bits of header->nb_sectors are used for image size.
>
> This patch implements support of this for qemu-img and also adds
> specific
> check for an incorrect image. Images with block size greater than
> INT_MAX/513 are not supported. The biggest available Parallels image
> cluster size in the field is 1 Mb. Thus this limit will not hurt
> anyone.
>
> Signed-off-by: Denis V. Lunev
> CC: Jeff Cody
> CC: Kevin Wolf
> CC: Stefan Hajnoczi
> Reviewed-by: Jeff Cody
> Signed-off-by: Stefan Hajnoczi
>
> Thus I believe that this is intentional.
Hm, fair. It's a weird number. I would have guessed a typo, but if it's
in the commit message as well, it might be intentional. Or just a typo
combined with copy & paste.
If we ever remember or find a new reason why it has to be 513 rather
than 512, adding a comment would be nice.
Kevin
Re: [PATCH] MAINTAINERS: add Vladimir as co-maintainer of NBD
On 3/4/21 4:35 AM, Vladimir Sementsov-Ogievskiy wrote: > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > > I have good knowledge of the subsystem and I'm an author of large part > of it :) Reviewed-by: Eric Blake I'll include this in my next NBD pull request > > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 92ba1fce5e..58994bfafc 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3024,6 +3024,7 @@ F: block/iscsi-opts.c > > Network Block Device (NBD) > M: Eric Blake > +M: Vladimir Sementsov-Ogievskiy > L: qemu-block@nongnu.org > S: Maintained > F: block/nbd* > @@ -3034,6 +3035,7 @@ F: blockdev-nbd.c > F: docs/interop/nbd.txt > F: docs/interop/qemu-nbd.rst > T: git https://repo.or.cz/qemu/ericb.git nbd > +T: git https://src.openvz.org/scm/~vsementsov/qemu.git nbd > > NFS > M: Peter Lieven > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 0/2] QOM type names and QAPI
On 04/03/21 15:02, Markus Armbruster wrote: [*] Paolo's "[PATCH 04/25] keyval: accept escaped commas in implied option" provides for comma-quoting. I'm ignoring it here for brevity. I assure you it doesn't weaken my argument. I even agree about that, for what it's worth. Acked-by: Paolo Bonzini Paolo Markus Armbruster (2): hw: Replace anti-social QOM type names memory: Drop "qemu:" prefix from QOM memory region type names include/exec/memory.h| 4 ++-- include/hw/arm/armv7m.h | 2 +- include/hw/arm/fsl-imx25.h | 2 +- include/hw/arm/fsl-imx31.h | 2 +- include/hw/arm/fsl-imx6.h| 2 +- include/hw/arm/fsl-imx6ul.h | 2 +- include/hw/arm/fsl-imx7.h| 2 +- include/hw/arm/xlnx-zynqmp.h | 2 +- include/hw/cris/etraxfs.h| 2 +- include/hw/i386/ich9.h | 2 +- include/hw/misc/grlib_ahb_apb_pnp.h | 4 ++-- include/hw/misc/zynq-xadc.h | 2 +- include/hw/register.h| 2 +- include/hw/sparc/grlib.h | 6 +++--- hw/arm/xilinx_zynq.c | 2 +- hw/audio/cs4231.c| 2 +- hw/block/fdc.c | 4 ++-- hw/char/etraxfs_ser.c| 2 +- hw/cris/axis_dev88.c | 6 +++--- hw/display/tcx.c | 2 +- hw/intc/etraxfs_pic.c| 2 +- hw/microblaze/xlnx-zynqmp-pmu.c | 2 +- hw/misc/zynq_slcr.c | 2 +- hw/sparc/sun4m.c | 12 ++-- hw/timer/etraxfs_timer.c | 2 +- softmmu/vl.c | 2 +- tests/vmstate-static-checker-data/dump1.json | 4 ++-- tests/vmstate-static-checker-data/dump2.json | 4 ++-- 28 files changed, 42 insertions(+), 42 deletions(-)
Re: [PATCH v2 0/6] parallels: load bitmap extension
Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben: > Hi all! > > We need to load bitmaps from parallels image in our product. > So here is a feature. Thanks, applied to the block branch. I changed some sentences in patch 2 as suggested in my reply to it. Please let me know if you're happy with these changes or if you would like to tweak them a bit more. Kevin
Re: [PATCH v2 1/3] fdc: Drop deprecated floppy configuration
On Thu, Mar 04, 2021 at 03:26:55PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé writes: > > > On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote: > >> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate > >> configuring floppies with -global isa-fdc" (v5.1.0). > >> > >> Signed-off-by: Markus Armbruster > >> --- > >> docs/system/deprecated.rst | 26 -- > >> docs/system/removed-features.rst | 26 ++ > >> hw/block/fdc.c | 54 +-- > >> tests/qemu-iotests/172 | 31 +- > >> tests/qemu-iotests/172.out | 562 +-- > >> 5 files changed, 30 insertions(+), 669 deletions(-) > >> > >> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > >> index 2fcac7861e..6a22bc07e2 100644 > >> --- a/docs/system/deprecated.rst > >> +++ b/docs/system/deprecated.rst > >> @@ -94,32 +94,6 @@ QEMU 5.1 has three options: > >>to the user to load all the images they need. > >> 3. ``-bios `` - Tells QEMU to load the specified file as the > >> firmwrae. > >> > >> -``Configuring floppies with ``-global`` > >> -''' > >> - > >> -Use ``-device floppy,...`` instead: > >> -:: > >> - > >> --global isa-fdc.driveA=... > >> --global sysbus-fdc.driveA=... > >> --global SUNW,fdtwo.drive=... > >> - > >> -become > >> -:: > >> - > >> --device floppy,unit=0,drive=... > >> - > >> -and > >> -:: > >> - > >> --global isa-fdc.driveB=... > >> --global sysbus-fdc.driveB=... > >> - > >> -become > >> -:: > >> - > >> --device floppy,unit=1,drive=... > >> - > >> ``-drive`` with bogus interface type > >> > >> > >> diff --git a/docs/system/removed-features.rst > >> b/docs/system/removed-features.rst > >> index c8481cafbd..b0e7350408 100644 > >> --- a/docs/system/removed-features.rst > >> +++ b/docs/system/removed-features.rst > >> @@ -38,6 +38,32 @@ or ``-display default,show-cursor=on`` instead. > >> QEMU 5.0 introduced an alternative syntax to specify the size of the > >> translation > >> block cache, ``-accel tcg,tb-size=``. > >> > >> +``Configuring floppies with ``-global`` (removed in 6.0) > >> + > >> + > >> +Use ``-device floppy,...`` instead: > >> +:: > >> + > >> +-global isa-fdc.driveA=... > >> +-global sysbus-fdc.driveA=... > >> +-global SUNW,fdtwo.drive=... > > > > It looks like we're not actually removing the use of -global, rather > > we're removing the driveA= and driveB= properties entirely, which > > simply means there's nothing to be set via -global. The distinction > > is important, because IIUC, it means that libvirt's use of these > > properties via -device is also impacted eg > > > > -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1,driveB=drive-fdc0-0-1 > > > > will no longer work too ? > > Correct. > > This was deprecated in commit 4a27a638e7 "fdc: Deprecate configuring > floppies with -global isa-fdc" (v5.1.0). Since then, its use triggers a > warning: > > $ qemu-system-x86_64 -nodefaults -M q35 -display none -drive > if=none,id=drive-fdc0-0-0 -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1 > qemu-system-x86_64: -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1: > warning: warning: property isa-fdc.driveA is deprecated > Use -device floppy,unit=0,drive=... instead. > > Note the -M q35. Needed because the default machine type has an onboard > isa-fdc, which cannot be configured this way. > > Sadly, the commit's update of docs/system/deprecated.rst neglects to > cover this use. Looks the series overtaxed my capacity to juggle > details; my apologies. > > Is libvirt still using these properties? Unfortunately yes, but it seems like it ought to be fairly easy to change the syntax. Just need to figure out what the right way to detect the availability of the new syntax is. Presumably just look for existance of the 'floppy' device type ? Can you confirm that switching from -global to the new -device floppy does /not/ have any live migration impact ? 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 v2 1/3] fdc: Drop deprecated floppy configuration
Daniel P. Berrangé writes: > On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote: >> Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate >> configuring floppies with -global isa-fdc" (v5.1.0). >> >> Signed-off-by: Markus Armbruster >> --- >> docs/system/deprecated.rst | 26 -- >> docs/system/removed-features.rst | 26 ++ >> hw/block/fdc.c | 54 +-- >> tests/qemu-iotests/172 | 31 +- >> tests/qemu-iotests/172.out | 562 +-- >> 5 files changed, 30 insertions(+), 669 deletions(-) >> >> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst >> index 2fcac7861e..6a22bc07e2 100644 >> --- a/docs/system/deprecated.rst >> +++ b/docs/system/deprecated.rst >> @@ -94,32 +94,6 @@ QEMU 5.1 has three options: >>to the user to load all the images they need. >> 3. ``-bios `` - Tells QEMU to load the specified file as the >> firmwrae. >> >> -``Configuring floppies with ``-global`` >> -''' >> - >> -Use ``-device floppy,...`` instead: >> -:: >> - >> --global isa-fdc.driveA=... >> --global sysbus-fdc.driveA=... >> --global SUNW,fdtwo.drive=... >> - >> -become >> -:: >> - >> --device floppy,unit=0,drive=... >> - >> -and >> -:: >> - >> --global isa-fdc.driveB=... >> --global sysbus-fdc.driveB=... >> - >> -become >> -:: >> - >> --device floppy,unit=1,drive=... >> - >> ``-drive`` with bogus interface type >> >> >> diff --git a/docs/system/removed-features.rst >> b/docs/system/removed-features.rst >> index c8481cafbd..b0e7350408 100644 >> --- a/docs/system/removed-features.rst >> +++ b/docs/system/removed-features.rst >> @@ -38,6 +38,32 @@ or ``-display default,show-cursor=on`` instead. >> QEMU 5.0 introduced an alternative syntax to specify the size of the >> translation >> block cache, ``-accel tcg,tb-size=``. >> >> +``Configuring floppies with ``-global`` (removed in 6.0) >> + >> + >> +Use ``-device floppy,...`` instead: >> +:: >> + >> +-global isa-fdc.driveA=... >> +-global sysbus-fdc.driveA=... >> +-global SUNW,fdtwo.drive=... > > It looks like we're not actually removing the use of -global, rather > we're removing the driveA= and driveB= properties entirely, which > simply means there's nothing to be set via -global. The distinction > is important, because IIUC, it means that libvirt's use of these > properties via -device is also impacted eg > > -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1,driveB=drive-fdc0-0-1 > > will no longer work too ? Correct. This was deprecated in commit 4a27a638e7 "fdc: Deprecate configuring floppies with -global isa-fdc" (v5.1.0). Since then, its use triggers a warning: $ qemu-system-x86_64 -nodefaults -M q35 -display none -drive if=none,id=drive-fdc0-0-0 -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1 qemu-system-x86_64: -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1: warning: warning: property isa-fdc.driveA is deprecated Use -device floppy,unit=0,drive=... instead. Note the -M q35. Needed because the default machine type has an onboard isa-fdc, which cannot be configured this way. Sadly, the commit's update of docs/system/deprecated.rst neglects to cover this use. Looks the series overtaxed my capacity to juggle details; my apologies. Is libvirt still using these properties?
Re: [PATCH v2 3/6] block/parallels: BDRVParallelsState: add cluster_size field
Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> We are going to use it in more places, calculating
> "s->tracks << BDRV_SECTOR_BITS" doesn't look good.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> @@ -771,6 +770,7 @@ static int parallels_open(BlockDriverState *bs, QDict
> *options, int flags,
> ret = -EFBIG;
> goto fail;
> }
> +s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
>
> s->bat_size = le32_to_cpu(ph.bat_entries);
> if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
Checking the context, I saw this a few lines above:
if (s->tracks > INT32_MAX/513) {
Is the 513 intentional?
Kevin
Re: [PATCH v2 2/6] parallels.txt: fix bitmap L1 table description
Am 24.02.2021 um 11:47 hat Vladimir Sementsov-Ogievskiy geschrieben: > Actually L1 table entry offset is in 512 bytes sectors. Fix the spec. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > docs/interop/parallels.txt | 27 +++ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/docs/interop/parallels.txt b/docs/interop/parallels.txt > index f15bf35bd1..73af9a2c4b 100644 > --- a/docs/interop/parallels.txt > +++ b/docs/interop/parallels.txt > @@ -208,21 +208,24 @@ of its data area are: >28 - 31:l1_size >The number of entries in the L1 table of the bitmap. > > - variable: l1_table (8 * l1_size bytes) > - L1 offset table (in bytes) > + variable: L1 offset table (l1_table), size: 8 * l1_size bytes > > -A dirty bitmap is stored using a one-level structure for the mapping to host > -clusters - an L1 table. > +Dirty bitmap is stored in the array of clusters inside Parallels Image file. > +Offsets of these clusters are saved in L1 offset table here. Each L1 table > +entry is a 64bit integer described below: I think the English grammar needs some fixes here (missing articles). If I understand correctly, it's also not really an array, which I would understand as consecutive clusters. Maybe something like this: The dirty bitmap described by this feature extension is stored in a set of clusters inside the Parallels image file. The offsets of these clusters are saved in the L1 offset table specified by the feature extension. Each L1 table entry is a 64 bit integer as described below: > -Given an offset in bytes into the bitmap data, the offset in bytes into the > -image file can be obtained as follows: > +Given an offset in bytes into the bitmap data, corresponding L1 entry is > > -offset = l1_table[offset / cluster_size] + (offset % cluster_size) > +l1_table[offset / cluster_size] > > -If an L1 table entry is 0, the corresponding cluster of the bitmap is assumed > -to be zero. > +If L1 table entry is 0, all bits in the corresponding cluster of the bitmap > +are assumed to be 0. "an L1 table", like before. > -If an L1 table entry is 1, the corresponding cluster of the bitmap is assumed > -to have all bits set. > +If L1 table entry is 1, all bits in the corresponding cluster of the bitmap > +are assumed to be 1. Same here. > -If an L1 table entry is not 0 or 1, it allocates a cluster from the data > area. > +If an L1 table entry is not 0 or 1, it contains corresponding cluster offset "the corresponding cluster offset" > +(in 512b sectors). Given an offset in bytes into the bitmap data the offset > in > +bytes into the image file can be obtained as follows: > + > +offset = l1_table[offset / cluster_size] * 512 + (offset % cluster_size) These changes can be made while applying the patch. Kevin
Re: QEMU RBD is slow with QCOW2 images
On Thu, Mar 04, 2021 at 01:05:02PM +0100, Kevin Wolf wrote: Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben: Hi Jason, as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD writing data is very slow compared to a raw file. Comparing raw vs QCOW2 image creation with RBD I found that we use a different object size, for the raw file I see '4 MiB objects', for QCOW2 I see '64 KiB objects' as reported on comment 14 [2]. This should be the main issue of slowness, indeed forcing in the code 4 MiB object size also for QCOW2 increased the speed a lot. Looking better I discovered that for raw files, we call rbd_create() with obj_order = 0 (if 'cluster_size' options is not defined), so the default object size is used. Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size' defined for QCOW2, is 64 KiB. Hm, the QemuOpts-based image creation is messy, but why does the rbd driver even see the cluster_size option? The first thing qcow2_co_create_opts() does is splitting the passed QemuOpts into options it will process on the qcow2 layer and options that are passed to the protocol layer. So if you pass a cluster_size option, qcow2 should take it for itself and not pass it to rbd. If it is passed to rbd, I think that's a bug in the qcow2 driver. IIUC qcow2 properyl remove it, but when rbd uses qemu_opt_get_size_del(opts, BLOCK_OPT_CLUSTER_SIZE, 0) the default value of qcow2 format is returned. Going in depth in qemu_opt_get_size_helper(), I found that qemu_opt_find() properly returns a NULL pointer, but then we call find_default_by_name() that returns the default value of qcow2 format (64k). Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster size, since in qcow2_co_create_opts() we remove the 'cluster_size' from QemuOpts calling qemu_opts_to_qdict_filtered(). For some reason that I have yet to understand, after this deletion, however remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts() So it seems you came to a similar conclusion. We need to find out where the 64k come from and just fix that so that rbd uses its default. Yes, I tried debugging above, but I'm not sure how to fix it. Maybe a new parameter in qemu_opt_get_size_helper() to prevent it from looking for the default value. Or we should prevent the default value from being added to the opts->list->desc, but that part is still not very clear to me. At this point my doubts are: Does it make sense to use the same cluster_size as qcow2 as object_size in RBD? If we want to keep the 2 options separated, how can it be done? Should we rename the option in block/rbd.c? My lazy answer is that you could just use QMP blockdev-create, where you create layer by layer separately. What could possibly be done for the QemuOpts is using the dotted syntax like for opening, so you could specify file.cluster_size=... for the protocol layer (or data_file.cluster_size=... for the external data file etc.) This would be cool :-) Thanks, Stefano
[PATCH v2 1/2] hw: Replace anti-social QOM type names
Several QOM type names contain ',':
ARM,bitband-memory
etraxfs,pic
etraxfs,serial
etraxfs,timer
fsl,imx25
fsl,imx31
fsl,imx6
fsl,imx6ul
fsl,imx7
grlib,ahbpnp
grlib,apbpnp
grlib,apbuart
grlib,gptimer
grlib,irqmp
qemu,register
SUNW,bpp
SUNW,CS4231
SUNW,DBRI
SUNW,DBRI.prom
SUNW,fdtwo
SUNW,sx
SUNW,tcx
xilinx,zynq_slcr
xlnx,zynqmp
xlnx,zynqmp-pmu-soc
xlnx,zynq-xadc
These are all device types. They can't be plugged with -device /
device_add, except for xlnx,zynqmp-pmu-soc, and I doubt that one
actually works.
They *can* be used with -device / device_add to request help.
Usability is poor, though: you have to double the comma, like this:
$ qemu-system-x86_64 -device SUNW,,fdtwo,help
Trap for the unwary. The fact that this was broken in
device-introspect-test for more than six years until commit e27bd49876
fixed it demonstrates that "the unwary" includes seasoned developers.
One QOM type name contains ' ': "ICH9 SMB". Because having to
remember just one way to quote would be too easy.
Rename the "SUNW,FOO types to "sun-FOO". Summarily replace ',' and '
' by '-' in the other type names.
Signed-off-by: Markus Armbruster
---
include/hw/arm/armv7m.h | 2 +-
include/hw/arm/fsl-imx25.h | 2 +-
include/hw/arm/fsl-imx31.h | 2 +-
include/hw/arm/fsl-imx6.h| 2 +-
include/hw/arm/fsl-imx6ul.h | 2 +-
include/hw/arm/fsl-imx7.h| 2 +-
include/hw/arm/xlnx-zynqmp.h | 2 +-
include/hw/cris/etraxfs.h| 2 +-
include/hw/i386/ich9.h | 2 +-
include/hw/misc/grlib_ahb_apb_pnp.h | 4 ++--
include/hw/misc/zynq-xadc.h | 2 +-
include/hw/register.h| 2 +-
include/hw/sparc/grlib.h | 6 +++---
hw/arm/xilinx_zynq.c | 2 +-
hw/audio/cs4231.c| 2 +-
hw/block/fdc.c | 4 ++--
hw/char/etraxfs_ser.c| 2 +-
hw/cris/axis_dev88.c | 6 +++---
hw/display/tcx.c | 2 +-
hw/intc/etraxfs_pic.c| 2 +-
hw/microblaze/xlnx-zynqmp-pmu.c | 2 +-
hw/misc/zynq_slcr.c | 2 +-
hw/sparc/sun4m.c | 12 ++--
hw/timer/etraxfs_timer.c | 2 +-
softmmu/vl.c | 2 +-
tests/vmstate-static-checker-data/dump1.json | 4 ++--
tests/vmstate-static-checker-data/dump2.json | 4 ++--
27 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/include/hw/arm/armv7m.h b/include/hw/arm/armv7m.h
index 0791dcb68a..189b23a8ce 100644
--- a/include/hw/arm/armv7m.h
+++ b/include/hw/arm/armv7m.h
@@ -15,7 +15,7 @@
#include "target/arm/idau.h"
#include "qom/object.h"
-#define TYPE_BITBAND "ARM,bitband-memory"
+#define TYPE_BITBAND "ARM-bitband-memory"
OBJECT_DECLARE_SIMPLE_TYPE(BitBandState, BITBAND)
struct BitBandState {
diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
index c1603b2ac2..1b1086e945 100644
--- a/include/hw/arm/fsl-imx25.h
+++ b/include/hw/arm/fsl-imx25.h
@@ -34,7 +34,7 @@
#include "target/arm/cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX25 "fsl,imx25"
+#define TYPE_FSL_IMX25 "fsl-imx25"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX25State, FSL_IMX25)
#define FSL_IMX25_NUM_UARTS 5
diff --git a/include/hw/arm/fsl-imx31.h b/include/hw/arm/fsl-imx31.h
index b9792d58ae..c116a73e0b 100644
--- a/include/hw/arm/fsl-imx31.h
+++ b/include/hw/arm/fsl-imx31.h
@@ -30,7 +30,7 @@
#include "target/arm/cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX31 "fsl,imx31"
+#define TYPE_FSL_IMX31 "fsl-imx31"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX31State, FSL_IMX31)
#define FSL_IMX31_NUM_UARTS 2
diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 29cc425acc..83291457cf 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -36,7 +36,7 @@
#include "cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX6 "fsl,imx6"
+#define TYPE_FSL_IMX6 "fsl-imx6"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX6State, FSL_IMX6)
#define FSL_IMX6_NUM_CPUS 4
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index f8ebfba4f9..7812e516a5 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -40,7 +40,7 @@
#include "cpu.h"
#include "qom/object.h"
-#define TYPE_FSL_IMX6UL "fsl,imx6ul"
+#define TYPE_FSL_IMX6UL "fsl-imx6ul"
OBJECT_DECLARE_SIMPLE_TYPE(FslIMX6ULState, FSL_IMX6UL)
enum FslIMX6ULConfiguration {
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 161fdc36da..f5d527a490 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -41,7 +41,7 @@
#include "cpu.h"
#include "qom/ob
[PATCH v2 0/2] QOM type names and QAPI
Based-on: <20210304100059.157158-1-arm...@redhat.com> Note: to apply it on master instead, replace one more "SUNW,fdtwo" by "sun-fdtwo" in hw/block/fdc.c. QAPI has naming rules. docs/devel/qapi-code-gen.txt: === Naming rules and reserved names === All names must begin with a letter, and contain only ASCII letters, digits, hyphen, and underscore. There are two exceptions: enum values may start with a digit, and names that are downstream extensions (see section Downstream extensions) start with underscore. [More on reserved names, upper vs. lower case, '-' vs. '_'...] The generator enforces the rules. Naming rules help in at least three ways: 1. They help with keeping names in interfaces consistent and predictable. 2. They make avoiding collisions with the users' names in the generator simpler. 3. They enable quote-less, evolvable syntax. For instance, keyval_parse() syntax consists of names, values, and special characters ',', '=', '.' Since names cannot contain special characters, there is no need for quoting[*]. Simple. Values are unrestricted, but only ',' is special there. We quote it by doubling. Together, we get exactly the same quoting as in QemuOpts. This is a feature. If we ever decice to extend key syntax, we have plenty of special characters to choose from. This is also a feature. Both features rely on naming rules. QOM has no naming rules whatsoever. Actual names aren't nearly as bad as they could be. Still, there are plenty of "funny" names. This may become a problem when we * Switch from QemuOpts to keyval_parse() Compared to QemuOpts, keyval_parse() restricts *keys*, but not *values*. "Funny" type names occuring as values are no worse than before: quoting issues, described below. Type names occuring in keys must be valid QAPI names. Should be avoidable. * QAPIfy (the compile-time static parts of) QOM QOM type names become QAPI enum values. They must conform to QAPI enum naming rules. Let's review the existing offenders in the qemu-system-FOO: 1. We have a few type names containing ',', and one containing ' '. The former require QemuOpts / keyval quoting (double the comma), the latter requires shell quoting. There is no excuse for making our users remember and do such crap. PATCH 1 eliminates it. 2. We have six type names containing '+', and two containing ':': Sun-UltraSparc-IIIi+-sparc64-cpu Sun-UltraSparc-IV+-sparc64-cpu power5+_v2.1-powerpc64-cpu power5+_v2.1-spapr-cpu-core power7+_v2.1-powerpc64-cpu power7+_v2.1-spapr-cpu-core qemu:iommu-memory-region qemu:memory-region Naming rules could be relaxed to accept '+' and ':'. I'm doubt this is worthwhile. PATCH 2 renames the ones with ':'. I'm leaving the ones with '+' alone for now. 3. We have some 550 type names containing '.'. QAPI's (enum) naming rules could be relaxed to accept '.'. keyval_parse()'s can't. Irrelevant, as long as type names only occur as values, not as keys. 4. We have some 450 names starting with a digit. Roughly half of them also contain '.'. Leading digit is okay as QAPI enum, not okay as keyval_parse() key fragment. Irrelevant, as long as type names only occur as *values*, not as *keys*. 5. We generate type names of the form T::I, where T is a type name, and I is the name of one of its interfaces. I hope these are just for internal use. One more thing on relaxing QAPI naming rules. QAPI names get mapped to (parts of) C identifiers. These mappings are not injective. The basic mapping is simple: replace characters other than letters and digits by '_'. This means names distinct QAPI names can clash in C. Fairly harmless when the only "other" characters are '-' and '_'. The more "others" we permit, the more likely confusing clashes become. Not a show stopper, "merely" an issue of ergonomics. v2: * No longer RFC * Cover letter: - Consider difference between between keys and values in keyval_parse() - Differentiate more clearly between general QAPI naming rules and QAPI enum naming rules - List the types containing '+' - Cover types containing ':' - Drop "Can we get rid of '.'?" [Peter Maydell] - Drop the idea to rename types starting with digits - Cover "T::I" types generated for interfaces - Cover ergonomics of relaxing QAPI naming rules * PATCH 1: Rename SUNW,FOO to sun-FOO [Mark Cave-Ayland] * PATCH 2: New [*] Paolo's "[PATCH 04/25] keyval: accept escaped commas in implied option" provides for comma-quoting. I'm ignoring it here for brevity. I assure you it doesn't weaken my argument. Markus Armbruster (2): hw: Replace anti-social QOM type names memory: Drop "qemu:" prefix from QOM memory region type names include/exec/memory.h| 4 ++-- include/hw/arm/armv7m.h | 2 +- include/hw/arm/fsl-imx25.h | 2 +-
[PATCH v2 2/2] memory: Drop "qemu:" prefix from QOM memory region type names
Almost all QOM type names consist only of letters, digits, '-', '_', and '.'. Just two contain ':': "qemu:memory-region" and "qemu:iommu-memory-region". Neither can be plugged with -object. Rename them to "memory-region" and "iommu-memory-region". Signed-off-by: Markus Armbruster --- include/exec/memory.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index c6fb714e49..3c95d7831a 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -33,11 +33,11 @@ #define MAX_PHYS_ADDR_SPACE_BITS 62 #define MAX_PHYS_ADDR(((hwaddr)1 << MAX_PHYS_ADDR_SPACE_BITS) - 1) -#define TYPE_MEMORY_REGION "qemu:memory-region" +#define TYPE_MEMORY_REGION "memory-region" DECLARE_INSTANCE_CHECKER(MemoryRegion, MEMORY_REGION, TYPE_MEMORY_REGION) -#define TYPE_IOMMU_MEMORY_REGION "qemu:iommu-memory-region" +#define TYPE_IOMMU_MEMORY_REGION "iommu-memory-region" typedef struct IOMMUMemoryRegionClass IOMMUMemoryRegionClass; DECLARE_OBJ_CHECKERS(IOMMUMemoryRegion, IOMMUMemoryRegionClass, IOMMU_MEMORY_REGION, TYPE_IOMMU_MEMORY_REGION) -- 2.26.2
Re: [PATCH] nbd: server: Report holes for raw images
Am 25.02.2021 um 19:50 hat Vladimir Sementsov-Ogievskiy geschrieben: > 19.02.2021 19:58, Eric Blake wrote: > > On 2/19/21 10:42 AM, Eric Blake wrote: > > > > > > To me, data=false looks compatible with NBD_STATE_HOLE. From user point > > > > of view, getting same results from qemu-nbd and qemu-img is more > > > > important than being more correct about allocation status. > > > > > > More to the point, here is our inconsistency: > > > > > > In nbd/server.c, we turn !BDRV_BLOCK_ALLOCATED into NBD_STATE_HOLE > > > > > > In block/nbd.c, we turn !NBD_STATE_HOLE into BDRV_BLOCK_DATA > > > > > > The fact that we are not doing a round-trip conversion means that one of > > > the two places is wrong. And your argument that the server side is > > > wrong makes sense to me. > > > > In fact, when I went back and researched when this was introduced (see > > commit e7b1948d51 in 2018), we may have been aware of the inconsistency > > between client and server, but didn't make up our minds at the time: > > https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03465.html > > "? Hm, don't remember, what we decided about DATA/HOLE flags mapping.." > > > > > > > > I'll wait a few days for any other reviewer commentary before taking > > > this through my NBD tree. > > > > > > > > I can add the following. > > First, link to my research of block_status in Qemu: > https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg05136.html > > And about HOLE and ZERO.. > > As I've noted in the research above, SCSI may return HOLE & !ZERO: > > from SCSI: Logical Block Provisioning Read Zeros (LBPRZ) bit 1 If > the logical block provisioning read zeros (LBPRZ) bit is set to one, > then, for an unmapped LBA specified by a read operation, the > deviceserver shall send user data with all bits set to zero to the > data-in buffer. 0 If the TPRZ bit is set to zero, then, for an > unmapped LBA specified by a read operation, the device server may send > user data with all bitsset to any value to the data-in buffer. > > So we can have an unmapped area that can be read as any random data. > Same thing can be said about null-co driver with read-zeroes=false > > Also, qcow2 support ALLOCATED ZERO clusters which reads as zero but > data is allocated - they are reasonable to report as ZERO & !HOLE > > And of-course UNALLOCATED ZERO clusters in qcow2 and lseek-holes are > reasonable to report as ZERO & HOLE, because they reads as zero and > "future writes to that area may cause fragmentation or encounter an > NBD_ENOSPC".. > > So, all combination are reasonable, we just need to fix Qemu NBD > server to report correct statuses in all these cases. > > It seems that ZERO/HOLE specification is a lot more reasonable than > what we have with ZERO/DATA/ALLOCATED in Qemu, and may be true way is > move internal block_status to use NBD terms. Is there not a 1:1 correspondence between our internal flags and the NBD ones? ZERO is exactly the same, and HOLE is the inversion of DATA. ALLOCATED is important internally when finding the node in a backing file chain that actually defines the content, but for a user it doesn't make a difference. This is why it isn't exposed in NBD. So I think both QEMU and NBD use the flags that make sense in the respective context. Kevin
Re: QEMU RBD is slow with QCOW2 images
Am 03.03.2021 um 18:40 hat Stefano Garzarella geschrieben: > Hi Jason, > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD > writing data is very slow compared to a raw file. > > Comparing raw vs QCOW2 image creation with RBD I found that we use a > different object size, for the raw file I see '4 MiB objects', for QCOW2 I > see '64 KiB objects' as reported on comment 14 [2]. > This should be the main issue of slowness, indeed forcing in the code 4 MiB > object size also for QCOW2 increased the speed a lot. > > Looking better I discovered that for raw files, we call rbd_create() with > obj_order = 0 (if 'cluster_size' options is not defined), so the default > object size is used. > Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size' > defined for QCOW2, is 64 KiB. Hm, the QemuOpts-based image creation is messy, but why does the rbd driver even see the cluster_size option? The first thing qcow2_co_create_opts() does is splitting the passed QemuOpts into options it will process on the qcow2 layer and options that are passed to the protocol layer. So if you pass a cluster_size option, qcow2 should take it for itself and not pass it to rbd. If it is passed to rbd, I think that's a bug in the qcow2 driver. > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from > QemuOpts calling qemu_opts_to_qdict_filtered(). > For some reason that I have yet to understand, after this deletion, however > remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB), > that it's used in qemu_rbd_co_create_opts() So it seems you came to a similar conclusion. We need to find out where the 64k come from and just fix that so that rbd uses its default. > At this point my doubts are: > Does it make sense to use the same cluster_size as qcow2 as object_size in > RBD? > If we want to keep the 2 options separated, how can it be done? Should we > rename the option in block/rbd.c? My lazy answer is that you could just use QMP blockdev-create, where you create layer by layer separately. What could possibly be done for the QemuOpts is using the dotted syntax like for opening, so you could specify file.cluster_size=... for the protocol layer (or data_file.cluster_size=... for the external data file etc.) Kevin
Re: [PATCH v6 03/11] target/arm: Restrict ARMv4 cpus to TCG accel
Hi, I am trying to take these patches, in the hope that they help with some of the test issues I am having with the kvm-only build, but they fail with: target/arm/Kconfig: does not exist in index so I guess I need the "target/arm/Kconfig" series right, how can I find that one? Thanks, Claudio On 1/31/21 12:50 PM, Philippe Mathieu-Daudé wrote: > KVM requires the target cpu to be at least ARMv8 architecture > (support on ARMv7 has been dropped in commit 82bf7ae84ce: > "target/arm: Remove KVM support for 32-bit Arm hosts"). > > Only enable the following ARMv4 CPUs when TCG is available: > > - StrongARM (SA1100/1110) > - OMAP1510 (TI925T) > > The following machines are no more built when TCG is disabled: > > - cheetah Palm Tungsten|E aka. Cheetah PDA (OMAP310) > - sx1 Siemens SX1 (OMAP310) V2 > - sx1-v1 Siemens SX1 (OMAP310) V1 > > Signed-off-by: Philippe Mathieu-Daudé > --- > default-configs/devices/arm-softmmu.mak | 2 -- > hw/arm/Kconfig | 4 > target/arm/Kconfig | 4 > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/default-configs/devices/arm-softmmu.mak > b/default-configs/devices/arm-softmmu.mak > index 0824e9be795..6ae964c14fd 100644 > --- a/default-configs/devices/arm-softmmu.mak > +++ b/default-configs/devices/arm-softmmu.mak > @@ -14,8 +14,6 @@ CONFIG_INTEGRATOR=y > CONFIG_FSL_IMX31=y > CONFIG_MUSICPAL=y > CONFIG_MUSCA=y > -CONFIG_CHEETAH=y > -CONFIG_SX1=y > CONFIG_NSERIES=y > CONFIG_STELLARIS=y > CONFIG_REALVIEW=y > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index f3ecb73a3d8..f2957b33bee 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -31,6 +31,8 @@ config ARM_VIRT > > config CHEETAH > bool > +default y if TCG && ARM > +select ARM_V4 > select OMAP > select TSC210X > > @@ -249,6 +251,8 @@ config COLLIE > > config SX1 > bool > +default y if TCG && ARM > +select ARM_V4 > select OMAP > > config VERSATILE > diff --git a/target/arm/Kconfig b/target/arm/Kconfig > index ae89d05c7e5..811e1e81652 100644 > --- a/target/arm/Kconfig > +++ b/target/arm/Kconfig > @@ -6,6 +6,10 @@ config AARCH64 > bool > select ARM > > +config ARM_V4 > +bool > +depends on TCG && ARM > + > config ARM_V7M > bool > select PTIMER >
Re: [PATCH 8/9] hw/arm/virt: Restrict 32-bit CPUs to TCG
On Fri, 5 Feb 2021 at 14:44, Philippe Mathieu-Daudé wrote:
>
> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> ("target/arm: Remove KVM support for 32-bit Arm hosts").
> Restrict the 32-bit CPUs to --enable-tcg builds.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/arm/virt.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f5e4a6ec914..ab6300650f9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -197,8 +197,10 @@ static const int a15irqmap[] = {
> };
>
> static const char *valid_cpus[] = {
> +#ifdef CONFIG_TCG
> ARM_CPU_TYPE_NAME("cortex-a7"),
> ARM_CPU_TYPE_NAME("cortex-a15"),
> +#endif /* CONFIG_TCG */
> #ifdef TARGET_AARCH64
> ARM_CPU_TYPE_NAME("cortex-a53"),
> ARM_CPU_TYPE_NAME("cortex-a57"),
How painful would it be to just have it check whether the
CPU type is present in the executable, rather than hard-coding an ifdef ?
I think that if you try to run the virt board with command line
arguments that (implicitly or explicitly) mean you've asked for
a CPU which isn't present in the QEMU executable, it should give
an error rather than silently selecting something else.
thanks
-- PMM
Re: [PATCH 8/9] hw/arm/virt: Restrict 32-bit CPUs to TCG
Hi Peter,
what do you think of the following patch? We messaged yesterday about
cortex-a15 being the default cpu for virt,
this patch would need also changing the default CPU for virt under KVM I would
think.
Or, we could change the virt default cpu to "max"?
Thanks,
Claudio
On 2/5/21 4:19 PM, Andrew Jones wrote:
> On Fri, Feb 05, 2021 at 03:43:44PM +0100, Philippe Mathieu-Daudé wrote:
>> Support for ARMv7 has been dropped in commit 82bf7ae84ce
>> ("target/arm: Remove KVM support for 32-bit Arm hosts").
>> Restrict the 32-bit CPUs to --enable-tcg builds.
>>
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> hw/arm/virt.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f5e4a6ec914..ab6300650f9 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -197,8 +197,10 @@ static const int a15irqmap[] = {
>> };
>>
>> static const char *valid_cpus[] = {
>> +#ifdef CONFIG_TCG
>> ARM_CPU_TYPE_NAME("cortex-a7"),
>> ARM_CPU_TYPE_NAME("cortex-a15"),
>> +#endif /* CONFIG_TCG */
>> #ifdef TARGET_AARCH64
>> ARM_CPU_TYPE_NAME("cortex-a53"),
>> ARM_CPU_TYPE_NAME("cortex-a57"),
>> --
>> 2.26.2
>>
>
> So this filters the cpus out of KVM only builds, which seems
> reasonable to do. Of course, if the build is for both KVM and
> TCG, then the cpus won't be filtered out and we'll have to rely
> on the runtime checks to error out if one where to try a 32-bit
> cpu with KVM. But that's fine too, so
>
> Reviewed-by: Andrew Jones
>
> Thanks,
> drew
>
>
Re: QEMU RBD is slow with QCOW2 images
On Thu, Mar 04, 2021 at 12:12:51PM +0100, Stefano Garzarella wrote: > On Thu, Mar 04, 2021 at 10:25:33AM +, Daniel P. Berrangé wrote: > > On Thu, Mar 04, 2021 at 09:55:40AM +0100, Stefano Garzarella wrote: > > > On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote: > > > > On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella > > > > wrote: > > > > > > > > > > Hi Jason, > > > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD > > > > > writing data is very slow compared to a raw file. > > > > > > > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a > > > > > different object size, for the raw file I see '4 MiB objects', for > > > > > QCOW2 > > > > > I see '64 KiB objects' as reported on comment 14 [2]. > > > > > This should be the main issue of slowness, indeed forcing in the code > > > > > 4 > > > > > MiB object size also for QCOW2 increased the speed a lot. > > > > > > > > > > Looking better I discovered that for raw files, we call rbd_create() > > > > > with obj_order = 0 (if 'cluster_size' options is not defined), so the > > > > > default object size is used. > > > > > Instead for QCOW2, we use obj_order = 16, since the default > > > > > 'cluster_size' defined for QCOW2, is 64 KiB. > > > > > > > > > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 > > > > > cluster > > > > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' > > > > > from > > > > > QemuOpts calling qemu_opts_to_qdict_filtered(). > > > > > For some reason that I have yet to understand, after this deletion, > > > > > however remains in QemuOpts the default value of 'cluster_size' for > > > > > qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts() > > > > > > > > > > At this point my doubts are: > > > > > Does it make sense to use the same cluster_size as qcow2 as > > > > > object_size > > > > > in RBD? > > > > > > > > No, not really. But it also doesn't really make any sense to put a > > > > QCOW2 image within an RBD image. To clarify from the BZ, OpenStack > > > > does not put QCOW2 images on RBD, it converts QCOW2 images into raw > > > > images to store in RBD. > > > > > > Yes, that was my doubt, thanks for the confirmation. > > > > > > Also Daniel (+CC) confirmed me the same thing, but just to be complete he > > > added that there is a case where OpenStack could use qcow2 on RBD, but in > > > this case using in-kernel RBD, so the QEMU RBD is not involved. > > > > > > > > > > > > If we want to keep the 2 options separated, how can it be done? Should > > > > > we rename the option in block/rbd.c? > > > > > > > > You can already pass overrides to the RBD block driver by just > > > > appending them after the > > > > "rbd:[:option1=value1[:option2=value2]]" portion, perhaps > > > > that could be re-used. > > > > > > I see, we should extend qemu_rbd_parse_filename() to suppurt it. > > > > We shouldn't really be extending the legacy filename syntax. > > If we need extra options we want them in the QAPI schema for > > blockdev. > > Got it. > > I'm still a bit confused about how QemuOpts are handled between format and > protocol drivers. > > It seems that in this case the protocol tries to access some information > from the format (BLOCK_OPT_CLUSTER_SIZE). > > Since the format removes this information from the QemuOpts passed to the > protocol, this takes the default value of the format, even if a different > value is specified. > > Is it correct for a protocol to access BLOCK_OPT_CLUSTER_SIZE? In a -blockdev world, the caller would be expected to set the values explicitly at all layers that need it. You're talking about a scenario that is non-blockdev though, and I'm not sure what the right answer is here. Will need Kevin/Max to answer that one. 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 0/9] hw/arm/virt: Improve CPU help and fix testing under KVM
On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote: > Yet again bugfixes and cleanup patches noticed while > rebasing my "Support disabling TCG on ARM (part 2)" series. > > Sending them independently as they aren't directly dependent > of it so don't have to be delayed by other unanswered questions. > > Please review, > > Phil. > > Philippe Mathieu-Daudé (9): > tests/qtest/arm-cpu-features: Remove Cortex-A15 check > tests/qtest: Restrict xlnx-can-test to TCG builds > tests/qtest/boot-serial-test: Test Virt machine with 'max' > tests/qtest/cdrom-test: Only allow the Virt machine under KVM > hw/arm/virt: Improve CPU name in help message > hw/arm/virt: Display list of valid CPUs for the Virt machine > hw/arm/virt: Do not include 64-bit CPUs in 32-bit build > hw/arm/virt: Restrict 32-bit CPUs to TCG > tests/qtest/arm-cpu-features: Restrict TCG-only tests > > hw/arm/virt.c | 20 +- > tests/qtest/arm-cpu-features.c | 37 ++ > tests/qtest/boot-serial-test.c | 2 +- > tests/qtest/cdrom-test.c | 5 - > tests/qtest/meson.build| 2 +- > 5 files changed, 54 insertions(+), 12 deletions(-) > Hi Philippe, Markus, I encountered an issue where device-introspect-test.c gets me a qemu-system-aarch64 launched as: ./qemu-system-aarch64 -qtest ... -display none -nodefaults -machine none -accel qtest and results in an attempt to create a device "ast2400-a1", which tries to create a arm926-arm-cpu, which fails because it is a "TCG" cpu, that is not built in my kvm-only build. Any ideas why this happens? Thanks, Claudio
Re: QEMU RBD is slow with QCOW2 images
On Thu, Mar 04, 2021 at 10:25:33AM +, Daniel P. Berrangé wrote: On Thu, Mar 04, 2021 at 09:55:40AM +0100, Stefano Garzarella wrote: On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote: > On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella wrote: > > > > Hi Jason, > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD > > writing data is very slow compared to a raw file. > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a > > different object size, for the raw file I see '4 MiB objects', for QCOW2 > > I see '64 KiB objects' as reported on comment 14 [2]. > > This should be the main issue of slowness, indeed forcing in the code 4 > > MiB object size also for QCOW2 increased the speed a lot. > > > > Looking better I discovered that for raw files, we call rbd_create() > > with obj_order = 0 (if 'cluster_size' options is not defined), so the > > default object size is used. > > Instead for QCOW2, we use obj_order = 16, since the default > > 'cluster_size' defined for QCOW2, is 64 KiB. > > > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from > > QemuOpts calling qemu_opts_to_qdict_filtered(). > > For some reason that I have yet to understand, after this deletion, > > however remains in QemuOpts the default value of 'cluster_size' for > > qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts() > > > > At this point my doubts are: > > Does it make sense to use the same cluster_size as qcow2 as object_size > > in RBD? > > No, not really. But it also doesn't really make any sense to put a > QCOW2 image within an RBD image. To clarify from the BZ, OpenStack > does not put QCOW2 images on RBD, it converts QCOW2 images into raw > images to store in RBD. Yes, that was my doubt, thanks for the confirmation. Also Daniel (+CC) confirmed me the same thing, but just to be complete he added that there is a case where OpenStack could use qcow2 on RBD, but in this case using in-kernel RBD, so the QEMU RBD is not involved. > > > If we want to keep the 2 options separated, how can it be done? Should > > we rename the option in block/rbd.c? > > You can already pass overrides to the RBD block driver by just > appending them after the > "rbd:[:option1=value1[:option2=value2]]" portion, perhaps > that could be re-used. I see, we should extend qemu_rbd_parse_filename() to suppurt it. We shouldn't really be extending the legacy filename syntax. If we need extra options we want them in the QAPI schema for blockdev. Got it. I'm still a bit confused about how QemuOpts are handled between format and protocol drivers. It seems that in this case the protocol tries to access some information from the format (BLOCK_OPT_CLUSTER_SIZE). Since the format removes this information from the QemuOpts passed to the protocol, this takes the default value of the format, even if a different value is specified. Is it correct for a protocol to access BLOCK_OPT_CLUSTER_SIZE? Maybe if we don't want to support this configuration, we should print some warning messages. Note these are separate layers in QEMU block layer. qcow2 is a format driver, while RBD is a protocol driver. QEMU lets any format driver be run on top of any protocol driver in general. In practice there are certain combinations that are more sane and commonly used than others, but QEMU doesn't document this or assign support level to pairing right now. Thanks for the clarification, Stefano
Re: [PATCH v2 1/3] fdc: Drop deprecated floppy configuration
On Thu, Mar 04, 2021 at 11:00:57AM +0100, Markus Armbruster wrote: > Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate > configuring floppies with -global isa-fdc" (v5.1.0). > > Signed-off-by: Markus Armbruster > --- > docs/system/deprecated.rst | 26 -- > docs/system/removed-features.rst | 26 ++ > hw/block/fdc.c | 54 +-- > tests/qemu-iotests/172 | 31 +- > tests/qemu-iotests/172.out | 562 +-- > 5 files changed, 30 insertions(+), 669 deletions(-) > > diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst > index 2fcac7861e..6a22bc07e2 100644 > --- a/docs/system/deprecated.rst > +++ b/docs/system/deprecated.rst > @@ -94,32 +94,6 @@ QEMU 5.1 has three options: >to the user to load all the images they need. > 3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae. > > -``Configuring floppies with ``-global`` > -''' > - > -Use ``-device floppy,...`` instead: > -:: > - > --global isa-fdc.driveA=... > --global sysbus-fdc.driveA=... > --global SUNW,fdtwo.drive=... > - > -become > -:: > - > --device floppy,unit=0,drive=... > - > -and > -:: > - > --global isa-fdc.driveB=... > --global sysbus-fdc.driveB=... > - > -become > -:: > - > --device floppy,unit=1,drive=... > - > ``-drive`` with bogus interface type > > > diff --git a/docs/system/removed-features.rst > b/docs/system/removed-features.rst > index c8481cafbd..b0e7350408 100644 > --- a/docs/system/removed-features.rst > +++ b/docs/system/removed-features.rst > @@ -38,6 +38,32 @@ or ``-display default,show-cursor=on`` instead. > QEMU 5.0 introduced an alternative syntax to specify the size of the > translation > block cache, ``-accel tcg,tb-size=``. > > +``Configuring floppies with ``-global`` (removed in 6.0) > + > + > +Use ``-device floppy,...`` instead: > +:: > + > +-global isa-fdc.driveA=... > +-global sysbus-fdc.driveA=... > +-global SUNW,fdtwo.drive=... It looks like we're not actually removing the use of -global, rather we're removing the driveA= and driveB= properties entirely, which simply means there's nothing to be set via -global. The distinction is important, because IIUC, it means that libvirt's use of these properties via -device is also impacted eg -device isa-fdc,driveA=drive-fdc0-0-0,bootindexA=1,driveB=drive-fdc0-0-1 will no longer work too ? 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 :|
[PATCH v2 9/8] MAINTAINERS: update Benchmark util: add git tree
Signed-off-by: Vladimir Sementsov-Ogievskiy --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9b2aa18e1f..642c1c8a46 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2484,6 +2484,7 @@ Benchmark util M: Vladimir Sementsov-Ogievskiy S: Maintained F: scripts/simplebench/ +T: git https://src.openvz.org/scm/~vsementsov/qemu.git simplebench QAPI M: Markus Armbruster -- 2.29.2
[PATCH] MAINTAINERS: add Vladimir as co-maintainer of NBD
Signed-off-by: Vladimir Sementsov-Ogievskiy --- I have good knowledge of the subsystem and I'm an author of large part of it :) MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 92ba1fce5e..58994bfafc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3024,6 +3024,7 @@ F: block/iscsi-opts.c Network Block Device (NBD) M: Eric Blake +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Maintained F: block/nbd* @@ -3034,6 +3035,7 @@ F: blockdev-nbd.c F: docs/interop/nbd.txt F: docs/interop/qemu-nbd.rst T: git https://repo.or.cz/qemu/ericb.git nbd +T: git https://src.openvz.org/scm/~vsementsov/qemu.git nbd NFS M: Peter Lieven -- 2.29.2
[PATCH v2 7/8] simplebench/bench-backup: add --count and --no-initial-run
Add arguments to set number of test runs per table cell and to disable
initial run that is not counted in results.
It's convenient to set --count 1 --no-initial-run to fast run test
onece, and to set --count to some large enough number for good
precision of the results.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench-backup.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/scripts/simplebench/bench-backup.py
b/scripts/simplebench/bench-backup.py
index a2120fcbf0..519a985a7f 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -155,7 +155,9 @@ def bench(args):
'qemu-binary': path
})
-result = simplebench.bench(bench_func, test_envs, test_cases, count=3)
+result = simplebench.bench(bench_func, test_envs, test_cases,
+ count=args.count,
+ initial_run = not args.no_initial_run)
with open('results.json', 'w') as f:
json.dump(result, f, indent=4)
print(results_to_text(result))
@@ -211,4 +213,10 @@ def __call__(self, parser, namespace, values,
option_string=None):
both: generate two test cases for each src:dst pair''',
default='direct', choices=('direct', 'cached', 'both'))
+p.add_argument('--count', type=int, default=3, help='''\
+Number of test runs per table cell''')
+
+p.add_argument('--no-initial-run', action='store_true', help='''\
+Don't do initial run of test for each cell which doesn't count''')
+
bench(p.parse_args())
--
2.29.2
Re: QEMU RBD is slow with QCOW2 images
On Thu, Mar 04, 2021 at 09:55:40AM +0100, Stefano Garzarella wrote: > On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote: > > On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella > > wrote: > > > > > > Hi Jason, > > > as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD > > > writing data is very slow compared to a raw file. > > > > > > Comparing raw vs QCOW2 image creation with RBD I found that we use a > > > different object size, for the raw file I see '4 MiB objects', for QCOW2 > > > I see '64 KiB objects' as reported on comment 14 [2]. > > > This should be the main issue of slowness, indeed forcing in the code 4 > > > MiB object size also for QCOW2 increased the speed a lot. > > > > > > Looking better I discovered that for raw files, we call rbd_create() > > > with obj_order = 0 (if 'cluster_size' options is not defined), so the > > > default object size is used. > > > Instead for QCOW2, we use obj_order = 16, since the default > > > 'cluster_size' defined for QCOW2, is 64 KiB. > > > > > > Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster > > > size, since in qcow2_co_create_opts() we remove the 'cluster_size' from > > > QemuOpts calling qemu_opts_to_qdict_filtered(). > > > For some reason that I have yet to understand, after this deletion, > > > however remains in QemuOpts the default value of 'cluster_size' for > > > qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts() > > > > > > At this point my doubts are: > > > Does it make sense to use the same cluster_size as qcow2 as object_size > > > in RBD? > > > > No, not really. But it also doesn't really make any sense to put a > > QCOW2 image within an RBD image. To clarify from the BZ, OpenStack > > does not put QCOW2 images on RBD, it converts QCOW2 images into raw > > images to store in RBD. > > Yes, that was my doubt, thanks for the confirmation. > > Also Daniel (+CC) confirmed me the same thing, but just to be complete he > added that there is a case where OpenStack could use qcow2 on RBD, but in > this case using in-kernel RBD, so the QEMU RBD is not involved. > > > > > > If we want to keep the 2 options separated, how can it be done? Should > > > we rename the option in block/rbd.c? > > > > You can already pass overrides to the RBD block driver by just > > appending them after the > > "rbd:[:option1=value1[:option2=value2]]" portion, perhaps > > that could be re-used. > > I see, we should extend qemu_rbd_parse_filename() to suppurt it. We shouldn't really be extending the legacy filename syntax. If we need extra options we want them in the QAPI schema for blockdev. > Maybe if we don't want to support this configuration, we should print some > warning messages. Note these are separate layers in QEMU block layer. qcow2 is a format driver, while RBD is a protocol driver. QEMU lets any format driver be run on top of any protocol driver in general. In practice there are certain combinations that are more sane and commonly used than others, but QEMU doesn't document this or assign support level to pairing right now. 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 v2 7/6] MAINTAINERS: update parallels block driver
On 3/4/21 12:58 PM, Vladimir Sementsov-Ogievskiy wrote: > 04.03.2021 12:51, Vladimir Sementsov-Ogievskiy wrote: >> Add new parallels-ext.c and myself as co-maintainer. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> MAINTAINERS | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 9b2aa18e1f..92ba1fce5e 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3117,9 +3117,11 @@ F: block/dmg.c >> parallels >> M: Stefan Hajnoczi >> M: Denis V. Lunev >> +M: Vladimir Sementsov-Ogievskiy >> L: qemu-block@nongnu.org >> S: Supported >> F: block/parallels.c >> +F: block/parallels-ext.c >> F: docs/interop/parallels.txt >> qed >> > > > squash-in: > > diff --git a/MAINTAINERS b/MAINTAINERS > index 92ba1fce5e..6d6480e1b0 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3123,6 +3123,7 @@ S: Supported > F: block/parallels.c > F: block/parallels-ext.c > F: docs/interop/parallels.txt > +T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels > > qed > M: Stefan Hajnoczi > > > Reviewed-by: Denis V. Lunev
[PATCH v2 4/8] simplebench/bench-backup: add target-cache argument
Allow benchmark with different kinds of target cache.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench-backup.py| 33 --
scripts/simplebench/bench_block_job.py | 10 +---
2 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/scripts/simplebench/bench-backup.py
b/scripts/simplebench/bench-backup.py
index 72eae85bb1..fbc85f266f 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -65,13 +65,26 @@ def bench(args):
test_cases.append({'id': t, 'source': source, 'target': nbd_drv})
continue
-fname = dirs[dst] + '/test-target'
-if args.compressed:
-fname += '.qcow2'
-target = drv_file(fname)
-if args.compressed:
-target = drv_qcow2(target)
-test_cases.append({'id': t, 'source': source, 'target': target})
+if args.target_cache == 'both':
+target_caches = ['direct', 'cached']
+else:
+target_caches = [args.target_cache]
+
+for c in target_caches:
+o_direct = c == 'direct'
+fname = dirs[dst] + '/test-target'
+if args.compressed:
+fname += '.qcow2'
+target = drv_file(fname, o_direct=o_direct)
+if args.compressed:
+target = drv_qcow2(target)
+
+test_id = t
+if args.target_cache == 'both':
+test_id += f'({c})'
+
+test_cases.append({'id': test_id, 'source': source,
+ 'target': target})
binaries = [] # list of (, , [])
for i, q in enumerate(args.env):
@@ -186,5 +199,11 @@ def __call__(self, parser, namespace, values,
option_string=None):
Use compressed backup. It automatically means
automatically creating qcow2 target with
lazy_refcounts for each test run''', action='store_true')
+p.add_argument('--target-cache', help='''\
+Setup cache for target nodes. Options:
+ direct: default, use O_DIRECT and aio=native
+ cached: use system cache (Qemu default) and aio=threads (Qemu default)
+ both: generate two test cases for each src:dst pair''',
+ default='direct', choices=('direct', 'cached', 'both'))
bench(p.parse_args())
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 08f86ed9c1..8f8385ccce 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -115,9 +115,13 @@ def bench_block_copy(qemu_binary, cmd, cmd_options,
source, target):
'-blockdev', json.dumps(target)])
-def drv_file(filename):
-return {'driver': 'file', 'filename': filename,
-'cache': {'direct': True}, 'aio': 'native'}
+def drv_file(filename, o_direct=True):
+node = {'driver': 'file', 'filename': filename}
+if o_direct:
+node['cache'] = {'direct': True}
+node['aio'] = 'native'
+
+return node
def drv_nbd(host, port):
--
2.29.2
[PATCH v2 8/8] simplebench/bench_block_job: drop caches before test run
It probably may improve reliability of results when testing in cached
mode.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench_block_job.py | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 4f03c12169..fa45ad2655 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -53,6 +53,8 @@ def bench_block_job(cmd, cmd_args, qemu_args):
return {'error': 'qemu failed: ' + str(vm.get_log())}
try:
+subprocess.run('sync; echo 3 > /proc/sys/vm/drop_caches', shell=True,
+ check=True)
res = vm.qmp(cmd, **cmd_args)
if res != {'return': {}}:
vm.shutdown()
--
2.29.2
[PATCH v2 6/8] simplebench/bench-backup: support qcow2 source files
Add support for qcow2 source. New option says to use test-source.qcow2
instead of test-source. Of course, test-source.qcow2 should be
precreated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench-backup.py| 5 +
scripts/simplebench/bench_block_job.py | 7 ++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/scripts/simplebench/bench-backup.py
b/scripts/simplebench/bench-backup.py
index fbc85f266f..a2120fcbf0 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -58,6 +58,8 @@ def bench(args):
if src == 'nbd':
source = nbd_drv
+elif args.qcow2_sources:
+source = drv_qcow2(drv_file(dirs[src] + '/test-source.qcow2'))
else:
source = drv_file(dirs[src] + '/test-source')
@@ -199,6 +201,9 @@ def __call__(self, parser, namespace, values,
option_string=None):
Use compressed backup. It automatically means
automatically creating qcow2 target with
lazy_refcounts for each test run''', action='store_true')
+p.add_argument('--qcow2-sources', help='''\
+Use test-source.qcow2 images as sources instead of
+test-source raw images''', action='store_true')
p.add_argument('--target-cache', help='''\
Setup cache for target nodes. Options:
direct: default, use O_DIRECT and aio=native
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 71d2e489c8..4f03c12169 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -88,6 +88,11 @@ def get_image_size(path):
return json.loads(out)['virtual-size']
+def get_blockdev_size(obj):
+img = obj['filename'] if 'filename' in obj else obj['file']['filename']
+return get_image_size(img)
+
+
# Bench backup or mirror
def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
"""Helper to run bench_block_job() for mirror or backup"""
@@ -101,7 +106,7 @@ def bench_block_copy(qemu_binary, cmd, cmd_options, source,
target):
subprocess.run(['qemu-img', 'create', '-f', 'qcow2',
target['file']['filename'],
-str(get_image_size(source['filename']))],
+str(get_blockdev_size(source))],
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL, check=True)
--
2.29.2
[PATCH v2 5/8] simplebench/bench_block_job: handle error in BLOCK_JOB_COMPLETED
We should not report success if there is an error in final event.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench_block_job.py | 4
1 file changed, 4 insertions(+)
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 8f8385ccce..71d2e489c8 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -70,6 +70,10 @@ def bench_block_job(cmd, cmd_args, qemu_args):
vm.shutdown()
return {'error': 'block-job failed: ' + str(e),
'vm-log': vm.get_log()}
+if 'error' in e['data']:
+vm.shutdown()
+return {'error': 'block-job failed: ' + e['data']['error'],
+'vm-log': vm.get_log()}
end_ms = e['timestamp']['seconds'] * 100 + \
e['timestamp']['microseconds']
finally:
--
2.29.2
[PATCH v2 1/8] simplebench: bench_one(): add slow_limit argument
Sometimes one of cells in a testing table runs too slow. And we really
don't want to wait so long. Limit number of runs in this case.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/simplebench.py | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/scripts/simplebench/simplebench.py
b/scripts/simplebench/simplebench.py
index f61513af90..b153cae274 100644
--- a/scripts/simplebench/simplebench.py
+++ b/scripts/simplebench/simplebench.py
@@ -19,9 +19,11 @@
#
import statistics
+import time
-def bench_one(test_func, test_env, test_case, count=5, initial_run=True):
+def bench_one(test_func, test_env, test_case, count=5, initial_run=True,
+ slow_limit=100):
"""Benchmark one test-case
test_func -- benchmarking function with prototype
@@ -36,6 +38,8 @@ def bench_one(test_func, test_env, test_case, count=5,
initial_run=True):
test_case -- test case - opaque second argument for test_func
count -- how many times to call test_func, to calculate average
initial_run -- do initial run of test_func, which don't get into result
+slow_limit -- reduce test runs to 2, if current run exceedes the limit
+ (in seconds)
Returns dict with the following fields:
'runs': list of test_func results
@@ -47,17 +51,34 @@ def bench_one(test_func, test_env, test_case, count=5,
initial_run=True):
'n-failed': number of failed runs (exists only if at least one run
failed)
"""
+runs = []
+i = 0
if initial_run:
+t = time.time()
+
print(' #initial run:')
-print(' ', test_func(test_env, test_case))
+res = test_func(test_env, test_case)
+print(' ', res)
+
+if time.time() - t > slow_limit:
+print('- initial run is too slow, so it counts')
+runs.append(res)
+i = 1
+
+for i in range(i, count):
+t = time.time()
-runs = []
-for i in range(count):
print(' #run {}'.format(i+1))
res = test_func(test_env, test_case)
print(' ', res)
runs.append(res)
+if time.time() - t > slow_limit and len(runs) >= 2:
+print('- run is too slow, and we have enough runs, stop here')
+break
+
+count = len(runs)
+
result = {'runs': runs}
succeeded = [r for r in runs if ('seconds' in r or 'iops' in r)]
--
2.29.2
[PATCH v2 3/8] simplebench/bench-backup: add --compressed option
Allow bench compressed backup.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
scripts/simplebench/bench-backup.py| 55 ++
scripts/simplebench/bench_block_job.py | 23 +++
2 files changed, 62 insertions(+), 16 deletions(-)
diff --git a/scripts/simplebench/bench-backup.py
b/scripts/simplebench/bench-backup.py
index 33a1ecfefa..72eae85bb1 100755
--- a/scripts/simplebench/bench-backup.py
+++ b/scripts/simplebench/bench-backup.py
@@ -23,7 +23,7 @@
import simplebench
from results_to_text import results_to_text
-from bench_block_job import bench_block_copy, drv_file, drv_nbd
+from bench_block_job import bench_block_copy, drv_file, drv_nbd, drv_qcow2
def bench_func(env, case):
@@ -37,29 +37,41 @@ def bench_func(env, case):
def bench(args):
test_cases = []
-sources = {}
-targets = {}
-for d in args.dir:
-label, path = d.split(':') # paths with colon not supported
-sources[label] = drv_file(path + '/test-source')
-targets[label] = drv_file(path + '/test-target')
+# paths with colon not supported, so we just split by ':'
+dirs = dict(d.split(':') for d in args.dir)
+nbd_drv = None
if args.nbd:
nbd = args.nbd.split(':')
host = nbd[0]
port = '10809' if len(nbd) == 1 else nbd[1]
-drv = drv_nbd(host, port)
-sources['nbd'] = drv
-targets['nbd'] = drv
+nbd_drv = drv_nbd(host, port)
for t in args.test:
src, dst = t.split(':')
-test_cases.append({
-'id': t,
-'source': sources[src],
-'target': targets[dst]
-})
+if src == 'nbd' and dst == 'nbd':
+raise ValueError("Can't use 'nbd' label for both src and dst")
+
+if (src == 'nbd' or dst == 'nbd') and not nbd_drv:
+raise ValueError("'nbd' label used but --nbd is not given")
+
+if src == 'nbd':
+source = nbd_drv
+else:
+source = drv_file(dirs[src] + '/test-source')
+
+if dst == 'nbd':
+test_cases.append({'id': t, 'source': source, 'target': nbd_drv})
+continue
+
+fname = dirs[dst] + '/test-target'
+if args.compressed:
+fname += '.qcow2'
+target = drv_file(fname)
+if args.compressed:
+target = drv_qcow2(target)
+test_cases.append({'id': t, 'source': source, 'target': target})
binaries = [] # list of (, , [])
for i, q in enumerate(args.env):
@@ -106,6 +118,13 @@ def bench(args):
elif opt.startswith('max-workers='):
x_perf['max-workers'] = int(opt.split('=')[1])
+backup_options = {}
+if x_perf:
+backup_options['x-perf'] = x_perf
+
+if args.compressed:
+backup_options['compress'] = True
+
if is_mirror:
assert not x_perf
test_envs.append({
@@ -117,7 +136,7 @@ def bench(args):
test_envs.append({
'id': f'backup({label})\n' + '\n'.join(opts),
'cmd': 'blockdev-backup',
-'cmd-options': {'x-perf': x_perf} if x_perf else {},
+'cmd-options': backup_options,
'qemu-binary': path
})
@@ -163,5 +182,9 @@ def __call__(self, parser, namespace, values,
option_string=None):
p.add_argument('--test', nargs='+', help='''\
Tests, in form source-dir-label:target-dir-label''',
action=ExtendAction)
+p.add_argument('--compressed', help='''\
+Use compressed backup. It automatically means
+automatically creating qcow2 target with
+lazy_refcounts for each test run''', action='store_true')
bench(p.parse_args())
diff --git a/scripts/simplebench/bench_block_job.py
b/scripts/simplebench/bench_block_job.py
index 7332845c1c..08f86ed9c1 100755
--- a/scripts/simplebench/bench_block_job.py
+++ b/scripts/simplebench/bench_block_job.py
@@ -21,6 +21,7 @@
import sys
import os
+import subprocess
import socket
import json
@@ -77,11 +78,29 @@ def bench_block_job(cmd, cmd_args, qemu_args):
return {'seconds': (end_ms - start_ms) / 100.0}
+def get_image_size(path):
+out = subprocess.run(['qemu-img', 'info', '--out=json', path],
+ stdout=subprocess.PIPE, check=True).stdout
+return json.loads(out)['virtual-size']
+
+
# Bench backup or mirror
def bench_block_copy(qemu_binary, cmd, cmd_options, source, target):
"""Helper to run bench_block_job() for mirror or backup"""
assert cmd in ('blockdev-backup', 'blockdev-mirror')
+if target['driver'] == 'qcow2':
+try:
+os.remove(target['file']['filename'])
+except OSError:
+pass
+
+subprocess.run(['qemu-img', 'create', '-f', 'qcow2',
+target['file']['filename'],
+str(get_image_size(source['filename']))],
+ stdout=subprocess.
[PATCH v2 0/8] simplebench improvements
Hi all! Here are some improvements to simplebench lib, to support my "qcow2: compressed write cache" series. v1 was inside "[PATCH 0/7] qcow2: compressed write cache" <20210129165030.640169-1-vsement...@virtuozzo.com> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg07795.html https://patchew.org/QEMU/20210129165030.640169-1-vsement...@virtuozzo.com/ v2: split out to be a separate series, which I can finally pull by myself. 01: fix s/50/slow_limit 05-08: new Vladimir Sementsov-Ogievskiy (8): simplebench: bench_one(): add slow_limit argument simplebench: bench_one(): support count=1 simplebench/bench-backup: add --compressed option simplebench/bench-backup: add target-cache argument simplebench/bench_block_job: handle error in BLOCK_JOB_COMPLETED simplebench/bench-backup: support qcow2 source files simplebench/bench-backup: add --count and --no-initial-run simplebench/bench_block_job: drop caches before test run scripts/simplebench/bench-backup.py| 89 +- scripts/simplebench/bench_block_job.py | 44 - scripts/simplebench/simplebench.py | 34 -- 3 files changed, 142 insertions(+), 25 deletions(-) -- 2.29.2
[PATCH v2 2/8] simplebench: bench_one(): support count=1
statistics.stdev raises if sequence length is less than two. Support that case by hand. Signed-off-by: Vladimir Sementsov-Ogievskiy --- scripts/simplebench/simplebench.py | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/simplebench/simplebench.py b/scripts/simplebench/simplebench.py index b153cae274..712e1f845b 100644 --- a/scripts/simplebench/simplebench.py +++ b/scripts/simplebench/simplebench.py @@ -92,7 +92,10 @@ def bench_one(test_func, test_env, test_case, count=5, initial_run=True, dim = 'seconds' result['dimension'] = dim result['average'] = statistics.mean(r[dim] for r in succeeded) -result['stdev'] = statistics.stdev(r[dim] for r in succeeded) +if len(succeeded) == 1: +result['stdev'] = 0 +else: +result['stdev'] = statistics.stdev(r[dim] for r in succeeded) if len(succeeded) < count: result['n-failed'] = count - len(succeeded) -- 2.29.2
[PATCH v2 1/3] fdc: Drop deprecated floppy configuration
Drop the crap deprecated in commit 4a27a638e7 "fdc: Deprecate
configuring floppies with -global isa-fdc" (v5.1.0).
Signed-off-by: Markus Armbruster
---
docs/system/deprecated.rst | 26 --
docs/system/removed-features.rst | 26 ++
hw/block/fdc.c | 54 +--
tests/qemu-iotests/172 | 31 +-
tests/qemu-iotests/172.out | 562 +--
5 files changed, 30 insertions(+), 669 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 2fcac7861e..6a22bc07e2 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -94,32 +94,6 @@ QEMU 5.1 has three options:
to the user to load all the images they need.
3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae.
-``Configuring floppies with ``-global``
-'''
-
-Use ``-device floppy,...`` instead:
-::
-
--global isa-fdc.driveA=...
--global sysbus-fdc.driveA=...
--global SUNW,fdtwo.drive=...
-
-become
-::
-
--device floppy,unit=0,drive=...
-
-and
-::
-
--global isa-fdc.driveB=...
--global sysbus-fdc.driveB=...
-
-become
-::
-
--device floppy,unit=1,drive=...
-
``-drive`` with bogus interface type
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index c8481cafbd..b0e7350408 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -38,6 +38,32 @@ or ``-display default,show-cursor=on`` instead.
QEMU 5.0 introduced an alternative syntax to specify the size of the
translation
block cache, ``-accel tcg,tb-size=``.
+``Configuring floppies with ``-global`` (removed in 6.0)
+
+
+Use ``-device floppy,...`` instead:
+::
+
+-global isa-fdc.driveA=...
+-global sysbus-fdc.driveA=...
+-global SUNW,fdtwo.drive=...
+
+become
+::
+
+-device floppy,unit=0,drive=...
+
+and
+::
+
+-global isa-fdc.driveB=...
+-global sysbus-fdc.driveB=...
+
+become
+::
+
+-device floppy,unit=1,drive=...
+
QEMU Machine Protocol (QMP) commands
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 198940e737..f978ddf647 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -870,7 +870,6 @@ struct FDCtrl {
uint8_t num_floppies;
FDrive drives[MAX_FD];
struct {
-BlockBackend *blk;
FloppyDriveType type;
} qdev_for_drives[MAX_FD];
int reset_sensei;
@@ -2517,56 +2516,12 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl,
DeviceState *fdc_dev,
{
unsigned int i;
FDrive *drive;
-DeviceState *dev;
-BlockBackend *blk;
-bool ok;
-const char *fdc_name, *drive_suffix;
for (i = 0; i < MAX_FD; i++) {
drive = &fdctrl->drives[i];
drive->fdctrl = fdctrl;
-
-/* If the drive is not present, we skip creating the qdev device, but
- * still have to initialise the controller. */
-blk = fdctrl->qdev_for_drives[i].blk;
-if (!blk) {
-fd_init(drive);
-fd_revalidate(drive);
-continue;
-}
-
-fdc_name = object_get_typename(OBJECT(fdc_dev));
-drive_suffix = !strcmp(fdc_name, "SUNW,fdtwo") ? "" : i ? "B" : "A";
-warn_report("warning: property %s.drive%s is deprecated",
-fdc_name, drive_suffix);
-error_printf("Use -device floppy,unit=%d,drive=... instead.\n", i);
-
-dev = qdev_new("floppy");
-qdev_prop_set_uint32(dev, "unit", i);
-qdev_prop_set_enum(dev, "drive-type", fdctrl->qdev_for_drives[i].type);
-
-/*
- * Hack alert: we move the backend from the floppy controller
- * device to the floppy device. We first need to detach the
- * controller, or else floppy_create()'s qdev_prop_set_drive()
- * will die when it attaches floppy device. We also need to
- * take another reference so that blk_detach_dev() doesn't
- * free blk while we still need it.
- *
- * The hack is probably a bad idea.
- */
-blk_ref(blk);
-blk_detach_dev(blk, fdc_dev);
-fdctrl->qdev_for_drives[i].blk = NULL;
-ok = qdev_prop_set_drive_err(dev, "drive", blk, errp);
-blk_unref(blk);
-if (!ok) {
-return;
-}
-
-if (!qdev_realize_and_unref(dev, &fdctrl->bus.bus, errp)) {
-return;
-}
+fd_init(drive);
+fd_revalidate(drive);
}
}
@@ -2882,8 +2837,6 @@ static Property isa_fdc_properties[] = {
DEFINE_PROP_UINT32("iobase", FDCtrlISABus, iobase, 0x3f0),
DEFINE_PROP_UINT32("irq", FDCtrlISABus, irq, 6),
DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
-DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
-DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_drives[1].blk),
DEFI
[PATCH v2 3/3] blockdev: Drop deprecated bogus -drive interface type
Drop the crap deprecated in commit a1b40bda08 "blockdev: Deprecate
-drive with bogus interface type" (v5.1.0).
Signed-off-by: Markus Armbruster
---
docs/system/deprecated.rst | 7 --
docs/system/removed-features.rst | 7 ++
include/sysemu/blockdev.h| 1 -
blockdev.c | 37 +---
softmmu/vl.c | 8 +--
5 files changed, 23 insertions(+), 37 deletions(-)
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 6a22bc07e2..01c58f8e88 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -94,13 +94,6 @@ QEMU 5.1 has three options:
to the user to load all the images they need.
3. ``-bios `` - Tells QEMU to load the specified file as the firmwrae.
-``-drive`` with bogus interface type
-
-
-Drives with interface types other than ``if=none`` are for onboard
-devices. It is possible to use drives the board doesn't pick up with
--device. This usage is now deprecated. Use ``if=none`` instead.
-
Short-form boolean options (since 6.0)
''
diff --git a/docs/system/removed-features.rst b/docs/system/removed-features.rst
index b0e7350408..a2b3a8a1ca 100644
--- a/docs/system/removed-features.rst
+++ b/docs/system/removed-features.rst
@@ -64,6 +64,13 @@ become
-device floppy,unit=1,drive=...
+``-drive`` with bogus interface type (removed in 6.0)
+'
+
+Drives with interface types other than ``if=none`` are for onboard
+devices. Drives the board doesn't pick up can no longer be used with
+-device. Use ``if=none`` instead.
+
QEMU Machine Protocol (QMP) commands
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3b5fcda08d..32c2d6023c 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -35,7 +35,6 @@ struct DriveInfo {
bool is_default;/* Added by default_drive() ? */
int media_cd;
QemuOpts *opts;
-bool claimed_by_board;
QTAILQ_ENTRY(DriveInfo) next;
};
diff --git a/blockdev.c b/blockdev.c
index cd438e60e3..2e01889cff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -240,19 +240,10 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus,
int unit)
return NULL;
}
-void drive_mark_claimed_by_board(void)
-{
-BlockBackend *blk;
-DriveInfo *dinfo;
-
-for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
-dinfo = blk_legacy_dinfo(blk);
-if (dinfo && blk_get_attached_dev(blk)) {
-dinfo->claimed_by_board = true;
-}
-}
-}
-
+/*
+ * Check board claimed all -drive that are meant to be claimed.
+ * Fatal error if any remain unclaimed.
+ */
void drive_check_orphaned(void)
{
BlockBackend *blk;
@@ -262,7 +253,17 @@ void drive_check_orphaned(void)
for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
dinfo = blk_legacy_dinfo(blk);
-if (dinfo->is_default || dinfo->type == IF_NONE) {
+/*
+ * Ignore default drives, because we create certain default
+ * drives unconditionally, then leave them unclaimed. Not the
+ * users fault.
+ * Ignore IF_VIRTIO, because it gets desugared into -device,
+ * so we can leave failing to -device.
+ * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
+ * available for device_add is a feature.
+ */
+if (dinfo->is_default || dinfo->type == IF_VIRTIO
+|| dinfo->type == IF_NONE) {
continue;
}
if (!blk_get_attached_dev(blk)) {
@@ -273,14 +274,6 @@ void drive_check_orphaned(void)
if_name[dinfo->type], dinfo->bus, dinfo->unit);
loc_pop(&loc);
orphans = true;
-continue;
-}
-if (!dinfo->claimed_by_board && dinfo->type != IF_VIRTIO) {
-loc_push_none(&loc);
-qemu_opts_loc_restore(dinfo->opts);
-warn_report("bogus if=%s is deprecated, use if=none",
-if_name[dinfo->type]);
-loc_pop(&loc);
}
}
diff --git a/softmmu/vl.c b/softmmu/vl.c
index b219ce1f35..8097a7b5fb 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2461,13 +2461,7 @@ static void qemu_init_board(void)
/* From here on we enter MACHINE_PHASE_INITIALIZED. */
machine_run_board_init(current_machine);
-/*
- * TODO To drop support for deprecated bogus if=..., move
- * drive_check_orphaned() here, replacing this call. Also drop
- * its deprecation warning, along with DriveInfo member
- * @claimed_by_board.
- */
-drive_mark_claimed_by_board();
+drive_check_orphaned();
realtime_init();
--
2.26.2
[PATCH v2 0/3] Drop deprecated floppy config & bogus -drive if=T
v2: * Rebased, straightforward conflict with commit f5d33dd51f "hw/block/fdc: Remove the check_media_rate property" resolved * PATCH 2: Commit message fixed [Kevin] Markus Armbruster (3): fdc: Drop deprecated floppy configuration fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common() blockdev: Drop deprecated bogus -drive interface type docs/system/deprecated.rst | 33 -- docs/system/removed-features.rst | 33 ++ include/sysemu/blockdev.h| 1 - blockdev.c | 37 +- hw/block/fdc.c | 73 +--- softmmu/vl.c | 8 +- tests/qemu-iotests/172 | 31 +- tests/qemu-iotests/172.out | 562 +-- 8 files changed, 59 insertions(+), 719 deletions(-) -- 2.26.2
[PATCH v2 2/3] fdc: Inline fdctrl_connect_drives() into fdctrl_realize_common()
The previous commit rendered the name fdctrl_connect_drives() somewhat
misleading. Get rid of it by inlining the (now pretty simple)
function into its only caller.
Signed-off-by: Markus Armbruster
---
hw/block/fdc.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f978ddf647..32701c2bc5 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2511,20 +2511,6 @@ void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds)
fdctrl_init_drives(&ISA_FDC(fdc)->state.bus, fds);
}
-static void fdctrl_connect_drives(FDCtrl *fdctrl, DeviceState *fdc_dev,
- Error **errp)
-{
-unsigned int i;
-FDrive *drive;
-
-for (i = 0; i < MAX_FD; i++) {
-drive = &fdctrl->drives[i];
-drive->fdctrl = fdctrl;
-fd_init(drive);
-fd_revalidate(drive);
-}
-}
-
void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
hwaddr mmio_base, DriveInfo **fds)
{
@@ -2565,6 +2551,7 @@ static void fdctrl_realize_common(DeviceState *dev,
FDCtrl *fdctrl,
Error **errp)
{
int i, j;
+FDrive *drive;
static int command_tables_inited = 0;
if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
@@ -2604,7 +2591,13 @@ static void fdctrl_realize_common(DeviceState *dev,
FDCtrl *fdctrl,
}
floppy_bus_create(fdctrl, &fdctrl->bus, dev);
-fdctrl_connect_drives(fdctrl, dev, errp);
+
+for (i = 0; i < MAX_FD; i++) {
+drive = &fdctrl->drives[i];
+drive->fdctrl = fdctrl;
+fd_init(drive);
+fd_revalidate(drive);
+}
}
static const MemoryRegionPortio fdc_portio_list[] = {
--
2.26.2
Re: [PATCH v2 7/6] MAINTAINERS: update parallels block driver
04.03.2021 12:51, Vladimir Sementsov-Ogievskiy wrote: Add new parallels-ext.c and myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9b2aa18e1f..92ba1fce5e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3117,9 +3117,11 @@ F: block/dmg.c parallels M: Stefan Hajnoczi M: Denis V. Lunev +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Supported F: block/parallels.c +F: block/parallels-ext.c F: docs/interop/parallels.txt qed squash-in: diff --git a/MAINTAINERS b/MAINTAINERS index 92ba1fce5e..6d6480e1b0 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3123,6 +3123,7 @@ S: Supported F: block/parallels.c F: block/parallels-ext.c F: docs/interop/parallels.txt +T: git https://src.openvz.org/scm/~vsementsov/qemu.git parallels qed M: Stefan Hajnoczi -- Best regards, Vladimir
Re: [PATCH v2 7/6] MAINTAINERS: update parallels block driver
On 3/4/21 12:51 PM, Vladimir Sementsov-Ogievskiy wrote: > Add new parallels-ext.c and myself as co-maintainer. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > MAINTAINERS | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9b2aa18e1f..92ba1fce5e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3117,9 +3117,11 @@ F: block/dmg.c > parallels > M: Stefan Hajnoczi > M: Denis V. Lunev > +M: Vladimir Sementsov-Ogievskiy > L: qemu-block@nongnu.org > S: Supported > F: block/parallels.c > +F: block/parallels-ext.c > F: docs/interop/parallels.txt > > qed Reviewed-by: Denis V. Lunev
[PATCH v2 7/6] MAINTAINERS: update parallels block driver
Add new parallels-ext.c and myself as co-maintainer. Signed-off-by: Vladimir Sementsov-Ogievskiy --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9b2aa18e1f..92ba1fce5e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3117,9 +3117,11 @@ F: block/dmg.c parallels M: Stefan Hajnoczi M: Denis V. Lunev +M: Vladimir Sementsov-Ogievskiy L: qemu-block@nongnu.org S: Supported F: block/parallels.c +F: block/parallels-ext.c F: docs/interop/parallels.txt qed -- 2.29.2
Re: QEMU RBD is slow with QCOW2 images
On Wed, Mar 03, 2021 at 10:26:12PM +0100, Peter Lieven wrote: Am 03.03.21 um 19:47 schrieb Jason Dillaman: On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella wrote: Hi Jason, as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD writing data is very slow compared to a raw file. Comparing raw vs QCOW2 image creation with RBD I found that we use a different object size, for the raw file I see '4 MiB objects', for QCOW2 I see '64 KiB objects' as reported on comment 14 [2]. This should be the main issue of slowness, indeed forcing in the code 4 MiB object size also for QCOW2 increased the speed a lot. Looking better I discovered that for raw files, we call rbd_create() with obj_order = 0 (if 'cluster_size' options is not defined), so the default object size is used. Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size' defined for QCOW2, is 64 KiB. Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster size, since in qcow2_co_create_opts() we remove the 'cluster_size' from QemuOpts calling qemu_opts_to_qdict_filtered(). For some reason that I have yet to understand, after this deletion, however remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts() At this point my doubts are: Does it make sense to use the same cluster_size as qcow2 as object_size in RBD? No, not really. But it also doesn't really make any sense to put a QCOW2 image within an RBD image. To clarify from the BZ, OpenStack does not put QCOW2 images on RBD, it converts QCOW2 images into raw images to store in RBD. As discussed earlier the only reasonable format for rbd image is raw. What is the idea behind putting a qcow2 on an rbd pool? Jason and I even discussed shortly durign the review of the rbd driver rewrite I posted earlier if it was ok to drop support for writing past the end of file. Anyway the reason why it is so slow is that write requests serialize if the qcow2 file grows. If there is a sane reason why we need qcow2 on rbd we need to implement at least preallocation mode = full to overcome the serialization. Agree, at most we could deprecate it by printing a message and then remove the support in future releases. Thanks, Stefano
Re: QEMU RBD is slow with QCOW2 images
On Wed, Mar 03, 2021 at 01:47:06PM -0500, Jason Dillaman wrote: On Wed, Mar 3, 2021 at 12:41 PM Stefano Garzarella wrote: Hi Jason, as reported in this BZ [1], when qemu-img creates a QCOW2 image on RBD writing data is very slow compared to a raw file. Comparing raw vs QCOW2 image creation with RBD I found that we use a different object size, for the raw file I see '4 MiB objects', for QCOW2 I see '64 KiB objects' as reported on comment 14 [2]. This should be the main issue of slowness, indeed forcing in the code 4 MiB object size also for QCOW2 increased the speed a lot. Looking better I discovered that for raw files, we call rbd_create() with obj_order = 0 (if 'cluster_size' options is not defined), so the default object size is used. Instead for QCOW2, we use obj_order = 16, since the default 'cluster_size' defined for QCOW2, is 64 KiB. Using '-o cluster_size=2M' with qemu-img changed only the qcow2 cluster size, since in qcow2_co_create_opts() we remove the 'cluster_size' from QemuOpts calling qemu_opts_to_qdict_filtered(). For some reason that I have yet to understand, after this deletion, however remains in QemuOpts the default value of 'cluster_size' for qcow2 (64 KiB), that it's used in qemu_rbd_co_create_opts() At this point my doubts are: Does it make sense to use the same cluster_size as qcow2 as object_size in RBD? No, not really. But it also doesn't really make any sense to put a QCOW2 image within an RBD image. To clarify from the BZ, OpenStack does not put QCOW2 images on RBD, it converts QCOW2 images into raw images to store in RBD. Yes, that was my doubt, thanks for the confirmation. Also Daniel (+CC) confirmed me the same thing, but just to be complete he added that there is a case where OpenStack could use qcow2 on RBD, but in this case using in-kernel RBD, so the QEMU RBD is not involved. If we want to keep the 2 options separated, how can it be done? Should we rename the option in block/rbd.c? You can already pass overrides to the RBD block driver by just appending them after the "rbd:[:option1=value1[:option2=value2]]" portion, perhaps that could be re-used. I see, we should extend qemu_rbd_parse_filename() to suppurt it. Maybe if we don't want to support this configuration, we should print some warning messages. Thanks, Stefano
