[PATCH v7 2/4] qapi/monitor: refactor set/expire_password with enums

2021-10-21 Thread Stefan Reiter
'protocol' and 'connected' are better suited as enums than as strings, make use of that. No functional change intended. Suggested-by: Markus Armbruster Reviewed-by: Markus Armbruster Signed-off-by: Stefan Reiter --- monitor/hmp-cmds.c | 29 +++-- monitor/qmp-cmds.c

[PATCH v7 4/4] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate

2021-10-21 Thread Stefan Reiter
VNC only supports 'keep' here, enforce this via a seperate SetPasswordActionVnc enum and mark the option 'deprecated' (as it is useless with only one value possible). Also add a deprecation note to docs. Suggested-by: Eric Blake Reviewed-by: Markus Armbruster Signed-off-by: Stefan Reiter

[PATCH v7 0/4] VNC-related HMP/QMP fixes

2021-10-21 Thread Stefan Reiter
't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (4): monitor/hmp: add support for flag argument with value qapi/monitor: refactor set/expire_password with enums qapi/monitor: allow VNC display id in set/expire_password qapi/monitor: on

[PATCH v7 1/4] monitor/hmp: add support for flag argument with value

2021-10-21 Thread Stefan Reiter
Adds support for the "-xV" parameter type, where "-x" denotes a flag name and the "V" suffix indicates that this flag is supposed to take an arbitrary string parameter. These parameters are always optional, the entry in the qdict will be omitted if the flag is not

[PATCH v7 3/4] qapi/monitor: allow VNC display id in set/expire_password

2021-10-21 Thread Stefan Reiter
col-discriminated unions. Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- hmp-commands.hx| 24 +- monitor/hmp-cmds.c | 45 -- monitor/qmp-cmds.c | 36 ++- qapi/ui.json | 112 +++-- 4 files changed,

Re: [PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password

2021-10-21 Thread Stefan Reiter
On 10/21/21 7:05 AM, Markus Armbruster wrote: Stefan Reiter writes: It is possible to specify more than one VNC server on the command line, either with an explicit ID or the auto-generated ones à la "default", "vnc2", "vnc3", ... It is not possible

[PATCH v6 1/5] monitor/hmp: add support for flag argument with value

2021-10-20 Thread Stefan Reiter
Adds support for the "-xV" parameter type, where "-x" denotes a flag name and the "V" suffix indicates that this flag is supposed to take an arbitrary string parameter. These parameters are always optional, the entry in the qdict will be omitted if the flag is not

[PATCH v6 0/5] VNC-related HMP/QMP fixes

2021-10-20 Thread Stefan Reiter
factor QMP schema for set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type

[PATCH v6 3/5] qapi/monitor: allow VNC display id in set/expire_password

2021-10-20 Thread Stefan Reiter
col-discriminated unions. Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- hmp-commands.hx| 24 +- monitor/hmp-cmds.c | 51 +++-- monitor/qmp-cmds.c | 36 ++- qapi/ui.json | 112 +++-- 4 files chang

[PATCH v6 5/5] docs: add deprecation note about 'set_password' param 'connected'

2021-10-20 Thread Stefan Reiter
Signed-off-by: Stefan Reiter --- Seperate patch since it read a bit unsure in the review, feel free to either drop or squash this. docs/about/deprecated.rst | 6 ++ 1 file changed, 6 insertions(+) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 0bed6ecb1d

[PATCH v6 4/5] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate

2021-10-20 Thread Stefan Reiter
VNC only supports 'keep' here, enforce this via a seperate SetPasswordActionVnc enum and mark the option 'deprecated' (as it is useless with only one value possible). Suggested-by: Eric Blake Signed-off-by: Stefan Reiter --- monitor/qmp-cmds.c | 5 - qapi/ui.json | 21

[PATCH v6 2/5] qapi/monitor: refactor set/expire_password with enums

2021-10-20 Thread Stefan Reiter
'protocol' and 'connected' are better suited as enums than as strings, make use of that. No functional change intended. Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- monitor/hmp-cmds.c | 29 +++-- monitor/qmp-cmds.c | 37

[PATCH v5 3/4] qapi/monitor: allow VNC display id in set/expire_password

2021-10-19 Thread Stefan Reiter
col-discriminated unions. Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- hmp-commands.hx| 24 +- monitor/hmp-cmds.c | 51 ++--- monitor/qmp-cmds.c | 44 +- qapi/ui.json | 112 +++-- 4 files cha

[PATCH v5 1/4] monitor/hmp: add support for flag argument with value

2021-10-19 Thread Stefan Reiter
iven. Reviewed-by: Eric Blake Signed-off-by: Stefan Reiter --- monitor/hmp.c | 17 - monitor/monitor-internal.h | 3 ++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index d50c3124e1..a32dce7a35 100644 --- a/mon

[PATCH v5 0/4] VNC-related HMP/QMP fixes

2021-10-19 Thread Stefan Reiter
Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type qemu: Stefan Reiter (4): monitor/hmp: add support for flag argument with va

[PATCH v5 2/4] qapi/monitor: refactor set/expire_password with enums

2021-10-19 Thread Stefan Reiter
'protocol' and 'connected' are better suited as enums than as strings, make use of that. No functional change intended. Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- monitor/hmp-cmds.c | 17 +++-- monitor/qmp-cmds.c | 35 ++- qapi

[PATCH v5 4/4] qapi/monitor: only allow 'keep' SetPasswordAction for VNC and deprecate

2021-10-19 Thread Stefan Reiter
VNC only supports 'keep' here, enforce this via a seperate SetPasswordActionVnc enum and mark the option 'deprecated' (as it is useless with only one value possible). Suggested-by: Eric Blake Signed-off-by: Stefan Reiter --- monitor/qmp-cmds.c | 5 - qapi/ui.json | 21

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

2021-10-14 Thread Stefan Reiter
On 10/14/21 9:14 AM, Markus Armbruster wrote: Stefan Reiter writes: On 10/12/21 11:24 AM, Markus Armbruster wrote: Stefan Reiter writes: It is possible to specify more than one VNC server on the command line, either with an explicit ID or the auto-generated ones à "default", &qu

Re: [PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

2021-10-13 Thread Stefan Reiter
On 10/12/21 11:24 AM, Markus Armbruster wrote: Stefan Reiter writes: It is possible to specify more than one VNC server on the command line, either with an explicit ID or the auto-generated ones à la "default", "vnc2", "vnc3", ... It is not possible

Re: [PATCH] monitor/qmp: fix race with clients disconnecting early

2021-10-13 Thread Stefan Reiter
On 10/12/21 11:27 AM, Markus Armbruster wrote: Stefan, any thoughts on this? Sorry, I didn't get to work on implementing this. Idea 3 does seem very reasonable, though I suppose the question is what all should go into the per-session state, and also how it is passed down. We did roll out my

[PATCH v4 1/2] monitor/hmp: add support for flag argument with value

2021-09-28 Thread Stefan Reiter
iven. Reviewed-by: Eric Blake Signed-off-by: Stefan Reiter --- monitor/hmp.c | 17 - 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/monitor/hmp.c b/monitor/hmp.c index d50c3124e1..a32dce7a35 100644 --- a/monitor/hmp.c +++ b/monitor/hmp.c @@ -980,6 +980,7 @@ static

[PATCH v4 2/2] monitor: refactor set/expire_password and allow VNC display id

2021-09-28 Thread Stefan Reiter
-discriminated unions. Suggested-by: Eric Blake Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- hmp-commands.hx| 24 --- monitor/hmp-cmds.c | 57 ++- monitor/qmp-cmds.c | 62 ++-- qapi/ui.json | 173 +---

[PATCH v4] VNC-related HMP/QMP fixes

2021-09-28 Thread Stefan Reiter
or set/expire_password as suggested by Eric Blake and Markus Armbruster v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (2): monit

[PATCH v3 1/3] monitor/hmp: correctly invert password argument detection again

2021-09-20 Thread Stefan Reiter
s inversion is wrong, as this will now ask the user for a password if one is already provided, and simply fail if none is given. Fixes: cfb5387a1d ("hmp: remove "change vnc TARGET" command") Reviewed-by: Marc-André Lureau Signed-off-by: Stefan Reiter --- monitor/hmp-cmds.c |

[PATCH v3 3/3] monitor: refactor set/expire_password and allow VNC display id

2021-09-20 Thread Stefan Reiter
-discriminated unions. Suggested-by: Eric Blake Suggested-by: Markus Armbruster Signed-off-by: Stefan Reiter --- The union schema simplifies the QMP code, but makes the HMP part a bit more involved. Since the parameters are practically the same, is there a way to just pass the HMP generated qdict

[PATCH v3 0/3] VNC-related HMP/QMP fixes

2021-09-20 Thread Stefan Reiter
"flags with values" arg type Stefan Reiter (3): monitor/hmp: correctly invert password argument detection again monitor/hmp: add support for flag argument with value monitor: refactor set/expire_password and allow VNC display id hmp-commands.hx| 29 monitor/

[PATCH v3 2/3] monitor/hmp: add support for flag argument with value

2021-09-20 Thread Stefan Reiter
Adds support for the "-xS" parameter type, where "-x" denotes a flag name and the "S" suffix indicates that this flag is supposed to take an arbitrary string parameter. These parameters are always optional, the entry in the qdict will be omitted if the flag is not

Re: [PATCH v3] qemu-sockets: fix unix socket path copy (again)

2021-09-07 Thread Stefan Reiter
Thanks for the patch, ran into the same issue here and was about to send my own ;) On 9/1/21 3:16 PM, Michael Tokarev wrote: Commit 4cfd970ec188558daa6214f26203fe553fb1e01f added an assert which ensures the path within an address of a unix socket returned from the kernel is at least one byte

[PATCH v2 0/3] VNC-related HMP/QMP fixes

2021-09-01 Thread Stefan Reiter
and left for a later date. v1 -> v2: * add Marc-André's R-b on patch 1 * use '-d' flag as suggested by Eric Blake and Gerd Hoffmann * I didn't see a way to do this yet, so I added a "flags with values" arg type Stefan Reiter (3): monitor/hmp: correctly invert password argument de

[PATCH v2 2/3] monitor/hmp: add support for flag argument with value

2021-09-01 Thread Stefan Reiter
Adds support for the "-xS" parameter type, where "-x" denotes a flag name and the "S" suffix indicates that this flag is supposed to take an arbitrary string parameter. These parameters are always optional, the entry in the qdict will be omitted if the flag is not

[PATCH v2 3/3] monitor: allow VNC related QMP and HMP commands to take a display ID

2021-09-01 Thread Stefan Reiter
y adding a "display" parameter to the "set_password" and "expire_password" QMP and HMP commands. For HMP, the display is specified using the "-d" value flag. Signed-off-by: Stefan Reiter --- hmp-commands.hx| 29 +++-- monitor/hm

[PATCH v2 1/3] monitor/hmp: correctly invert password argument detection again

2021-09-01 Thread Stefan Reiter
s inversion is wrong, as this will now ask the user for a password if one is already provided, and simply fail if none is given. Fixes: cfb5387a1d ("hmp: remove "change vnc TARGET" command") Reviewed-by: Marc-André Lureau Signed-off-by: Stefan Reiter --- monitor/hmp-cmds.c |

Re: [PATCH] monitor/qmp: fix race with clients disconnecting early

2021-08-26 Thread Stefan Reiter
On 26/08/2021 15:50, Markus Armbruster wrote: Markus Armbruster writes: [...] Let me re-explain the bug in my own words, to make sure I understand. A QMP monitor normally runs in the monitor I/O thread. A QMP monitor can serve only one client at a time. It executes out-of-band commands

Re: [PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID

2021-08-25 Thread Stefan Reiter
On 8/25/21 12:59 PM, Marc-André Lureau wrote: Hi On Wed, Aug 25, 2021 at 1:39 PM Stefan Reiter wrote: It is possible to specify more than one VNC server on the command line, either with an explicit ID or the auto-generated ones à la "default", "vnc2", "vnc3&

[PATCH 2/2] monitor: allow VNC related QMP and HMP commands to take a display ID

2021-08-25 Thread Stefan Reiter
" to allow correct parsing. With this prefix, no existing command or workflow should be affected. While rewriting the descriptions, also remove the line "Use zero to make the password stay valid forever." from 'set_password', I believe this was intended for 'expire_password', but would

[PATCH 0/2] VNC-related HMP/QMP fixes

2021-08-25 Thread Stefan Reiter
and left for a later date. Stefan Reiter (2): monitor/hmp: correctly invert password argument detection again monitor: allow VNC related QMP and HMP commands to take a display ID hmp-commands.hx| 28 +++- monitor/hmp-cmds.c | 22 +++--- monitor/qmp

[PATCH 1/2] monitor/hmp: correctly invert password argument detection again

2021-08-25 Thread Stefan Reiter
s inversion is wrong, as this will now ask the user for a password if one is already provided, and simply fail if none is given. Fixes: cfb5387a1d ("hmp: remove "change vnc TARGET" command") Signed-off-by: Stefan Reiter --- monitor/hmp-cmds.c | 2 +- 1 file changed, 1 insertion

[PATCH] monitor/qmp: fix race with clients disconnecting early

2021-08-23 Thread Stefan Reiter
From: Stefan Reiter The following sequence can produce a race condition that results in responses meant for different clients being sent to the wrong one: (QMP, no OOB) 1) client A connects 2) client A sends 'qmp_capabilities' 3) 'qmp_dispatch' runs in coroutine, schedules out

Re: [PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB

2021-03-29 Thread Stefan Reiter
On 3/26/21 3:48 PM, Markus Armbruster wrote: Wolfgang Bumiller writes: On Thu, Mar 18, 2021 at 02:35:50PM +0100, Stefan Reiter wrote: If OOB is disabled, events received in monitor_qmp_event will be handled in the main context. Thus, we must not acquire a qmp_queue_lock

[PATCH v2] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB

2021-03-22 Thread Stefan Reiter
not need to be called from main context, so we can call it immediately after popping a request from the queue, which allows us to drop the qmp_queue_lock mutex before yielding. Suggested-by: Wolfgang Bumiller Signed-off-by: Stefan Reiter --- v2: * different approach: move everything that needs

Re: [PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB

2021-03-22 Thread Stefan Reiter
On 3/22/21 12:08 PM, Wolfgang Bumiller wrote: On Thu, Mar 18, 2021 at 02:35:50PM +0100, Stefan Reiter wrote: If OOB is disabled, events received in monitor_qmp_event will be handled in the main context. Thus, we must not acquire a qmp_queue_lock there, as the dispatcher coroutine holds one over

[PATCH] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB

2021-03-18 Thread Stefan Reiter
to the iothread afterward, and thus the cleanup will happen before it gets to its next iteration. Signed-off-by: Stefan Reiter --- We've had some spurious reports of people reporting (usually multiple) total VM hangs on our forum, and finally got our hands on some stack traces: Thread 1 (Thread

Re: Potential regression in 'qemu-img convert' to LVM

2021-03-04 Thread Stefan Reiter
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

[PATCH] migration: only check page size match if RAM postcopy is enabled

2021-02-04 Thread Stefan Reiter
tcopy") Signed-off-by: Stefan Reiter --- migration/ram.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/ram.c b/migration/ram.c index 7811cde643..6ace15261c 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3521,7 +3521,7 @@ static int ram_load_precopy(Q

[PATCH] docs: don't install corresponding man page if guest agent is disabled

2021-01-28 Thread Stefan Reiter
No sense outputting the qemu-ga and qemu-ga-ref man pages when the guest agent binary itself is disabled. This mirrors behaviour from before the meson switch. Signed-off-by: Stefan Reiter --- docs/meson.build | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs

Re: [PATCH] migration/block-dirty-bitmap: fix larger granularity bitmaps

2020-10-22 Thread Stefan Reiter
On 10/21/20 5:17 PM, Vladimir Sementsov-Ogievskiy wrote: 21.10.2020 17:44, Stefan Reiter wrote: sectors_per_chunk is a 64 bit integer, but the calculation is done in 32 bits, leading to an overflow for coarse bitmap granularities. If that results in the value 0, it leads to a hang where

[PATCH] migration/block-dirty-bitmap: fix larger granularity bitmaps

2020-10-21 Thread Stefan Reiter
-by: Stefan Reiter --- migration/block-dirty-bitmap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 5bef793ac0..5398869e2b 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c

Re: [SeaBIOS] Regression with latest SeaBIOS booting multi-disk root LVs?

2020-09-21 Thread Stefan Reiter
On 21/09/2020 15:44, Paul Menzel wrote: Dear Stefan, Am 21.09.20 um 15:10 schrieb Stefan Reiter: since SeaBIOS 1.14.0 (QEMU 5.1) VMs with LVM root disks spanning more than one PV fail to boot, if only the first is set as bootable. I believe this is due to the changes in SeaBIOS only

Regression with latest SeaBIOS booting multi-disk root LVs?

2020-09-21 Thread Stefan Reiter
Hi list, since SeaBIOS 1.14.0 (QEMU 5.1) VMs with LVM root disks spanning more than one PV fail to boot, if only the first is set as bootable. I believe this is due to the changes in SeaBIOS only initializing drives marked as 'bootable' by QEMU. One fix is to mark all disks containing root

Re: Potential regression in 'qemu-img convert' to LVM

2020-09-15 Thread Stefan Reiter
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

Potential regression in 'qemu-img convert' to LVM

2020-09-14 Thread Stefan Reiter
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

Re: [PATCH 0/3] Add support for sequential backups

2020-09-14 Thread Stefan Reiter
Friendly ping :) On 8/26/20 2:13 PM, Stefan Reiter wrote: Backups can already be done for multiple drives in a transaction. However, these jobs will start all at once, potentially hogging a lot of disk IO all at once. This problem is made worse, since IO throttling is only available on a per

[PATCH 2/3] blockdev: add sequential mode to *-backup transactions

2020-08-26 Thread Stefan Reiter
a host's IO capabilities in general. Signed-off-by: Stefan Reiter --- blockdev.c| 25 ++--- qapi/transaction.json | 6 +- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3848a9c8ab..3691e5e791 100644 --- a/blockdev.c

[PATCH 1/3] job: add sequential transaction support

2020-08-26 Thread Stefan Reiter
Jobs in a sequential transaction should never be started with job_start manually. job_txn_start_seq and the sequentially called job_start will take care of it, 'assert'ing in case a job is already running or has finished. Signed-off-by: Stefan Reiter --- include/qemu/job.h | 12

[PATCH 0/3] Add support for sequential backups

2020-08-26 Thread Stefan Reiter
in a different fashion. This series is the result of aligning our internal changes closer to upstream, and might be useful for other people as well. Stefan Reiter (3): job: add sequential transaction support blockdev: add sequential mode to *-backup transactions backup: initialize bcs bitmap on job

[PATCH 3/3] backup: initialize bcs bitmap on job create, not start

2020-08-26 Thread Stefan Reiter
After backup_init_bcs_bitmap the copy-before-write behaviour is active. This way, multiple backup jobs created at once but running in a sequential transaction will still represent the same point in time. Signed-off-by: Stefan Reiter --- I'd imagine this was done on job start for a purpose, so

Re: [PATCH for-5.1 v2 2/2] iotests: add test for unaligned granularity bitmap backup

2020-08-10 Thread Stefan Reiter
for applying the patches! On 10.08.20 11:55, Stefan Reiter wrote: Start a VM with a 4097 byte image attached, add a 4096 byte granularity dirty bitmap, mark it dirty, and then do a backup. This used to run into an assert and fail, check that it works as expected and also check the created image

[PATCH for-5.1 v2 2/2] iotests: add test for unaligned granularity bitmap backup

2020-08-10 Thread Stefan Reiter
. Signed-off-by: Stefan Reiter --- I saw Andrey's big series covering iotest 303 so I went for 304. Never submitted one before so I hope that's okay, if not feel free to renumber it. tests/qemu-iotests/304 | 68 ++ tests/qemu-iotests/304.out | 2 ++ tests/qemu

[PATCH for-5.1 v2 1/2] block/block-copy: always align copied region to cluster size

2020-08-10 Thread Stefan Reiter
ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy, which requires the 'bytes' parameter to be aligned to cluster size. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Stefan Reiter --- I've now marked it for-5.1 since it is just a fix, but it's probably okay if done later

[PATCH] block/block-copy: always align copied region to cluster size

2020-08-06 Thread Stefan Reiter
. Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy, which requires the 'bytes' parameter to be aligned to cluster size. Signed-off-by: Stefan Reiter --- This causes backups with unaligned image sizes to fail on the last block in my testing (e.g. a backup job with 4k cluster size

Re: [RFC PATCH 2/3] block: Allow bdrv_run_co() from different AioContext

2020-05-27 Thread Stefan Reiter
On 5/26/20 6:42 PM, Kevin Wolf wrote: Am 25.05.2020 um 18:41 hat Kevin Wolf geschrieben: Am 25.05.2020 um 16:18 hat Stefan Reiter geschrieben: On 5/12/20 4:43 PM, Kevin Wolf wrote: Coroutine functions that are entered through bdrv_run_co() are already safe to call from synchronous code

Re: [RFC PATCH 2/3] block: Allow bdrv_run_co() from different AioContext

2020-05-25 Thread Stefan Reiter
On 5/12/20 4:43 PM, Kevin Wolf wrote: Coroutine functions that are entered through bdrv_run_co() are already safe to call from synchronous code in a different AioContext because bdrv_coroutine_enter() will schedule them in the context of the node. However, the coroutine fastpath still requires

Re: [RFC PATCH 3/3] block: Assert we're running in the right thread

2020-05-14 Thread Stefan Reiter
On 5/12/20 4:43 PM, Kevin Wolf wrote: tracked_request_begin() is called for most I/O operations, so it's a good place to assert that we're indeed running in the home thread of the node's AioContext. Is this patch supposed to be always correct or only together with nr. 2? I changed our code

Re: [RFC] bdrv_flush: only use fast path when in owned AioContext

2020-05-12 Thread Stefan Reiter
On 5/12/20 1:32 PM, Kevin Wolf wrote: Am 12.05.2020 um 12:57 hat Kevin Wolf geschrieben: Am 11.05.2020 um 18:50 hat Stefan Reiter geschrieben: Just because we're in a coroutine doesn't imply ownership of the context of the flushed drive. In such a case use the slow path which explicitly enters

[RFC] bdrv_flush: only use fast path when in owned AioContext

2020-05-11 Thread Stefan Reiter
Just because we're in a coroutine doesn't imply ownership of the context of the flushed drive. In such a case use the slow path which explicitly enters bdrv_flush_co_entry in the correct AioContext. Signed-off-by: Stefan Reiter --- We've experienced some lockups in this codepath when taking

qemu_coroutine_yield switches thread?

2020-04-16 Thread Stefan Reiter
Hi list, quick question: Can a resume from a qemu_coroutine_yield happen in a different thread? Well, it can, since I'm seeing it happen, but is that okay or a bug? I.e. in a backup-job the following can sporadically trip: unsigned long tid = pthread_self();

[PATCH for-5.0 v5 3/3] backup: don't acquire aio_context in backup_clean

2020-04-07 Thread Stefan Reiter
using IO threads, since the BDRV_POLL_WHILE in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock once, thus deadlocking with the IO thread. This is a partial revert of 0abf2581717a19. Signed-off-by: Stefan Reiter Reviewed-by: Max Reitz --- block/backup.c | 4

[PATCH for-5.0 v5 0/3] Fix some AIO context locking in jobs

2020-04-07 Thread Stefan Reiter
Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 qemu: Stefan Reiter (3): job: take each job's lock individually in job_txn_apply replication: assert we own context before job_cancel_sync backup: don't acquire aio_context in backup_clean block/backup.c|

[PATCH for-5.0 v5 1/3] job: take each job's lock individually in job_txn_apply

2020-04-07 Thread Stefan Reiter
. This is only necessary for qmp_block_job_finalize, qmp_job_finalize and job_exit, since everyone else calls through job_exit. One test needed adapting, since it calls job_finalize directly, so it manually needs to acquire the correct context. Signed-off-by: Stefan Reiter --- blockdev.c

[PATCH for-5.0 v5 2/3] replication: assert we own context before job_cancel_sync

2020-04-07 Thread Stefan Reiter
as the one used for the commit_job. Signed-off-by: Stefan Reiter --- block/replication.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/replication.c b/block/replication.c index 413d95407d..da013c2041 100644 --- a/block/replication.c +++ b/block/replication.c @@ -144,12

Re: [PATCH v4 2/3] replication: acquire aio context before calling job_cancel_sync

2020-04-02 Thread Stefan Reiter
On 02/04/2020 14:41, Max Reitz wrote: On 01.04.20 10:15, Stefan Reiter wrote: job_cancel_sync requires the job's lock to be held, all other callers already do this (replication_stop, drive_backup_abort, blockdev_backup_abort, job_cancel_sync_all, cancel_common). I think all other callers come

Re: [PATCH v4 1/3] job: take each job's lock individually in job_txn_apply

2020-04-02 Thread Stefan Reiter
On 02/04/2020 14:33, Max Reitz wrote: On 01.04.20 10:15, Stefan Reiter wrote: All callers of job_txn_apply hold a single job's lock, but different jobs within a transaction can have different contexts, thus we need to lock each one individually before applying the callback function. Similar

[PATCH v4 1/3] job: take each job's lock individually in job_txn_apply

2020-04-01 Thread Stefan Reiter
directly, so it manually needs to acquire the correct context. Signed-off-by: Stefan Reiter --- job.c | 48 ++- tests/test-blockjob.c | 2 ++ 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/job.c b/job.c index 134a07b92e

[PATCH v4 0/3] Fix some AIO context locking in jobs

2020-04-01 Thread Stefan Reiter
to the end to not introduce temporary breakages * added more fixes to job txn patch (should now pass the tests) Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 qemu: Stefan Reiter (3): job: take each job's lock individually in job_txn_apply replication: acquire aio

[PATCH v4 3/3] backup: don't acquire aio_context in backup_clean

2020-04-01 Thread Stefan Reiter
using IO threads, since the BDRV_POLL_WHILE in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock once, thus deadlocking with the IO thread. This is a partial revert of 0abf2581717a19. Signed-off-by: Stefan Reiter --- With the two previous patches applied, the com

[PATCH v4 2/3] replication: acquire aio context before calling job_cancel_sync

2020-04-01 Thread Stefan Reiter
job_cancel_sync requires the job's lock to be held, all other callers already do this (replication_stop, drive_backup_abort, blockdev_backup_abort, job_cancel_sync_all, cancel_common). Signed-off-by: Stefan Reiter --- block/replication.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion

[PATCH v3 1/3] job: take each job's lock individually in job_txn_apply

2020-03-31 Thread Stefan Reiter
directly, so it manually needs to acquire the correct context. Signed-off-by: Stefan Reiter --- job.c | 48 ++- tests/test-blockjob.c | 2 ++ 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/job.c b/job.c index 134a07b92e

[PATCH v3 0/3] Fix some AIO context locking in jobs

2020-03-31 Thread Stefan Reiter
/qemu-devel/2020-03/msg07929.html Changes from v2: * reordered patch 1 to the end to not introduce temporary breakages * added more fixes to job txn patch (should now pass the tests) Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 qemu: Stefan Reiter (3): job: take

[PATCH v3 2/3] replication: acquire aio context before calling job_cancel_sync

2020-03-31 Thread Stefan Reiter
job_cancel_sync requires the job's lock to be held, all other callers already do this (replication_stop, drive_backup_abort, blockdev_backup_abort, job_cancel_sync_all, cancel_common). Signed-off-by: Stefan Reiter --- block/replication.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion

[PATCH v3 3/3] backup: don't acquire aio_context in backup_clean

2020-03-31 Thread Stefan Reiter
using IO threads, since the BDRV_POLL_WHILE in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock once, thus deadlocking with the IO thread. This is a partial revert of 0abf2581717a19. Signed-off-by: Stefan Reiter --- With the two previous patches applied, the com

[PATCH v2 3/3] replication: acquire aio context before calling job_cancel_sync

2020-03-26 Thread Stefan Reiter
job_cancel_sync requires the job's lock to be held, all other callers already do this (replication_stop, drive_backup_abort, blockdev_backup_abort, job_cancel_sync_all, cancel_common). Signed-off-by: Stefan Reiter --- block/replication.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion

[PATCH v2 2/3] job: take each job's lock individually in job_txn_apply

2020-03-26 Thread Stefan Reiter
and reacquiring it after to avoid recursive locks which might break AIO_WAIT_WHILE in the callback. Signed-off-by: Stefan Reiter --- job.c | 32 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/job.c b/job.c index 134a07b92e..e0966162fa 100644

[PATCH v2 1/3] backup: don't acquire aio_context in backup_clean

2020-03-26 Thread Stefan Reiter
using IO threads, since the BDRV_POLL_WHILE in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock once, thus deadlocking with the IO thread. This is a partial revert of 0abf2581717a19. Signed-off-by: Stefan Reiter --- block/backup.c | 4 1 file changed, 4 deleti

[PATCH v2 0/3] Fix some AIO context locking in jobs

2020-03-26 Thread Stefan Reiter
/qemu-devel/2020-03/msg07929.html I *think* the second patch also fixes the hangs on backup abort that I and Dietmar noticed in v1, but I'm not sure, they we're somewhat intermittent before too. Changes from v1: * fixed commit message for patch 1 * added patches 2 and 3 Stefan Reiter (3): backup

Re: [PATCH] backup: don't acquire aio_context in backup_clean

2020-03-26 Thread Stefan Reiter
On 26/03/2020 06:54, Vladimir Sementsov-Ogievskiy wrote: 25.03.2020 18:50, Stefan Reiter wrote: backup_clean is only ever called as a handler via job_exit, which Hmm.. I'm afraid it's not quite correct. job_clean   job_finalize_single job_completed_txn_abort (lock aio context

[PATCH] backup: don't acquire aio_context in backup_clean

2020-03-25 Thread Stefan Reiter
for disks using IO threads, since the BDRV_POLL_WHILE in bdrv_backup_top_drop -> bdrv_do_drained_begin would only release the lock once, thus deadlocking with the IO thread. Signed-off-by: Stefan Reiter --- This is a fix for the issue discussed in this part of the thread: https://lists.gnu.

Re: backup transaction with io-thread core dumps

2020-03-25 Thread Stefan Reiter
On 24/03/2020 17:49, Dietmar Maurer wrote: A more serious issue is that I also get a hang inside the poll loop when the VM is under load. For example, running "stress -d 5" inside the VM (Debian Buster). Then running a simply drive-backup like: { "execute": "drive-backup", "arguments": {