[PATCH v7 17/18] job.c: enable job lock/unlock and remove Aiocontext locks

2022-06-16 Thread Emanuele Giuseppe Esposito
ned-off-by: Emanuele Giuseppe Esposito --- blockdev.c | 72 - include/qemu/job.h | 19 +++ job-qmp.c| 44 +++ job.c| 92 ++-- tests/unit/test-b

[PATCH v7 10/18] jobs: rename static functions called with job_mutex held

2022-06-16 Thread Emanuele Giuseppe Esposito
lock. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- job.c | 247 +- 1 file changed, 141 insertions(+), 106 deletions(-) diff --git a/job.c b/job.c index 55b92b2332..4f4b387625 100644 --- a/job.c +++ b/jo

[PATCH v7 05/18] job.h: add _locked duplicates for job API functions called with and without job_mutex

2022-06-16 Thread Emanuele Giuseppe Esposito
) job_cancel_requested (_locked version is static) Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job.h | 25 +--- job.c | 48

[PATCH v7 18/18] block_job_query: remove atomic read

2022-06-16 Thread Emanuele Giuseppe Esposito
Not sure what the atomic here was supposed to do, since job.busy is protected by the job lock. Since the whole function is called under job_mutex, just remove the atomic. Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 2 +- 1 file changed, 1 insertion

[PATCH v7 14/18] commit and mirror: create new nodes using bdrv_get_aio_context, and not the job aiocontext

2022-06-16 Thread Emanuele Giuseppe Esposito
We are always using the given bs AioContext, so there is no need to take the job ones (which is identical anyways). This also reduces the point we need to check when protecting job.aio_context field. Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- block/commit.c | 4

[PATCH v7 07/18] jobs: add job lock in find_* functions

2022-06-16 Thread Emanuele Giuseppe Esposito
*. Signed-off-by: Emanuele Giuseppe Esposito --- blockdev.c | 46 +++--- job-qmp.c | 37 + 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/blockdev.c b/blockdev.c index b1099e678c..6f83783f10 100644

[PATCH v7 06/18] jobs: protect jobs with job_lock/unlock

2022-06-16 Thread Emanuele Giuseppe Esposito
macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 18 --- block/replication.c | 8 ++- blockdev.c | 17 -- blockjob.c | 56 +--- job-qmp.c | 2 + job.c | 125

[PATCH v7 02/18] job.h: categorize fields in struct Job

2022-06-16 Thread Emanuele Giuseppe Esposito
Categorize the fields in struct Job to understand which ones need to be protected by the job mutex and which don't. Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job.h | 61 +++--- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git

[PATCH v7 09/18] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU

2022-06-16 Thread Emanuele Giuseppe Esposito
Once job lock is used and aiocontext is removed, mirror has to perform job operations under the same critical section, using the helpers prepared in previous commit. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito Reviewed

[PATCH v7 01/18] job.c: make job_mutex and job_lock/unlock() public

2022-06-16 Thread Emanuele Giuseppe Esposito
change the nop into an actual mutex and remove the aiocontext lock. Since job_mutex is already being used, add static real_job_{lock/unlock} for the existing usage. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- include/qemu/job.h | 24

[PATCH v7 16/18] jobs: protect job.aio_context with BQL and job_mutex

2022-06-16 Thread Emanuele Giuseppe Esposito
introduce job_set_aio_context and make sure that the context is set under BQL, job_mutex and drain. Also make sure all other places where the aiocontext is read are protected. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Suggested-by: Paolo Bonzini Signed-off-by: Emanuel

[PATCH v7 00/18] job: replace AioContext lock with job_mutex

2022-06-16 Thread Emanuele Giuseppe Esposito
s * job API split patches are sent separately in another series * use of empty job_{lock/unlock} and JOB_LOCK_GUARD/WITH_JOB_LOCK_GUARD to avoid deadlocks and simplify the reviewer job * move patch 11 (block_job_query: remove atomic read) as last Emanuele Giuseppe Esposito (17): job.c: make jo

[PATCH v7 04/18] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED

2022-06-16 Thread Emanuele Giuseppe Esposito
Same as AIO_WAIT_WHILE macro, but if we are in the Main loop do not release and then acquire ctx_ 's aiocontext. Once all Aiocontext locks go away, this macro will replace AIO_WAIT_WHILE. Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- include/block/aio-wait.h | 17

[PATCH v7 12/18] block_job: rename block_job functions called with job_mutex held

2022-06-16 Thread Emanuele Giuseppe Esposito
Just as for the job API, rename block_job functions that are always called under job lock. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 3 ++- block/backup.c | 4 ++-- blockdev.c | 12 +++- blockjob.c

[PATCH v7 15/18] job: detect change of aiocontext within job coroutine

2022-06-16 Thread Emanuele Giuseppe Esposito
From: Paolo Bonzini We want to make sure access of job->aio_context is always done under either BQL or job_mutex. The problem is that using aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond makes the coroutine immediately resume, so we can't hold the job lock. And caching it

[PATCH v7 03/18] job.c: API functions not used outside should be static

2022-06-16 Thread Emanuele Giuseppe Esposito
job_event_* functions can all be static, as they are not used outside job.c. Same applies for job_txn_add_job(). Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job.h | 18 -- job.c | 22 +++--- 2 files

[PATCH v7 13/18] job.h: define unlocked functions

2022-06-16 Thread Emanuele Giuseppe Esposito
in job.c). Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 20 include/qemu/job.h | 36 +--- job.c

[PATCH v7 08/18] jobs: use job locks also in the unit tests

2022-06-16 Thread Emanuele Giuseppe Esposito
Add missing job synchronization in the unit tests, with explicit locks. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito --- tests/unit/test-bdrv-drain.c | 76 - tests/unit/test-block-iothread.c

[PATCH v7 11/18] job.h: rename job API functions called with job_mutex held

2022-06-16 Thread Emanuele Giuseppe Esposito
miss Note that "locked" refers to the *nop* job_lock/unlock, and not real_job_lock/unlock. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 2 +- block/mirror.c | 2 +- block/replication.

[PATCH] main loop: add missing documentation links to GS/IO macros

2022-06-09 Thread Emanuele Giuseppe Esposito
hat refers to such documentation. Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/main-loop.h | 18 +++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index 5518845299..c50d1b7e3a 100644 --- a/include

[PATCH 3/8] virtio_blk_process_queued_requests: always run in a bh

2022-06-09 Thread Emanuele Giuseppe Esposito
blk_set_aio_context fails, and we still need to process the requests. Since the logic of the bh is exactly the same as virtio_blk_dma_restart, so rename the function and make it public so that we can utilize it here too. Signed-off-by: Emanuele Giuseppe Esposito --- hw/block/dataplane/virtio-blk.c | 10

[PATCH 4/8] virtio: categorize callbacks in GS

2022-06-09 Thread Emanuele Giuseppe Esposito
yet. Signed-off-by: Emanuele Giuseppe Esposito --- hw/block/virtio-blk.c | 2 ++ hw/virtio/virtio-bus.c | 5 + hw/virtio/virtio-pci.c | 2 ++ hw/virtio/virtio.c | 8 4 files changed, 17 insertions(+) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 29a9c53ebc

[PATCH 2/8] block-backend: enable_write_cache should be atomic

2022-06-09 Thread Emanuele Giuseppe Esposito
It is read from IO_CODE and written with BQL held, so setting it as atomic should be enough. Also remove the aiocontext lock that was sporadically taken around the set. Signed-off-by: Emanuele Giuseppe Esposito --- block/block-backend.c | 6 +++--- hw/block/virtio-blk.c | 4 2 files

[PATCH 2/2] thread-pool: use ThreadPool from the running thread

2022-06-09 Thread Emanuele Giuseppe Esposito
Remove usage of aio_context_acquire by always submitting work items to the current thread's ThreadPool. Signed-off-by: Paolo Bonzini Signed-off-by: Emanuele Giuseppe Esposito --- block/file-posix.c| 19 +-- block/file-win32.c| 2 +- block/qcow2-threads.c | 2 +- util

[PATCH 0/8] virtio-blk: removal of AioContext lock

2022-06-09 Thread Emanuele Giuseppe Esposito
if we know that the main loop is not interfering, we are safe. However, if we want to consider multiple iothreads concurrently running, we need to take additional precautions. A good example of the above is in patch 7. Signed-off-by: Emanuele Giuseppe Esposito Emanuele Giuseppe Esposito (8

[PATCH 1/2] linux-aio: use LinuxAioState from the running thread

2022-06-09 Thread Emanuele Giuseppe Esposito
From: Paolo Bonzini Remove usage of aio_context_acquire by always submitting asynchronous AIO to the current thread's LinuxAioState. Signed-off-by: Paolo Bonzini Signed-off-by: Emanuele Giuseppe Esposito --- block/file-posix.c | 3 ++- block/linux-aio.c | 13 ++--- include/block

[PATCH 0/2] AioContext removal: LinuxAioState and ThreadPool

2022-06-09 Thread Emanuele Giuseppe Esposito
Just remove some AioContext lock in LinuxAioState and ThreadPool. Not related to anything specific, so I decided to send it as a separate patch. These patches are taken from Paolo's old draft series. Emanuele Giuseppe Esposito (1): thread-pool: use ThreadPool from the running thread Paolo

[PATCH 8/8] virtio-blk: remove unnecessary AioContext lock from function already safe

2022-06-09 Thread Emanuele Giuseppe Esposito
. Signed-off-by: Emanuele Giuseppe Esposito --- hw/block/virtio-blk.c | 18 ++ 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 88c61457e1..ce8efd8381 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c

[PATCH 5/8] virtio-blk: mark GLOBAL_STATE_CODE functions

2022-06-09 Thread Emanuele Giuseppe Esposito
Just as done in the block API, mark functions in virtio-blk that are always called in the main loop with BQL held. We know such functions are GS because they all are callbacks from virtio.c API that has already classified them as GS. Signed-off-by: Emanuele Giuseppe Esposito --- hw/block

[PATCH 6/8] virtio-blk: mark IO_CODE functions

2022-06-09 Thread Emanuele Giuseppe Esposito
, itself is categorized as IO_CODE too). Signed-off-by: Emanuele Giuseppe Esposito --- hw/block/dataplane/virtio-blk.c | 4 hw/block/virtio-blk.c | 35 + 2 files changed, 39 insertions(+) diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block

[PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock

2022-06-09 Thread Emanuele Giuseppe Esposito
scheduled (thus serialized) by the main loop. Therefore removing the AioContext lock is safe, but unfortunately we can't do it right now since bdrv_set_aio_context() and aio_wait_bh_oneshot() still need to have it. Signed-off-by: Emanuele Giuseppe Esposito --- hw/block/dataplane/virtio-blk.c | 14

[PATCH 7/8] VirtIOBlock: protect rq with its own lock

2022-06-09 Thread Emanuele Giuseppe Esposito
st. Signed-off-by: Emanuele Giuseppe Esposito --- hw/block/virtio-blk.c | 38 ++ include/hw/virtio/virtio-blk.h | 5 - 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index e1aaa606ba..88

[RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

2022-07-12 Thread Emanuele Giuseppe Esposito
re. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 30 -- block/block-backend.c | 8 ++-- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/block.c b/block.c index a7ba590dfa..101188a2d4 100644 --- a/block.c +++ b/block.c @@ -2

[RFC PATCH 5/8] block: implement .change_aio_ctx in child_of_bds

2022-07-12 Thread Emanuele Giuseppe Esposito
bdrv_child_cb_change_aio_ctx() is identical to bdrv_child_cb_can_set_aio_ctx(), as we only need to recursively go on the parent bs. Note: bdrv_child_try_change_aio_context() is not called by anyone at this point. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 9 + 1 file

[RFC PATCH 2/8] transactions: add tran_add_back

2022-07-12 Thread Emanuele Giuseppe Esposito
n invoking finalize/commit/abort. For example (A and B are lists transaction callbacks): for (i=0; i < 3; i++) { tran_add(A[i]); tran_add_tail(B[i]); } tran_commit(); Will process transactions in this order: A2 - A1 - A0 - B0 - B1 - B2 Signed-off-by: Emanuele Giuseppe Esposito

[RFC PATCH 1/8] block.c: assert bs->aio_context is written under BQL and drains

2022-07-12 Thread Emanuele Giuseppe Esposito
Also here ->aio_context is read by I/O threads and written under BQL. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block.c b/block.c index d0db104d71..267a39c0de 100644 --- a/block.c +++ b/block.c @@ -7285,6 +7285,7 @@ static v

[RFC PATCH 0/8] Refactor bdrv_try_set_aio_context using transactions

2022-07-12 Thread Emanuele Giuseppe Esposito
uses job_set_aio_context() introduced there. Suggested-by: Paolo Bonzini Based-on: <20220706201533.289775-1-eespo...@redhat.com> Emanuele Giuseppe Esposito (8): block.c: assert bs->aio_context is written under BQL and drains transactions: add tran_add_back RFC: block: use transactio

[RFC PATCH 6/8] block-backend: implement .change_aio_ctx in child_root

2022-07-12 Thread Emanuele Giuseppe Esposito
: bdrv_child_try_change_aio_context() is not called by anyone at this point. Signed-off-by: Emanuele Giuseppe Esposito --- block/block-backend.c | 54 +++ 1 file changed, 54 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index

[RFC PATCH 4/8] blockjob: implement .change_aio_ctx in child_job

2022-07-12 Thread Emanuele Giuseppe Esposito
(), but it doesn't need to invoke the recursion, as this is already taken care by child_job_change_aio_ctx(). Note: bdrv_child_try_change_aio_context() is not called by anyone at this point. Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 45 + 1 file

[RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

2022-07-12 Thread Emanuele Giuseppe Esposito
Together with all _can_set_ and _set_ APIs, as they are not needed anymore. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 196 - block/block-backend.c | 33 - blockjob.c | 35

[RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

2022-07-12 Thread Emanuele Giuseppe Esposito
() under the new one. - Because of the above, we don't need to release and re-acquire the old AioContext every time, as everything is done once (and not per-node drain and aiocontext change). Note that the "change" API is not yet invoked anywhere. Signed-off-by: Emanuele G

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

2022-07-20 Thread Emanuele Giuseppe Esposito
Am 19/07/2022 um 20:07 schrieb Paolo Bonzini: > On 7/19/22 11:57, Emanuele Giuseppe Esposito wrote: >> >> Wrapping the new drains in aio_context_acquire/release(new_context) is >> not so much helpful either, since apparently the following >> blk_set_aio_context mak

Re: [PATCH v9 09/21] jobs: use job locks also in the unit tests

2022-07-20 Thread Emanuele Giuseppe Esposito
Am 20/07/2022 um 15:06 schrieb Vladimir Sementsov-Ogievskiy: > On 7/19/22 15:00, Emanuele Giuseppe Esposito wrote: >> >> >> Am 11/07/2022 um 15:08 schrieb Vladimir Sementsov-Ogievskiy: >>> >>> That made me ask: >>> >>> 1. Are all tes

Re: [PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct

2022-07-20 Thread Emanuele Giuseppe Esposito
Am 20/07/2022 um 15:15 schrieb Vladimir Sementsov-Ogievskiy: > On 7/19/22 16:07, Emanuele Giuseppe Esposito wrote: >> >> >> Am 11/07/2022 um 16:51 schrieb Vladimir Sementsov-Ogievskiy: >>> On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >>>> iost

Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

2022-07-18 Thread Emanuele Giuseppe Esposito
Am 18/07/2022 um 10:45 schrieb Emanuele Giuseppe Esposito: > > > Am 15/07/2022 um 16:34 schrieb Hanna Reitz: >> On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: >>> Together with all _can_set_ and _set_ APIs, as they are not needed >>> anymore. >>

Re: [RFC PATCH 7/8] block: use the new _change_ API instead of _can_set_ and _set_

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 18/07/2022 um 18:39 schrieb Paolo Bonzini: > On 7/12/22 23:19, Emanuele Giuseppe Esposito wrote: >> diff --git a/block/block-backend.c b/block/block-backend.c >> index 674eaaa2bf..6e90ac3a6a 100644 >> --- a/block/block-backend.c >> +++ b/block/block-backend.c >&

Re: [PATCH v9 09/21] jobs: use job locks also in the unit tests

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 15:08 schrieb Vladimir Sementsov-Ogievskiy: > > That made me ask: > > 1. Are all tests always run in main loop? If yes, why to protect status > reading in test_complete_in_standby() ? > > 2. Maybe, we don't need to protect anything here? Why to protect other > things if we

Re: [PATCH v9 15/21] job.c: enable job lock/unlock and remove Aiocontext locks

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 16:33 schrieb Vladimir Sementsov-Ogievskiy: > On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >> Change the job_{lock/unlock} and macros to use job_mutex. >> >> Now that they are not nop anymore, remove the aiocontext >> to avoid deadlocks.

Re: [PATCH v9 11/21] jobs: group together API calls under the same job lock

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 15:26 schrieb Vladimir Sementsov-Ogievskiy: >>   } >>     static bool child_job_drained_poll(BdrvChild *c) >> @@ -111,8 +113,10 @@ static bool child_job_drained_poll(BdrvChild *c) >>   /* An inactive or completed job doesn't have any pending >> requests. Jobs >>    *

Re: [PATCH v9 17/21] blockjob.h: categorize fields in struct BlockJob

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 16:44 schrieb Vladimir Sementsov-Ogievskiy: > On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >> The same job lock is being used also to protect some of blockjob fields. >> Categorize them just as done in job.h. > > Thanks! > >> >> Signe

Re: [PATCH v9 14/21] jobs: protect job.aio_context with BQL and job_mutex

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 16:19 schrieb Vladimir Sementsov-Ogievskiy: > On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >> In order to make it thread safe, implement a "fake rwlock", >> where we allow reads under BQL *or* job_mutex held, but >> writes only under BQL

Re: [PATCH v9 00/21] job: replace AioContext lock with job_mutex

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 17:57 schrieb Vladimir Sementsov-Ogievskiy: > That seems a lot closer! > > Now I'm going to vocation from tomorrow up to the end of week, so I'll > return next Monday. > > > On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >> In thi

Re: [PATCH v9 19/21] blockjob: protect iostatus field in BlockJob struct

2022-07-19 Thread Emanuele Giuseppe Esposito
Am 11/07/2022 um 16:51 schrieb Vladimir Sementsov-Ogievskiy: > On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >> iostatus is the only field (together with .job) that needs >> protection using the job mutex. >> >> It is set in the main loop (GLOBAL_STATE functio

[PATCH v10 03/21] job.c: API functions not used outside should be static

2022-07-25 Thread Emanuele Giuseppe Esposito
job_event_* functions can all be static, as they are not used outside job.c. Same applies for job_txn_add_job(). Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/qemu/job.h | 18 -- job.c

[PATCH v10 16/21] blockjob: rename notifier callbacks as _locked

2022-07-25 Thread Emanuele Giuseppe Esposito
They all are called with job_lock held, in job_event_*_locked() Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 25 +++-- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

2022-07-25 Thread Emanuele Giuseppe Esposito
Am 20/07/2022 um 16:09 schrieb Vladimir Sementsov-Ogievskiy: > On 7/13/22 00:19, Emanuele Giuseppe Esposito wrote: >> +/* >> + * @visited will accumulate all visited BdrvChild object. The caller is >> + * responsible for freeing the list afterwards. >> + */ >&

[PATCH v10 13/21] job: detect change of aiocontext within job coroutine

2022-07-25 Thread Emanuele Giuseppe Esposito
From: Paolo Bonzini We want to make sure access of job->aio_context is always done under either BQL or job_mutex. The problem is that using aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond makes the coroutine immediately resume, so we can't hold the job lock. And caching it

[PATCH v10 15/21] blockjob.h: categorize fields in struct BlockJob

2022-07-25 Thread Emanuele Giuseppe Esposito
The same job lock is being used also to protect some of blockjob fields. Categorize them just as done in job.h. Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Emanuele Giuseppe Esposito --- include/block/blockjob.h | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions

[PATCH v10 12/21] commit and mirror: create new nodes using bdrv_get_aio_context, and not the job aiocontext

2022-07-25 Thread Emanuele Giuseppe Esposito
Giuseppe Esposito --- block/commit.c | 4 ++-- block/mirror.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/block/commit.c b/block/commit.c index 851d1c557a..336f799172 100644 --- a/block/commit.c +++ b/block/commit.c @@ -370,7 +370,7 @@ void commit_start(const char

[PATCH v10 21/21] job: remove unused functions

2022-07-25 Thread Emanuele Giuseppe Esposito
These public functions are not used anywhere, thus can be dropped. Also, since this is the final job API that doesn't use AioContext lock and replaces it with job_lock, adjust all remaining function documentation to clearly specify if the job lock is taken or not. Signed-off-by: Emanuele Giuseppe

[PATCH v10 02/21] job.h: categorize fields in struct Job

2022-07-25 Thread Emanuele Giuseppe Esposito
Categorize the fields in struct Job to understand which ones need to be protected by the job mutex and which don't. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/qemu/job.h | 61 +++--- 1 file changed, 36

[PATCH v10 07/21] blockjob: introduce block_job _locked() APIs

2022-07-25 Thread Emanuele Giuseppe Esposito
-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 52 include/block/blockjob.h | 18 ++ 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/blockjob.c b/blockjob.c index 7da59a1f1c

[PATCH v10 01/21] job.c: make job_mutex and job_lock/unlock() public

2022-07-25 Thread Emanuele Giuseppe Esposito
change the nop into an actual mutex and remove the aiocontext lock. Since job_mutex is already being used, add static real_job_{lock/unlock} for the existing usage. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov-Ogievskiy --- include

[PATCH v10 08/21] jobs: add job lock in find_* functions

2022-07-25 Thread Emanuele Giuseppe Esposito
*. Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- blockdev.c | 67 +- job-qmp.c | 57 -- 2 files changed, 86 insertions(+), 38 deletions

[PATCH v10 11/21] jobs: group together API calls under the same job lock

2022-07-25 Thread Emanuele Giuseppe Esposito
-by: Emanuele Giuseppe Esposito --- block.c| 17 ++--- blockdev.c | 12 +--- blockjob.c | 35 ++- job-qmp.c | 4 +++- job.c | 7 +-- monitor/qmp-cmds.c | 7 +-- qemu-img.c | 37

[PATCH v10 09/21] jobs: use job locks also in the unit tests

2022-07-25 Thread Emanuele Giuseppe Esposito
job_pause() is/will be only used in tests to avoid: WITH_JOB_LOCK_GUARD(){ job_pause_locked(); } then it is not worth keeping job_pause(), and just use the guard. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito Reviewed

[PATCH v10 04/21] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED

2022-07-25 Thread Emanuele Giuseppe Esposito
Same as AIO_WAIT_WHILE macro, but if we are in the Main loop do not release and then acquire ctx_ 's aiocontext. Once all Aiocontext locks go away, this macro will replace AIO_WAIT_WHILE. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi Reviewed-by: Vladimir Sementsov

[PATCH v10 14/21] jobs: protect job.aio_context with BQL and job_mutex

2022-07-25 Thread Emanuele Giuseppe Esposito
introduce job_set_aio_context and make sure that the context is set under BQL, job_mutex and drain. Also make sure all other places where the aiocontext is read are protected. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Suggested-by: Paolo Bonzini Signed-off-by: Emanuel

[PATCH v10 10/21] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU

2022-07-25 Thread Emanuele Giuseppe Esposito
-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito --- block/mirror.c | 19 ++- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index d8ecb9efa2..b38676e19d 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -654,9 +654,13

[PATCH v10 06/21] job: move and update comments from blockjob.c

2022-07-25 Thread Emanuele Giuseppe Esposito
Giuseppe Esposito --- blockjob.c | 20 job.c | 14 ++ 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/blockjob.c b/blockjob.c index 4868453d74..7da59a1f1c 100644 --- a/blockjob.c +++ b/blockjob.c @@ -36,21 +36,6 @@ #include "qemu/main-l

[PATCH v10 05/21] job.c: add job_lock/unlock while keeping job.h intact

2022-07-25 Thread Emanuele Giuseppe Esposito
lock guard macros are *nop* Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job.h | 138 ++- job.c | 600 +++-- 2 files changed, 553 insertions(+), 185 deletions(-) diff --git a/in

[PATCH v10 18/21] job.c: enable job lock/unlock and remove Aiocontext locks

2022-07-25 Thread Emanuele Giuseppe Esposito
der aiocontext lock. Also remove real_job_{lock/unlock}, as they are replaced by the public functions. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockdev.c | 74 +--- include/qemu/job.h | 22 -

[PATCH v10 00/21] job: replace AioContext lock with job_mutex

2022-07-25 Thread Emanuele Giuseppe Esposito
RD to avoid deadlocks and simplify the reviewer job * move patch 11 (block_job_query: remove atomic read) as last Emanuele Giuseppe Esposito (20): job.c: make job_mutex and job_lock/unlock() public job.h: categorize fields in struct Job job.c: API functions not used outside should be static aio-w

[PATCH v10 20/21] blockjob: remove unused functions

2022-07-25 Thread Emanuele Giuseppe Esposito
These public functions are not used anywhere, thus can be dropped. Signed-off-by: Emanuele Giuseppe Esposito Reviewed-by: Stefan Hajnoczi --- blockjob.c | 16 ++-- include/block/blockjob.h | 31 --- 2 files changed, 14 insertions(+), 33

[PATCH v10 17/21] blockjob: protect iostatus field in BlockJob struct

2022-07-25 Thread Emanuele Giuseppe Esposito
called under job lock. Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/blockjob.c b/blockjob.c index 0663faee2c..448bdb5a53 100644 --- a/blockjob.c +++ b/blockjob.c @@ -363,7 +363,8 @@ BlockJobInfo *block_job_query

[PATCH v10 19/21] block_job_query: remove atomic read

2022-07-25 Thread Emanuele Giuseppe Esposito
Not sure what the atomic here was supposed to do, since job.busy is protected by the job lock. Since the whole function is called under job_mutex, just remove the atomic. Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Stefan Hajnoczi Signed-off-by: Emanuele Giuseppe Esposito

[PATCH v2 08/11] block: use the new _change_ API instead of _can_set_ and _set_

2022-07-25 Thread Emanuele Giuseppe Esposito
re. Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 44 --- block/block-backend.c | 8 ++-- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/block.c b/block.c index bcc9b0d099..9b47aacad2 100644 --- a/block.

[PATCH v2 11/11] block: remove bdrv_try_set_aio_context and replace it with bdrv_try_change_aio_context

2022-07-25 Thread Emanuele Giuseppe Esposito
No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 14 -- block/export/export.c | 2 +- blockdev.c | 22 +++--- docs/devel/multiple-iothreads.txt | 4 ++-- include

[PATCH v2 05/11] blockjob: implement .change_aio_ctx in child_job

2022-07-25 Thread Emanuele Giuseppe Esposito
(), but it doesn't need to invoke the recursion, as this is already taken care by child_job_change_aio_ctx(). Note: bdrv_child_try_change_aio_context() is not called by anyone at this point. Reviewed-by: Hanna Reitz Signed-off-by: Emanuele Giuseppe Esposito --- blockjob.c | 45

[PATCH v2 04/11] bdrv_child_try_change_aio_context: add transaction parameter

2022-07-25 Thread Emanuele Giuseppe Esposito
This enables the caller to use the same transaction to also keep track of aiocontext changes. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 31 -- include/block/block-global-state.h | 5 + 2 files changed, 30 insertions(+), 6

[PATCH v2 10/11] block: rename bdrv_child_try_change_aio_context in bdrv_try_change_aio_context

2022-07-25 Thread Emanuele Giuseppe Esposito
No functional changes intended. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 19 --- block/block-backend.c | 3 +-- include/block/block-global-state.h | 12 +--- 3 files changed, 14 insertions(+), 20 deletions(-) diff

[PATCH v2 03/11] bdrv_change_aio_context: use hash table instead of list of visited nodes

2022-07-25 Thread Emanuele Giuseppe Esposito
pairs. Suggested-by: Hanna Reitz Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 28 include/block/block-global-state.h | 2 +- include/block/block_int-common.h | 3 ++- 3 files changed, 19 insertions(+), 14 deletions(-) diff

[PATCH v2 02/11] block: use transactions as a replacement of ->{can_}set_aio_context()

2022-07-25 Thread Emanuele Giuseppe Esposito
uot;change" API is not yet invoked anywhere. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 203 - include/block/block-global-state.h | 6 + include/block/block_int-common.h | 3 + 3 files changed, 211 insertions(+), 1 del

[PATCH v2 01/11] block.c: assert bs->aio_context is written under BQL and drains

2022-07-25 Thread Emanuele Giuseppe Esposito
Also here ->aio_context is read by I/O threads and written under BQL. Reviewed-by: Hanna Reitz Signed-off-by: Emanuele Giuseppe Esposito --- block.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block.c b/block.c index 7559965dbc..58a9cfc8b7 100644 --- a/block.c +++ b/bloc

[PATCH v2 07/11] block-backend: implement .change_aio_ctx in child_root

2022-07-25 Thread Emanuele Giuseppe Esposito
: bdrv_child_try_change_aio_context() is not called by anyone at this point. Signed-off-by: Emanuele Giuseppe Esposito --- block/block-backend.c | 52 +++ 1 file changed, 52 insertions(+) diff --git a/block/block-backend.c b/block/block-backend.c index

[PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

2022-07-25 Thread Emanuele Giuseppe Esposito
nups in block-backend callbacks * use hash map instead of glist when marking visited nodes * 2 more additional patches, getting rid of bdrv_try_set_aio_context and bdrv_child_try_change_aio_context Emanuele Giuseppe Esposito (11): block.c: assert bs->aio_context is written under BQL an

[PATCH v2 06/11] block: implement .change_aio_ctx in child_of_bds

2022-07-25 Thread Emanuele Giuseppe Esposito
bdrv_child_cb_change_aio_ctx() is identical to bdrv_child_cb_can_set_aio_ctx(), as we only need to recursively go on the parent bs. Note: bdrv_child_try_change_aio_context() is not called by anyone at this point. Reviewed-by: Hanna Reitz Signed-off-by: Emanuele Giuseppe Esposito --- block.c

[PATCH v2 09/11] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

2022-07-25 Thread Emanuele Giuseppe Esposito
Together with all _can_set_ and _set_ APIs, as they are not needed anymore. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 196 - block/block-backend.c | 33 - blockjob.c | 35

Re: [PATCH v2 00/11] Refactor bdrv_try_set_aio_context using transactions

2022-07-26 Thread Emanuele Giuseppe Esposito
Am 26/07/2022 um 16:24 schrieb Vladimir Sementsov-Ogievskiy: > On 7/25/22 15:21, Emanuele Giuseppe Esposito wrote: >> The aim of this series is to reorganize bdrv_try_set_aio_context >> and drop BDS ->set_aio_context and ->can_set_aio_ctx callbacks in >> favour of

Re: [RFC PATCH 8/8] block: remove all unused ->can_set_aio_ctx and ->set_aio_ctx callbacks

2022-07-18 Thread Emanuele Giuseppe Esposito
Am 15/07/2022 um 16:34 schrieb Hanna Reitz: > On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: >> Together with all _can_set_ and _set_ APIs, as they are not needed >> anymore. >> >> Signed-off-by: Emanuele Giuseppe Esposito >> --- >>   b

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

2022-07-18 Thread Emanuele Giuseppe Esposito
Am 14/07/2022 um 18:45 schrieb Hanna Reitz: > On 12.07.22 23:19, Emanuele Giuseppe Esposito wrote: >> - >> RFC because I am not sure about the AioContext locks. >> - Do we need to take the new AioContext lock? what does it protect? >> - Taking the old AioC

Re: [RFC PATCH 3/8] RFC: block: use transactions as a replacement of ->{can_}set_aio_context()

2022-07-18 Thread Emanuele Giuseppe Esposito
Am 14/07/2022 um 18:45 schrieb Hanna Reitz: >> + * First, go recursively through all nodes in the graph, and see if they >> + * can change their AioContext. >> + * If so, add for each node a new transaction with a callback to >> change the >> + * AioContext with the new one. >> + * Once

Re: [PATCH v7 10/18] jobs: rename static functions called with job_mutex held

2022-06-28 Thread Emanuele Giuseppe Esposito
Am 28/06/2022 um 17:26 schrieb Vladimir Sementsov-Ogievskiy: > On 6/28/22 18:22, Vladimir Sementsov-Ogievskiy wrote: >> On 6/28/22 16:04, Emanuele Giuseppe Esposito wrote: >>>>> Ok so far I did the following: >>>>> >>>>> - dupli

Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

2022-05-02 Thread Emanuele Giuseppe Esposito
Am 30/04/2022 um 07:17 schrieb Stefan Hajnoczi: > On Thu, Apr 28, 2022 at 11:56:09PM +0200, Emanuele Giuseppe Esposito wrote: >> >> >> Am 28/04/2022 um 12:45 schrieb Stefan Hajnoczi: >>> On Wed, Apr 27, 2022 at 08:55:35AM +0200, Emanuele Giuseppe Esposito wrote: &

Re: [RFC PATCH v2 3/8] block: introduce a lock to protect graph operations

2022-05-02 Thread Emanuele Giuseppe Esposito
Am 30/04/2022 um 07:48 schrieb Stefan Hajnoczi: > On Fri, Apr 29, 2022 at 10:37:54AM +0200, Emanuele Giuseppe Esposito wrote: >> Am 28/04/2022 um 15:45 schrieb Stefan Hajnoczi: >>> On Tue, Apr 26, 2022 at 04:51:09AM -0400, Emanuele Giuseppe Esposito wrote: >>&

Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

2022-04-28 Thread Emanuele Giuseppe Esposito
Am 28/04/2022 um 12:45 schrieb Stefan Hajnoczi: > On Wed, Apr 27, 2022 at 08:55:35AM +0200, Emanuele Giuseppe Esposito wrote: >> >> >> Am 26/04/2022 um 10:51 schrieb Emanuele Giuseppe Esposito: >>> Luckly, most of the cases where we recursively go through a grap

Re: [RFC PATCH v2 2/8] coroutine-lock: release lock when restarting all coroutines

2022-04-29 Thread Emanuele Giuseppe Esposito
Am 29/04/2022 um 00:14 schrieb Paolo Bonzini: > On 4/28/22 13:21, Stefan Hajnoczi wrote: >> It's unclear whether this patch fixes a bug or introduces a new API that >> will be used in later patches. >> >> The commit message is a bit misleading: existing functions are not >> changed to release

Re: [RFC PATCH v2 0/8] Removal of AioContext lock, bs->parents and ->children: new rwlock

2022-04-29 Thread Emanuele Giuseppe Esposito
Am 28/04/2022 um 12:34 schrieb Stefan Hajnoczi: > On Tue, Apr 26, 2022 at 04:51:06AM -0400, Emanuele Giuseppe Esposito wrote: >> Next step is the most complex one: figure where to put the rdlocks. > > I got a little lost at this step but will hopefully understand more

Re: [RFC PATCH v2 1/8] aio_wait_kick: add missing memory barrier

2022-04-29 Thread Emanuele Giuseppe Esposito
Am 28/04/2022 um 13:09 schrieb Stefan Hajnoczi: > On Tue, Apr 26, 2022 at 04:51:07AM -0400, Emanuele Giuseppe Esposito wrote: >> It seems that aio_wait_kick always required a memory barrier >> or atomic operation in the caller, but almost nobody actually >> took care of

Re: [RFC PATCH v2 4/8] async: register/unregister aiocontext in graph lock list

2022-04-29 Thread Emanuele Giuseppe Esposito
Am 29/04/2022 um 00:19 schrieb Paolo Bonzini: > On 4/28/22 15:46, Stefan Hajnoczi wrote: >>>     if have_block >>>     util_ss.add(files('aiocb.c', 'async.c', 'aio-wait.c')) >>> +  util_ss.add(files('../block/graph-lock.c')) >> Why is it in block/ if it needs to be built into libqemuutil? >

<    6   7   8   9   10   11   12   13   14   15   >