Re: [PATCH v2 2/3] block: make BlockBackend->disable_request_queuing atomic

2023-03-07 Thread Philippe Mathieu-Daudé
On 7/3/23 22:04, Stefan Hajnoczi wrote: This field is accessed by multiple threads without a lock. Use explicit qatomic_read()/qatomic_set() calls. There is no need for acquire/release because blk_set_disable_request_queuing() doesn't provide any guarantees (it helps that it's used at BlockBacken

Re: [PATCH v2 1/3] block: make BlockBackend->quiesce_counter atomic

2023-03-07 Thread Philippe Mathieu-Daudé
On 7/3/23 22:04, Stefan Hajnoczi wrote: The main loop thread increments/decrements BlockBackend->quiesce_counter when drained sections begin/end. The counter is read in the I/O code path. Therefore this field is used to communicate between threads without a lock. Acquire/release are not necessar

[PATCH v2 2/3] block: make BlockBackend->disable_request_queuing atomic

2023-03-07 Thread Stefan Hajnoczi
This field is accessed by multiple threads without a lock. Use explicit qatomic_read()/qatomic_set() calls. There is no need for acquire/release because blk_set_disable_request_queuing() doesn't provide any guarantees (it helps that it's used at BlockBackend creation time and not when there is I/O

[PATCH v2 3/3] block: protect BlockBackend->queued_requests with a lock

2023-03-07 Thread Stefan Hajnoczi
The CoQueue API offers thread-safety via the lock argument that qemu_co_queue_wait() and qemu_co_enter_next() take. BlockBackend currently does not make use of the lock argument. This means that multiple threads submitting I/O requests can corrupt the CoQueue's QSIMPLEQ. Add a QemuMutex and pass i

[PATCH v2 0/3] block: protect BlockBackend->queued_requests with a lock

2023-03-07 Thread Stefan Hajnoczi
v2: - Use qatomic_fetch_inc/dec() for readability in Patch 1 [Hanna] QEMU block layer multi-queue support involves running I/O requests from multiple threads. Shared state must be protected somehow to avoid thread-safety issues. The BlockBackend->queued_requests CoQueue is accessed without a lock

[PATCH v2 1/3] block: make BlockBackend->quiesce_counter atomic

2023-03-07 Thread Stefan Hajnoczi
The main loop thread increments/decrements BlockBackend->quiesce_counter when drained sections begin/end. The counter is read in the I/O code path. Therefore this field is used to communicate between threads without a lock. Acquire/release are not necessary because the BlockBackend->in_flight coun

Re: [PATCH 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

2023-03-07 Thread Philippe Mathieu-Daudé
On 1/3/23 21:58, Stefan Hajnoczi wrote: monitor_cleanup() is called from the main loop thread. Calling AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks the AioContext and the latter's assertion that w

Re: [PATCH 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

2023-03-07 Thread Philippe Mathieu-Daudé
On 1/3/23 21:58, Stefan Hajnoczi wrote: The HMP monitor runs in the main loop thread. Calling AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks the AioContext and the latter's assertion that we're in t

Re: [PATCH 4/6] block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()

2023-03-07 Thread Philippe Mathieu-Daudé
On 1/3/23 21:57, Stefan Hajnoczi wrote: Since the AioContext argument was already NULL, AIO_WAIT_WHILE() was never going to unlock the AioContext. Therefore it is possible to replace AIO_WAIT_WHILE() with AIO_WAIT_WHILE_UNLOCKED(). Signed-off-by: Stefan Hajnoczi --- block/io.c | 2 +- 1 file

Re: [PATCH 3/6] block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

2023-03-07 Thread Philippe Mathieu-Daudé
On 2/3/23 11:19, Philippe Mathieu-Daudé wrote: On 1/3/23 21:57, Stefan Hajnoczi wrote: The following conversion is safe and does not change behavior:   GLOBAL_STATE_CODE();   ...    -  AIO_WAIT_WHILE(qemu_get_aio_context(), ...);    +  AIO_WAIT_WHILE_UNLOCKED(NULL, ...); Since we're in

Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

2023-03-07 Thread Philippe Mathieu-Daudé
On 1/3/23 21:57, Stefan Hajnoczi wrote: There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED() instead of AIO_WAIT_WHILE() to document that this code has already been audited and converted. The AioContext argument is already NULL so aio_context_release() is never called anyway. Sign

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

2023-03-07 Thread Stefan Hajnoczi
On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote: > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > > There is no need for the AioContext lock in bdrv_drain_all() because > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > > > Note that the NULL AioC

Re: [PATCH 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

2023-03-07 Thread Kevin Wolf
Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > AIO_WAIT_WHILE_UNLOCKED() is the future replacement for AIO_WAIT_WHILE(). Most > callers haven't been converted yet because they rely on the AioContext lock. I > looked through the code and found the easy cases that can be converted today.

Re: [PATCH 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

2023-03-07 Thread Kevin Wolf
Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > There is no need for the AioContext lock in bdrv_drain_all() because > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > the future it can be

Re: [PATCH] pflash: Fix blk_pread_nonzeroes()

2023-03-07 Thread Maciej S. Szmigiero
On 7.03.2023 15:02, Kevin Wolf wrote: Commit a4b15a8b introduced a new function blk_pread_nonzeroes(). Instead of reading directly from the root node of the BlockBackend, it reads from its 'file' child node. This can happen to mostly work for raw images (as long as the 'raw' format driver is in u

Re: [PULL 1/5] hw/nvme: move adjustment of data_units{read,written}

2023-03-07 Thread Joel Granados
On Mon, Mar 06, 2023 at 03:34:29PM +0100, Klaus Jensen wrote: > From: Joel Granados > > Move the rounding of bytes read/written into nvme_smart_log which > reports in units of 512 bytes, rounded up in thousands. This is in > preparation for adding the Endurance Group Information log page which >

Re: Deadlock with ide_issue_trim and draining

2023-03-07 Thread Hanna Czenczek
On 07.03.23 14:44, Hanna Czenczek wrote: On 07.03.23 13:22, Fiona Ebner wrote: Hi, I am suspecting that commit 7e5cdb345f ("ide: Increment BB in-flight counter for TRIM BH") introduced an issue in combination with draining.  From a debug session on a costumer's machine I gathered the following

Re: [PATCH 0/2] block/fuse: Let PUNCH_HOLE write zeroes

2023-03-07 Thread Kevin Wolf
Am 27.02.2023 um 11:47 hat Hanna Czenczek geschrieben: > Hi, > > https://gitlab.com/qemu-project/qemu/-/issues/1507 reports a bug in FUSE > exports: fallocate(PUNCH_HOLE) is implemented with blk_pdiscard(), but > its man page documents that a successful call will result in the data > being read as

Re: [PATCH] pflash: Fix blk_pread_nonzeroes()

2023-03-07 Thread Cédric Le Goater
On 3/7/23 15:02, Kevin Wolf wrote: Commit a4b15a8b introduced a new function blk_pread_nonzeroes(). Instead of reading directly from the root node of the BlockBackend, it reads from its 'file' child node. This can happen to mostly work for raw images (as long as the 'raw' format driver is in use,

Re: [PATCH v5 1/4] linux-aio: use LinuxAioState from the running thread

2023-03-07 Thread Stefan Hajnoczi
On Tue, Mar 07, 2023 at 09:48:51AM +0100, Kevin Wolf wrote: > Am 01.03.2023 um 17:16 hat Stefan Hajnoczi geschrieben: > > On Fri, Feb 03, 2023 at 08:17:28AM -0500, Emanuele Giuseppe Esposito wrote: > > > Remove usage of aio_context_acquire by always submitting asynchronous > > > AIO to the current

Re: [PULL 03/38] pflash: Only read non-zero parts of backend image

2023-03-07 Thread Cédric Le Goater
On 3/7/23 15:00, Kevin Wolf wrote: Am 03.03.2023 um 23:51 hat Maciej S. Szmigiero geschrieben: On 8.02.2023 12:19, Cédric Le Goater wrote: On 2/7/23 13:48, Kevin Wolf wrote: Am 07.02.2023 um 10:19 hat Cédric Le Goater geschrieben: On 2/7/23 09:38, Kevin Wolf wrote: Am 06.02.2023 um 16:54 hat

[PATCH] pflash: Fix blk_pread_nonzeroes()

2023-03-07 Thread Kevin Wolf
Commit a4b15a8b introduced a new function blk_pread_nonzeroes(). Instead of reading directly from the root node of the BlockBackend, it reads from its 'file' child node. This can happen to mostly work for raw images (as long as the 'raw' format driver is in use, but not actually doing anything), bu

Re: [PULL 03/38] pflash: Only read non-zero parts of backend image

2023-03-07 Thread Kevin Wolf
Am 03.03.2023 um 23:51 hat Maciej S. Szmigiero geschrieben: > On 8.02.2023 12:19, Cédric Le Goater wrote: > > On 2/7/23 13:48, Kevin Wolf wrote: > > > Am 07.02.2023 um 10:19 hat Cédric Le Goater geschrieben: > > > > On 2/7/23 09:38, Kevin Wolf wrote: > > > > > Am 06.02.2023 um 16:54 hat Cédric Le G

Re: Deadlock with ide_issue_trim and draining

2023-03-07 Thread Hanna Czenczek
On 07.03.23 13:22, Fiona Ebner wrote: Hi, I am suspecting that commit 7e5cdb345f ("ide: Increment BB in-flight counter for TRIM BH") introduced an issue in combination with draining. From a debug session on a costumer's machine I gathered the following information: * The QEMU process hangs in a

Re: [PULL 0/5] hw/nvme updates

2023-03-07 Thread Peter Maydell
On Mon, 6 Mar 2023 at 14:34, Klaus Jensen wrote: > > From: Klaus Jensen > > Hi, > > The following changes since commit f003dd8d81f7d88f4b1f8802309eaa76f6eb223a: > > Merge tag 'pull-tcg-20230305' of https://gitlab.com/rth7680/qemu into > staging (2023-03-06 10:20:04 +) > > are available in

Deadlock with ide_issue_trim and draining

2023-03-07 Thread Fiona Ebner
Hi, I am suspecting that commit 7e5cdb345f ("ide: Increment BB in-flight counter for TRIM BH") introduced an issue in combination with draining. >From a debug session on a costumer's machine I gathered the following information: * The QEMU process hangs in aio_poll called during draining and doesn

Re: [PATCH v10 00/12] parallels: Refactor the code of images checks and fix a bug

2023-03-07 Thread Hanna Czenczek
On 03.02.23 10:18, Alexander Ivanov wrote: Fix image inflation when offset in BAT is out of image. Replace whole BAT syncing by flushing only dirty blocks. Move all the checks outside the main check function in separate functions Use WITH_QEMU_LOCK_GUARD for simplier code. Fix incorrect condi

Re: [PATCH v10 09/12] parallels: Move check of leaks to a separate function

2023-03-07 Thread Hanna Czenczek
On 03.02.23 10:18, Alexander Ivanov wrote: We will add more and more checks so we need a better code structure in parallels_co_check. Let each check performs in a separate loop in a separate helper. Signed-off-by: Alexander Ivanov --- block/parallels.c | 80 ---

Re: [PATCH v5 1/4] linux-aio: use LinuxAioState from the running thread

2023-03-07 Thread Kevin Wolf
Am 07.03.2023 um 11:58 hat Paolo Bonzini geschrieben: > On 3/7/23 09:48, Kevin Wolf wrote: > > You mean we have a device that has a separate iothread, but a request is > > submitted from the main thread? This isn't even allowed today; if a node > > is in an iothread, all I/O must be submitted from

Re: [PATCH v10 07/12] parallels: Move check of cluster outside image to a separate function

2023-03-07 Thread Hanna Czenczek
On 03.02.23 10:18, Alexander Ivanov wrote: We will add more and more checks so we need a better code structure in parallels_co_check. Let each check performs in a separate loop in a separate helper. Signed-off-by: Alexander Ivanov Reviewed-by: Denis V. Lunev --- block/parallels.c | 81 ++

Re: [PATCH v5 1/4] linux-aio: use LinuxAioState from the running thread

2023-03-07 Thread Paolo Bonzini
On 3/7/23 09:48, Kevin Wolf wrote: You mean we have a device that has a separate iothread, but a request is submitted from the main thread? This isn't even allowed today; if a node is in an iothread, all I/O must be submitted from that iothread. Do you know any code that does submit I/O from the

Re: [PATCH v5 1/4] linux-aio: use LinuxAioState from the running thread

2023-03-07 Thread Kevin Wolf
Am 01.03.2023 um 17:16 hat Stefan Hajnoczi geschrieben: > On Fri, Feb 03, 2023 at 08:17:28AM -0500, Emanuele Giuseppe Esposito wrote: > > Remove usage of aio_context_acquire by always submitting asynchronous > > AIO to the current thread's LinuxAioState. > > > > In order to prevent mistakes from t