| Message ID | 20220208143513.1077229-7-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
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 <eesposit@redhat.com> > --- > 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.
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 <eesposit@redhat.com> >> --- >> 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. >
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..a6774aaaa5 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;
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 <eesposit@redhat.com> --- blockdev.c | 4 ---- job-qmp.c | 4 ---- 2 files changed, 8 deletions(-)