On 1/5/22 15:01, Emanuele Giuseppe Esposito wrote:
> In this series, we want to remove the AioContext lock and instead
> use the already existent job_mutex to protect the job structures
> and list. This is part of the work to get rid of AioContext lock
> usage in favour of smaller granularity locks.
>
> In order to simplify reviewer's job, job lock/unlock functions and
> macros are added as empty prototypes (nop) in patch 1.
> They are converted to use the actual job mutex only in the last
> patch, 14. In this way we can freely create locking sections
> without worrying about deadlocks with the aiocontext lock.
Oops, sorry -- I missed this explanation when first reading the cover
letter. Good job, though it needs another iteration; especially for
patch 14, and possibly to decide the right placement of patches 10-13.
Thanks,
Paolo
> Patch 2 defines what fields in the job structure need protection,
> and patches 3-4 categorize respectively locked and unlocked
> functions in the job API.
>
> Patch 5-9 are in preparation to the job locks, they try to reduce
> the aiocontext critical sections and other minor fixes.
>
> Patch 10-13 introduces the (nop) job lock into the job API and
> its users, following the comments and categorizations done in
> patch 2-3-4.
>
> Patch 14 makes the prototypes in patch 1 use the job_mutex and
> removes all aiocontext lock at the same time.
>
> Tested this series by running unit tests, qemu-iotests and qtests
> (x86_64).
>
> This serie is based on my previous series "block layer: split
> block APIs in global state and I/O".
>
> Based-on: <20211124064418.3120601-1-eesposit@redhat.com>
> ---
> v3:
> * add "_locked" suffix to the functions called under job_mutex lock
> * rename _job_lock in real_job_lock
> * job_mutex is now public, and drivers like monitor use it directly
> * introduce and protect job_get_aio_context
> * remove mirror-specific APIs and just use WITH_JOB_GUARD
> * more extensive use of WITH_JOB_GUARD and JOB_LOCK_GUARD
>
> RFC v2:
> * use JOB_LOCK_GUARD and WITH_JOB_LOCK_GUARD
> * mu(u)ltiple typos in commit messages
> * 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 (16):
> job.c: make job_mutex and job_lock/unlock() public
> job.h: categorize fields in struct Job
> job.h: define locked functions
> job.h: define unlocked functions
> block/mirror.c: use of job helpers in drivers to avoid TOC/TOU
> job.c: make job_event_* functions static
> job.c: move inner aiocontext lock in callbacks
> aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
> jobs: remove aiocontext locks since the functions are under BQL
> jobs: protect jobs with job_lock/unlock
> jobs: document all static functions and add _locked() suffix
> jobs: use job locks and helpers also in the unit tests
> jobs: add job lock in find_* functions
> job.c: use job_get_aio_context()
> job.c: enable job lock/unlock and remove Aiocontext locks
> block_job_query: remove atomic read
>
> block.c | 18 +-
> block/commit.c | 4 +-
> block/mirror.c | 21 +-
> block/replication.c | 10 +-
> blockdev.c | 112 ++----
> blockjob.c | 122 +++---
> include/block/aio-wait.h | 15 +-
> include/qemu/job.h | 317 +++++++++++----
> job-qmp.c | 74 ++--
> job.c | 656 +++++++++++++++++++------------
> monitor/qmp-cmds.c | 6 +-
> qemu-img.c | 41 +-
> tests/unit/test-bdrv-drain.c | 46 +--
> tests/unit/test-block-iothread.c | 14 +-
> tests/unit/test-blockjob-txn.c | 24 +-
> tests/unit/test-blockjob.c | 98 ++---
> 16 files changed, 947 insertions(+), 631 deletions(-)
>