| Message ID | 20220105140208.365608-10-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
On 1/5/22 15:02, 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 can only be *wrapped by* > the aiocontext lock, and not vice-versa, to avoid deadlocks. Better to avoid the passive: "must be taken inside the AioContext lock, and taking it outside would cause deadlocks". Also add a note about the lock hierarchy to patch 1. > @@ -3707,15 +3707,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; > } block_job_next, block_job_query, etc. do not have the _locked suffix. Is this because all block_job_ functions need the job_mutex held, or just laziness? :) Paolo > - aio_context = blk_get_aio_context(job->blk); > - 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 de4120a1d4..f6f9840436 100644 > --- a/job-qmp.c > +++ b/job-qmp.c > @@ -173,15 +173,11 @@ JobInfoList *qmp_query_jobs(Error **errp) > > for (job = job_next_locked(NULL); job; job = job_next_locked(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;
On 19/01/2022 12:09, Paolo Bonzini wrote: >> @@ -3707,15 +3707,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; >> } > > block_job_next, block_job_query, etc. do not have the _locked suffix. Is > this because all block_job_ functions need the job_mutex held, or just > laziness? :) > I wasn't really sure whether to touch that API naming or not (+ laziness :D ) But it makes sense to add _locked also there. Will do. Thank you, Emanuele
diff --git a/blockdev.c b/blockdev.c index 11fd651bde..ee35aff13a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3707,15 +3707,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 = blk_get_aio_context(job->blk); - 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 de4120a1d4..f6f9840436 100644 --- a/job-qmp.c +++ b/job-qmp.c @@ -173,15 +173,11 @@ JobInfoList *qmp_query_jobs(Error **errp) for (job = job_next_locked(NULL); job; job = job_next_locked(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;
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 can only be *wrapped by* the aiocontext lock, and not vice-versa, to avoid deadlocks. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- blockdev.c | 4 ---- job-qmp.c | 4 ---- 2 files changed, 8 deletions(-)