| Message ID | 20211104145334.1346363-12-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: > Not sure what the atomic here was supposed to do, since job.busy > is protected by the job lock. In block_job_query() it is protected only since previous commit. So, before previous commit, atomic read make sense. Hmm. but job_lock() is still a no-op at this point. So, actually, it would be more correct to drop this qatomic_read after patch 14. > Since the whole function will > be called under job_mutex, just remove the atomic. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > blockjob.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/blockjob.c b/blockjob.c > index dcc13dc336..426dcddcc1 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -314,6 +314,7 @@ int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) > return ratelimit_calculate_delay(&job->limit, n); > } > > +/* Called with job_mutex held */ > BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > { > BlockJobInfo *info; > @@ -332,13 +333,13 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > info = g_new0(BlockJobInfo, 1); > info->type = g_strdup(job_type_str(&job->job)); > info->device = g_strdup(job->job.id); > - info->busy = qatomic_read(&job->job.busy); > + info->busy = job->job.busy; > info->paused = job->job.pause_count > 0; > info->offset = progress_current; > info->len = progress_total; > info->speed = job->speed; > info->io_status = job->iostatus; > - info->ready = job_is_ready(&job->job), > + info->ready = job_is_ready_locked(&job->job), > info->status = job->job.status; > info->auto_finalize = job->job.auto_finalize; > info->auto_dismiss = job->job.auto_dismiss; >
On 18/12/2021 13:07, Vladimir Sementsov-Ogievskiy wrote: > 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: >> Not sure what the atomic here was supposed to do, since job.busy >> is protected by the job lock. > > In block_job_query() it is protected only since previous commit. So, > before previous commit, atomic read make sense. To me it doesn't really, because it is protected with job_lock/unlock in job.c, and here is read with an atomic. But maybe I am missing something. > Hmm. but job_lock() is still a no-op at this point. So, actually, it > would be more correct to drop this qatomic_read after patch 14. > Will do. Thank you, Emanuele
diff --git a/blockjob.c b/blockjob.c index dcc13dc336..426dcddcc1 100644 --- a/blockjob.c +++ b/blockjob.c @@ -314,6 +314,7 @@ int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) return ratelimit_calculate_delay(&job->limit, n); } +/* Called with job_mutex held */ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) { BlockJobInfo *info; @@ -332,13 +333,13 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) info = g_new0(BlockJobInfo, 1); info->type = g_strdup(job_type_str(&job->job)); info->device = g_strdup(job->job.id); - info->busy = qatomic_read(&job->job.busy); + info->busy = job->job.busy; info->paused = job->job.pause_count > 0; info->offset = progress_current; info->len = progress_total; info->speed = job->speed; info->io_status = job->iostatus; - info->ready = job_is_ready(&job->job), + info->ready = job_is_ready_locked(&job->job), info->status = job->job.status; info->auto_finalize = job->job.auto_finalize; info->auto_dismiss = job->job.auto_dismiss;
Not sure what the atomic here was supposed to do, since job.busy is protected by the job lock. Since the whole function will be called under job_mutex, just remove the atomic. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- blockjob.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)