[PATCH v5 06/20] jobs: remove aiocontext locks since the functions are under BQL
In preparation to the job_lock/unlock patch, remove these
aiocontext locks.
The main reason these two locks are removed here is because
they are inside a loop iterating on the jobs list. Once the
job_lock is added, it will have to protect the whole loop,
wrapping also the aiocontext acquire/release.
We don't want this, as job_lock must be taken inside the AioContext
lock, and taking it outside would cause deadlocks.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockdev.c | 4
job-qmp.c | 4
2 files changed, 8 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 8cac3d739c..e315466914 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3713,15 +3713,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
BlockJobInfo *value;
-AioContext *aio_context;
if (block_job_is_internal(job)) {
continue;
}
-aio_context = block_job_get_aio_context(job);
-aio_context_acquire(aio_context);
value = block_job_query(job, errp);
-aio_context_release(aio_context);
if (!value) {
qapi_free_BlockJobInfoList(head);
return NULL;
diff --git a/job-qmp.c b/job-qmp.c
index 829a28aa70..a67745 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -173,15 +173,11 @@ JobInfoList *qmp_query_jobs(Error **errp)
for (job = job_next(NULL); job; job = job_next(job)) {
JobInfo *value;
-AioContext *aio_context;
if (job_is_internal(job)) {
continue;
}
-aio_context = job->aio_context;
-aio_context_acquire(aio_context);
value = job_query_single(job, errp);
-aio_context_release(aio_context);
if (!value) {
qapi_free_JobInfoList(head);
return NULL;
--
2.31.1
Re: [PATCH v5 06/20] jobs: remove aiocontext locks since the functions are under BQL
On Tue, Feb 08, 2022 at 09:34:59AM -0500, Emanuele Giuseppe Esposito wrote:
> In preparation to the job_lock/unlock patch, remove these
> aiocontext locks.
> The main reason these two locks are removed here is because
> they are inside a loop iterating on the jobs list. Once the
> job_lock is added, it will have to protect the whole loop,
> wrapping also the aiocontext acquire/release.
>
> We don't want this, as job_lock must be taken inside the AioContext
> lock, and taking it outside would cause deadlocks.
>
> Signed-off-by: Emanuele Giuseppe Esposito
> ---
> blockdev.c | 4
> job-qmp.c | 4
> 2 files changed, 8 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 8cac3d739c..e315466914 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3713,15 +3713,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>
> for (job = block_job_next(NULL); job; job = block_job_next(job)) {
I'm confused. block_job_next() is supposed to be called with job_mutex
held since it iterates the jobs list.
The patch series might fix this later on but it's hard to review patches
with broken invariants.
Does this mean git-bisect(1) is broken since intermediate commits are
not thread-safe?
> BlockJobInfo *value;
> -AioContext *aio_context;
>
> if (block_job_is_internal(job)) {
> continue;
> }
> -aio_context = block_job_get_aio_context(job);
> -aio_context_acquire(aio_context);
> value = block_job_query(job, errp);
This function accesses fields that are protected by job_mutex, which we
don't hold.
signature.asc
Description: PGP signature
Re: [PATCH v5 06/20] jobs: remove aiocontext locks since the functions are under BQL
On 17/02/2022 15:12, Stefan Hajnoczi wrote:
> On Tue, Feb 08, 2022 at 09:34:59AM -0500, Emanuele Giuseppe Esposito wrote:
>> In preparation to the job_lock/unlock patch, remove these
>> aiocontext locks.
>> The main reason these two locks are removed here is because
>> they are inside a loop iterating on the jobs list. Once the
>> job_lock is added, it will have to protect the whole loop,
>> wrapping also the aiocontext acquire/release.
>>
>> We don't want this, as job_lock must be taken inside the AioContext
>> lock, and taking it outside would cause deadlocks.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito
>> ---
>> blockdev.c | 4
>> job-qmp.c | 4
>> 2 files changed, 8 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 8cac3d739c..e315466914 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3713,15 +3713,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>>
>> for (job = block_job_next(NULL); job; job = block_job_next(job)) {
>
> I'm confused. block_job_next() is supposed to be called with job_mutex
> held since it iterates the jobs list.
Ok here it is clear that the lock is taken to protect the list. So I
should squash this patch with the one that enables job lock/unlock
(patch 19).
Emanuele
>
> The patch series might fix this later on but it's hard to review patches
> with broken invariants.
>
> Does this mean git-bisect(1) is broken since intermediate commits are
> not thread-safe?
>
>> BlockJobInfo *value;
>> -AioContext *aio_context;
>>
>> if (block_job_is_internal(job)) {
>> continue;
>> }
>> -aio_context = block_job_get_aio_context(job);
>> -aio_context_acquire(aio_context);
>> value = block_job_query(job, errp);
>
> This function accesses fields that are protected by job_mutex, which we
> don't hold.
>
