| Message ID | 20211029163914.4044794-12-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
Hmm. subject looks like it's safe to remove aiocontext locks from any qmp command? For example, commit 91005a495e228eb added aiocontext locks back to qmp bitmap add/remove commands because otherwise they crashed. So, I'm not sure that being under BQL is enough to drop aiocontext locking.. 29.10.2021 19:39, 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. > > 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 d9bf842a81..67b55eec11 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3719,15 +3719,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 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; >
On 02/11/2021 13:41, Vladimir Sementsov-Ogievskiy wrote: > Hmm. subject looks like it's safe to remove aiocontext locks from any > qmp command? > > For example, commit 91005a495e228eb added aiocontext locks back to qmp > bitmap add/remove commands because otherwise they crashed. > > So, I'm not sure that being under BQL is enough to drop aiocontext > locking.. > > In this specific case the aiocontext is useless there. I am not sure what it was used for before, but if you look at what it protects in this patch we have: - block_job_query: not called by anyone else, and internally calls: * block_job_is_internal = checks struct job id, that is only written at job initialization * progress_get_snapshot = we made it thread safe, as you might remember * accesses struct job fields, that we protect anyways with job_mutex. Anyways the aiocontext is not used to protect jobs fields. - job_query_single: same as block_job_query, calls progress_get_snapshot and accesses job fields. So yes, I am positive that here removing the aiocontext lock does not break anything. Thank you, Emanuele
diff --git a/blockdev.c b/blockdev.c index d9bf842a81..67b55eec11 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3719,15 +3719,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 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 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(-)