| Message ID | 20220629141538.3400679-9-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
On Wed, Jun 29, 2022 at 10:15:26AM -0400, Emanuele Giuseppe Esposito wrote: > +BlockJob *block_job_next(BlockJob *bjob) > { > - Job *job = job_get(id); > + JOB_LOCK_GUARD(); > + return block_job_next_locked(bjob); > +} This seems unsafe for the same reason as job_ref(). How can the caller be sure bjob is still valid if it doesn't hold the mutex and has no reference to it? Maybe the assumption is that the next()/get()/unref() APIs are GLOBAL_STATE_CODE(), so there can be no race between them?
Am 05/07/2022 um 09:58 schrieb Stefan Hajnoczi: > On Wed, Jun 29, 2022 at 10:15:26AM -0400, Emanuele Giuseppe Esposito wrote: >> +BlockJob *block_job_next(BlockJob *bjob) >> { >> - Job *job = job_get(id); >> + JOB_LOCK_GUARD(); >> + return block_job_next_locked(bjob); >> +} > > This seems unsafe for the same reason as job_ref(). How can the caller > be sure bjob is still valid if it doesn't hold the mutex and has no > reference to it? > > Maybe the assumption is that the next()/get()/unref() APIs are > GLOBAL_STATE_CODE(), so there can be no race between them? > Same answer as job_ref. Unfortunately if we want to keep this logic in this serie that's the price to pay (even though it's just till patch 13). No assumption I would say. Emanuele
On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote: > Just as done with job.h, create _locked() functions in blockjob.h We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject. Also, we start to introduce _locked block_job_* APIs. Does it mean that BlockJob and Job share the global mutex to protect themselves? Than I think we should document in BlockJob struct what is protected by job_mutex. And please, let's be consistent on whether we add or not add "with mutex held" / "with mutex not held" comments. For job API you mostly add it for each function.. Let's do same here? Same for "temporary unlock" comments. > > These functions will be later useful when caller has already taken > the lock. All blockjob _locked functions call job _locked functions. > > Note: at this stage, job_{lock/unlock} and job lock guard macros > are *nop*. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > blockjob.c | 52 ++++++++++++++++++++++++++++++++-------- > include/block/blockjob.h | 15 ++++++++++++ > 2 files changed, 57 insertions(+), 10 deletions(-) > > diff --git a/blockjob.c b/blockjob.c > index 7da59a1f1c..0d59aba439 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -44,21 +44,27 @@ static bool is_block_job(Job *job) > job_type(job) == JOB_TYPE_STREAM; > } > > -BlockJob *block_job_next(BlockJob *bjob) > +BlockJob *block_job_next_locked(BlockJob *bjob) > { > Job *job = bjob ? &bjob->job : NULL; > GLOBAL_STATE_CODE(); > > do { > - job = job_next(job); > + job = job_next_locked(job); > } while (job && !is_block_job(job)); > > return job ? container_of(job, BlockJob, job) : NULL; > } > > -BlockJob *block_job_get(const char *id) > +BlockJob *block_job_next(BlockJob *bjob) > { > - Job *job = job_get(id); > + JOB_LOCK_GUARD(); > + return block_job_next_locked(bjob); > +} > + > +BlockJob *block_job_get_locked(const char *id) > +{ > + Job *job = job_get_locked(id); > GLOBAL_STATE_CODE(); > > if (job && is_block_job(job)) { > @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id) > } > } > > +BlockJob *block_job_get(const char *id) > +{ > + JOB_LOCK_GUARD(); > + return block_job_get_locked(id); > +} > + > void block_job_free(Job *job) > { > BlockJob *bjob = container_of(job, BlockJob, job); > @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job) > return timer_pending(&job->sleep_timer); > } > > -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) > +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp) > { > const BlockJobDriver *drv = block_job_driver(job); > int64_t old_speed = job->speed; > > GLOBAL_STATE_CODE(); > > - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { > + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { > return false; > } > if (speed < 0) { > @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) > job->speed = speed; > > if (drv->set_speed) { > + job_unlock(); > drv->set_speed(job, speed); > + job_lock(); > } > > if (speed && speed <= old_speed) { > @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) > } > > /* kick only if a timer is pending */ > - job_enter_cond(&job->job, job_timer_pending); > + job_enter_cond_locked(&job->job, job_timer_pending); > > return true; > } > > +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) > +{ > + JOB_LOCK_GUARD(); > + return block_job_set_speed_locked(job, speed, errp); > +} > + > int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) > { > IO_CODE(); > return ratelimit_calculate_delay(&job->limit, n); > } > > -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) > { > BlockJobInfo *info; > uint64_t progress_current, progress_total; > @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > 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; > @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > return info; > } > > +BlockJobInfo *block_job_query(BlockJob *job, Error **errp) > +{ > + JOB_LOCK_GUARD(); > + return block_job_query_locked(job, errp); > +} > + > static void block_job_iostatus_set_err(BlockJob *job, int error) > { > if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { > @@ -478,7 +504,7 @@ fail: > return NULL; > } > > -void block_job_iostatus_reset(BlockJob *job) > +void block_job_iostatus_reset_locked(BlockJob *job) > { > GLOBAL_STATE_CODE(); > if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { > @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job) > job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; > } > > +void block_job_iostatus_reset(BlockJob *job) > +{ > + JOB_LOCK_GUARD(); > + block_job_iostatus_reset_locked(job); > +} > + > void block_job_user_resume(Job *job) > { > BlockJob *bjob = container_of(job, BlockJob, job); > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index 6525e16fd5..3959a98612 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -92,6 +92,9 @@ typedef struct BlockJob { > */ > BlockJob *block_job_next(BlockJob *job); > > +/* Same as block_job_next(), but called with job lock held. */ > +BlockJob *block_job_next_locked(BlockJob *job); > + > /** > * block_job_get: > * @id: The id of the block job. > @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job); > */ > BlockJob *block_job_get(const char *id); > > +/* Same as block_job_get(), but called with job lock held. */ > +BlockJob *block_job_get_locked(const char *id); > + > /** > * block_job_add_bdrv: > * @job: A block job > @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); > */ > bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); > > +/* Same as block_job_set_speed(), but called with job lock held. */ > +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); > + > /** > * block_job_query: > * @job: The job to get information about. > @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); > */ > BlockJobInfo *block_job_query(BlockJob *job, Error **errp); > > +/* Same as block_job_query(), but called with job lock held. */ > +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); > + > /** > * block_job_iostatus_reset: > * @job: The job whose I/O status should be reset. > @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); > */ > void block_job_iostatus_reset(BlockJob *job); > > +/* Same as block_job_iostatus_reset(), but called with job lock held. */ > +void block_job_iostatus_reset_locked(BlockJob *job); > + > /* > * block_job_get_aio_context: > *
Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy: > On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote: >> Just as done with job.h, create _locked() functions in blockjob.h > > We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject. > > Also, we start to introduce _locked block_job_* APIs. > > Does it mean that BlockJob and Job share the global mutex to protect > themselves? Than I think we should document in BlockJob struct what is > protected by job_mutex. There is nothing in the struct (apart from Job) that is protected by the job lock. I can add a comment "Protected by job mutex" on top of Job job field? > > And please, let's be consistent on whether we add or not add "with mutex > held" / "with mutex not held" comments. For job API you mostly add it > for each function.. Let's do same here? Same for "temporary unlock" > comments. Where did I miss the mutex lock/unlock comments? Yes I forgot the "temporary unlock" thing but apart from that all functions have a comment saying if they take the lock or not. > >> >> These functions will be later useful when caller has already taken >> the lock. All blockjob _locked functions call job _locked functions. >> >> Note: at this stage, job_{lock/unlock} and job lock guard macros >> are *nop*. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> blockjob.c | 52 ++++++++++++++++++++++++++++++++-------- >> include/block/blockjob.h | 15 ++++++++++++ >> 2 files changed, 57 insertions(+), 10 deletions(-) >> >> diff --git a/blockjob.c b/blockjob.c >> index 7da59a1f1c..0d59aba439 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -44,21 +44,27 @@ static bool is_block_job(Job *job) >> job_type(job) == JOB_TYPE_STREAM; >> } >> -BlockJob *block_job_next(BlockJob *bjob) >> +BlockJob *block_job_next_locked(BlockJob *bjob) >> { >> Job *job = bjob ? &bjob->job : NULL; >> GLOBAL_STATE_CODE(); >> do { >> - job = job_next(job); >> + job = job_next_locked(job); >> } while (job && !is_block_job(job)); >> return job ? container_of(job, BlockJob, job) : NULL; >> } >> -BlockJob *block_job_get(const char *id) >> +BlockJob *block_job_next(BlockJob *bjob) >> { >> - Job *job = job_get(id); >> + JOB_LOCK_GUARD(); >> + return block_job_next_locked(bjob); >> +} >> + >> +BlockJob *block_job_get_locked(const char *id) >> +{ >> + Job *job = job_get_locked(id); >> GLOBAL_STATE_CODE(); >> if (job && is_block_job(job)) { >> @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id) >> } >> } >> +BlockJob *block_job_get(const char *id) >> +{ >> + JOB_LOCK_GUARD(); >> + return block_job_get_locked(id); >> +} >> + >> void block_job_free(Job *job) >> { >> BlockJob *bjob = container_of(job, BlockJob, job); >> @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job) >> return timer_pending(&job->sleep_timer); >> } >> -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) >> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error >> **errp) >> { >> const BlockJobDriver *drv = block_job_driver(job); >> int64_t old_speed = job->speed; >> GLOBAL_STATE_CODE(); >> - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { >> + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < >> 0) { >> return false; >> } >> if (speed < 0) { >> @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t >> speed, Error **errp) >> job->speed = speed; >> if (drv->set_speed) { >> + job_unlock(); >> drv->set_speed(job, speed); >> + job_lock(); >> } >> if (speed && speed <= old_speed) { >> @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t >> speed, Error **errp) >> } >> /* kick only if a timer is pending */ >> - job_enter_cond(&job->job, job_timer_pending); >> + job_enter_cond_locked(&job->job, job_timer_pending); >> return true; >> } >> +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) >> +{ >> + JOB_LOCK_GUARD(); >> + return block_job_set_speed_locked(job, speed, errp); >> +} >> + >> int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) >> { >> IO_CODE(); >> return ratelimit_calculate_delay(&job->limit, n); >> } >> -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) >> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) >> { >> BlockJobInfo *info; >> uint64_t progress_current, progress_total; >> @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error >> **errp) >> 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; >> @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, >> Error **errp) >> return info; >> } >> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp) >> +{ >> + JOB_LOCK_GUARD(); >> + return block_job_query_locked(job, errp); >> +} >> + >> static void block_job_iostatus_set_err(BlockJob *job, int error) >> { >> if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { >> @@ -478,7 +504,7 @@ fail: >> return NULL; >> } >> -void block_job_iostatus_reset(BlockJob *job) >> +void block_job_iostatus_reset_locked(BlockJob *job) >> { >> GLOBAL_STATE_CODE(); >> if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { >> @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job) >> job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; >> } >> +void block_job_iostatus_reset(BlockJob *job) >> +{ >> + JOB_LOCK_GUARD(); >> + block_job_iostatus_reset_locked(job); >> +} >> + >> void block_job_user_resume(Job *job) >> { >> BlockJob *bjob = container_of(job, BlockJob, job); >> diff --git a/include/block/blockjob.h b/include/block/blockjob.h >> index 6525e16fd5..3959a98612 100644 >> --- a/include/block/blockjob.h >> +++ b/include/block/blockjob.h >> @@ -92,6 +92,9 @@ typedef struct BlockJob { >> */ >> BlockJob *block_job_next(BlockJob *job); >> +/* Same as block_job_next(), but called with job lock held. */ >> +BlockJob *block_job_next_locked(BlockJob *job); >> + >> /** >> * block_job_get: >> * @id: The id of the block job. >> @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job); >> */ >> BlockJob *block_job_get(const char *id); >> +/* Same as block_job_get(), but called with job lock held. */ >> +BlockJob *block_job_get_locked(const char *id); >> + >> /** >> * block_job_add_bdrv: >> * @job: A block job >> @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job, >> BlockDriverState *bs); >> */ >> bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); >> +/* Same as block_job_set_speed(), but called with job lock held. */ >> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error >> **errp); >> + >> /** >> * block_job_query: >> * @job: The job to get information about. >> @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t >> speed, Error **errp); >> */ >> BlockJobInfo *block_job_query(BlockJob *job, Error **errp); >> +/* Same as block_job_query(), but called with job lock held. */ >> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); >> + >> /** >> * block_job_iostatus_reset: >> * @job: The job whose I/O status should be reset. >> @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error >> **errp); >> */ >> void block_job_iostatus_reset(BlockJob *job); >> +/* Same as block_job_iostatus_reset(), but called with job lock >> held. */ >> +void block_job_iostatus_reset_locked(BlockJob *job); >> + >> /* >> * block_job_get_aio_context: >> * > >
On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote: > > > Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy: >> On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote: >>> Just as done with job.h, create _locked() functions in blockjob.h >> >> We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject. >> >> Also, we start to introduce _locked block_job_* APIs. >> >> Does it mean that BlockJob and Job share the global mutex to protect >> themselves? Than I think we should document in BlockJob struct what is >> protected by job_mutex. > > There is nothing in the struct (apart from Job) that is protected by the > job lock. I can add a comment "Protected by job mutex" on top of Job job > field? Yes, I think that's worth doing. Other fields doesn't need the lock? > >> >> And please, let's be consistent on whether we add or not add "with mutex >> held" / "with mutex not held" comments. For job API you mostly add it >> for each function.. Let's do same here? Same for "temporary unlock" >> comments. > > Where did I miss the mutex lock/unlock comments? Yes I forgot the > "temporary unlock" thing but apart from that all functions have a > comment saying if they take the lock or not. Probably that's my impression because you add some comments in separate patches. OK. > >> >>> >>> These functions will be later useful when caller has already taken >>> the lock. All blockjob _locked functions call job _locked functions. >>> >>> Note: at this stage, job_{lock/unlock} and job lock guard macros >>> are *nop*. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> --- >>> blockjob.c | 52 ++++++++++++++++++++++++++++++++-------- >>> include/block/blockjob.h | 15 ++++++++++++ >>> 2 files changed, 57 insertions(+), 10 deletions(-) >>> >>> diff --git a/blockjob.c b/blockjob.c >>> index 7da59a1f1c..0d59aba439 100644 >>> --- a/blockjob.c >>> +++ b/blockjob.c >>> @@ -44,21 +44,27 @@ static bool is_block_job(Job *job) >>> job_type(job) == JOB_TYPE_STREAM; >>> } >>> -BlockJob *block_job_next(BlockJob *bjob) >>> +BlockJob *block_job_next_locked(BlockJob *bjob) >>> { >>> Job *job = bjob ? &bjob->job : NULL; >>> GLOBAL_STATE_CODE(); >>> do { >>> - job = job_next(job); >>> + job = job_next_locked(job); >>> } while (job && !is_block_job(job)); >>> return job ? container_of(job, BlockJob, job) : NULL; >>> } >>> -BlockJob *block_job_get(const char *id) >>> +BlockJob *block_job_next(BlockJob *bjob) >>> { >>> - Job *job = job_get(id); >>> + JOB_LOCK_GUARD(); >>> + return block_job_next_locked(bjob); >>> +} >>> + >>> +BlockJob *block_job_get_locked(const char *id) >>> +{ >>> + Job *job = job_get_locked(id); >>> GLOBAL_STATE_CODE(); >>> if (job && is_block_job(job)) { >>> @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id) >>> } >>> } >>> +BlockJob *block_job_get(const char *id) >>> +{ >>> + JOB_LOCK_GUARD(); >>> + return block_job_get_locked(id); >>> +} >>> + >>> void block_job_free(Job *job) >>> { >>> BlockJob *bjob = container_of(job, BlockJob, job); >>> @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job) >>> return timer_pending(&job->sleep_timer); >>> } >>> -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) >>> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error >>> **errp) >>> { >>> const BlockJobDriver *drv = block_job_driver(job); >>> int64_t old_speed = job->speed; >>> GLOBAL_STATE_CODE(); >>> - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { >>> + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < >>> 0) { >>> return false; >>> } >>> if (speed < 0) { >>> @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t >>> speed, Error **errp) >>> job->speed = speed; >>> if (drv->set_speed) { >>> + job_unlock(); >>> drv->set_speed(job, speed); >>> + job_lock(); >>> } >>> if (speed && speed <= old_speed) { >>> @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t >>> speed, Error **errp) >>> } >>> /* kick only if a timer is pending */ >>> - job_enter_cond(&job->job, job_timer_pending); >>> + job_enter_cond_locked(&job->job, job_timer_pending); >>> return true; >>> } >>> +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) >>> +{ >>> + JOB_LOCK_GUARD(); >>> + return block_job_set_speed_locked(job, speed, errp); >>> +} >>> + >>> int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) >>> { >>> IO_CODE(); >>> return ratelimit_calculate_delay(&job->limit, n); >>> } >>> -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) >>> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) >>> { >>> BlockJobInfo *info; >>> uint64_t progress_current, progress_total; >>> @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error >>> **errp) >>> 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; >>> @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, >>> Error **errp) >>> return info; >>> } >>> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp) >>> +{ >>> + JOB_LOCK_GUARD(); >>> + return block_job_query_locked(job, errp); >>> +} >>> + >>> static void block_job_iostatus_set_err(BlockJob *job, int error) >>> { >>> if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { >>> @@ -478,7 +504,7 @@ fail: >>> return NULL; >>> } >>> -void block_job_iostatus_reset(BlockJob *job) >>> +void block_job_iostatus_reset_locked(BlockJob *job) >>> { >>> GLOBAL_STATE_CODE(); >>> if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { >>> @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job) >>> job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; >>> } >>> +void block_job_iostatus_reset(BlockJob *job) >>> +{ >>> + JOB_LOCK_GUARD(); >>> + block_job_iostatus_reset_locked(job); >>> +} >>> + >>> void block_job_user_resume(Job *job) >>> { >>> BlockJob *bjob = container_of(job, BlockJob, job); >>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h >>> index 6525e16fd5..3959a98612 100644 >>> --- a/include/block/blockjob.h >>> +++ b/include/block/blockjob.h >>> @@ -92,6 +92,9 @@ typedef struct BlockJob { >>> */ >>> BlockJob *block_job_next(BlockJob *job); >>> +/* Same as block_job_next(), but called with job lock held. */ >>> +BlockJob *block_job_next_locked(BlockJob *job); >>> + >>> /** >>> * block_job_get: >>> * @id: The id of the block job. >>> @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job); >>> */ >>> BlockJob *block_job_get(const char *id); >>> +/* Same as block_job_get(), but called with job lock held. */ >>> +BlockJob *block_job_get_locked(const char *id); >>> + >>> /** >>> * block_job_add_bdrv: >>> * @job: A block job >>> @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job, >>> BlockDriverState *bs); >>> */ >>> bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); >>> +/* Same as block_job_set_speed(), but called with job lock held. */ >>> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error >>> **errp); >>> + >>> /** >>> * block_job_query: >>> * @job: The job to get information about. >>> @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t >>> speed, Error **errp); >>> */ >>> BlockJobInfo *block_job_query(BlockJob *job, Error **errp); >>> +/* Same as block_job_query(), but called with job lock held. */ >>> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); >>> + >>> /** >>> * block_job_iostatus_reset: >>> * @job: The job whose I/O status should be reset. >>> @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error >>> **errp); >>> */ >>> void block_job_iostatus_reset(BlockJob *job); >>> +/* Same as block_job_iostatus_reset(), but called with job lock >>> held. */ >>> +void block_job_iostatus_reset_locked(BlockJob *job); >>> + >>> /* >>> * block_job_get_aio_context: >>> * >> >> >
Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy: > On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote: >> >> >> Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy: >>> On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote: >>>> Just as done with job.h, create _locked() functions in blockjob.h >>> >>> We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject. >>> >>> Also, we start to introduce _locked block_job_* APIs. >>> >>> Does it mean that BlockJob and Job share the global mutex to protect >>> themselves? Than I think we should document in BlockJob struct what is >>> protected by job_mutex. >> >> There is nothing in the struct (apart from Job) that is protected by the >> job lock. I can add a comment "Protected by job mutex" on top of Job job >> field? > > Yes, I think that's worth doing. > > Other fields doesn't need the lock? > Well I didn't plan to actually look at it but now that you ask: /** needs protection, so it can go under job lock */ BlockDeviceIoStatus iostatus; /** mostly under lock, not sure when it is called as notifier callback though. I think they are GLOBAL_STATE, what do you think? */ int64_t speed; /** thread safe API */ RateLimit limit; /** I think it's also thread safe */ Error *blocker; /* always under job lock */ Notifier finalize_cancelled_notifier; Notifier finalize_completed_notifier; Notifier pending_notifier; Notifier ready_notifier; Notifier idle_notifier; Not sure about blockjob->speed though. Emanuele
Am 06/07/2022 um 14:36 schrieb Emanuele Giuseppe Esposito: > > > Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy: >> On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote: >>> >>> >>> Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy: >>>> On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote: >>>>> Just as done with job.h, create _locked() functions in blockjob.h >>>> >>>> We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject. >>>> >>>> Also, we start to introduce _locked block_job_* APIs. >>>> >>>> Does it mean that BlockJob and Job share the global mutex to protect >>>> themselves? Than I think we should document in BlockJob struct what is >>>> protected by job_mutex. >>> >>> There is nothing in the struct (apart from Job) that is protected by the >>> job lock. I can add a comment "Protected by job mutex" on top of Job job >>> field? >> >> Yes, I think that's worth doing. >> >> Other fields doesn't need the lock? >> > Well I didn't plan to actually look at it but now that you ask: > > /** needs protection, so it can go under job lock */ > BlockDeviceIoStatus iostatus; > > /** mostly under lock, not sure when it is called as notifier callback > though. I think they are GLOBAL_STATE, what do you think? */ > int64_t speed; > > /** thread safe API */ > RateLimit limit; > > /** I think it's also thread safe */ > Error *blocker; > > /* always under job lock */ Actually that's wrong, they are just set once and never modified. And GSList *nodes; is also always called under GS. So there's only iostatus to protect and maybe speed. Emanuele > Notifier finalize_cancelled_notifier; > Notifier finalize_completed_notifier; > Notifier pending_notifier; > Notifier ready_notifier; > Notifier idle_notifier; > > Not sure about blockjob->speed though. > > Emanuele >
On 7/6/22 15:59, Emanuele Giuseppe Esposito wrote: > > > Am 06/07/2022 um 14:36 schrieb Emanuele Giuseppe Esposito: >> >> >> Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy: >>> On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote: >>>> >>>> >>>> Am 05/07/2022 um 17:01 schrieb Vladimir Sementsov-Ogievskiy: >>>>> On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote: >>>>>> Just as done with job.h, create _locked() functions in blockjob.h >>>>> >>>>> We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject. >>>>> >>>>> Also, we start to introduce _locked block_job_* APIs. >>>>> >>>>> Does it mean that BlockJob and Job share the global mutex to protect >>>>> themselves? Than I think we should document in BlockJob struct what is >>>>> protected by job_mutex. >>>> >>>> There is nothing in the struct (apart from Job) that is protected by the >>>> job lock. I can add a comment "Protected by job mutex" on top of Job job >>>> field? >>> >>> Yes, I think that's worth doing. >>> >>> Other fields doesn't need the lock? >>> >> Well I didn't plan to actually look at it but now that you ask: >> >> /** needs protection, so it can go under job lock */ >> BlockDeviceIoStatus iostatus; >> >> /** mostly under lock, not sure when it is called as notifier callback >> though. I think they are GLOBAL_STATE, what do you think? */ >> int64_t speed; Hmm I doubt that notifier callbacks are always called from GS code.. But reading .speed to send an event doesn't seem to worth any locking. >> >> /** thread safe API */ >> RateLimit limit; >> >> /** I think it's also thread safe */ >> Error *blocker; >> >> /* always under job lock */ > Actually that's wrong, they are just set once and never modified. > > And GSList *nodes; is also always called under GS. > > So there's only iostatus to protect and maybe speed. >
diff --git a/blockjob.c b/blockjob.c index 7da59a1f1c..0d59aba439 100644 --- a/blockjob.c +++ b/blockjob.c @@ -44,21 +44,27 @@ static bool is_block_job(Job *job) job_type(job) == JOB_TYPE_STREAM; } -BlockJob *block_job_next(BlockJob *bjob) +BlockJob *block_job_next_locked(BlockJob *bjob) { Job *job = bjob ? &bjob->job : NULL; GLOBAL_STATE_CODE(); do { - job = job_next(job); + job = job_next_locked(job); } while (job && !is_block_job(job)); return job ? container_of(job, BlockJob, job) : NULL; } -BlockJob *block_job_get(const char *id) +BlockJob *block_job_next(BlockJob *bjob) { - Job *job = job_get(id); + JOB_LOCK_GUARD(); + return block_job_next_locked(bjob); +} + +BlockJob *block_job_get_locked(const char *id) +{ + Job *job = job_get_locked(id); GLOBAL_STATE_CODE(); if (job && is_block_job(job)) { @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id) } } +BlockJob *block_job_get(const char *id) +{ + JOB_LOCK_GUARD(); + return block_job_get_locked(id); +} + void block_job_free(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job) return timer_pending(&job->sleep_timer); } -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp) { const BlockJobDriver *drv = block_job_driver(job); int64_t old_speed = job->speed; GLOBAL_STATE_CODE(); - if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { + if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < 0) { return false; } if (speed < 0) { @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) job->speed = speed; if (drv->set_speed) { + job_unlock(); drv->set_speed(job, speed); + job_lock(); } if (speed && speed <= old_speed) { @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) } /* kick only if a timer is pending */ - job_enter_cond(&job->job, job_timer_pending); + job_enter_cond_locked(&job->job, job_timer_pending); return true; } +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) +{ + JOB_LOCK_GUARD(); + return block_job_set_speed_locked(job, speed, errp); +} + int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n) { IO_CODE(); return ratelimit_calculate_delay(&job->limit, n); } -BlockJobInfo *block_job_query(BlockJob *job, Error **errp) +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp) { BlockJobInfo *info; uint64_t progress_current, progress_total; @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) 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; @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp) return info; } +BlockJobInfo *block_job_query(BlockJob *job, Error **errp) +{ + JOB_LOCK_GUARD(); + return block_job_query_locked(job, errp); +} + static void block_job_iostatus_set_err(BlockJob *job, int error) { if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -478,7 +504,7 @@ fail: return NULL; } -void block_job_iostatus_reset(BlockJob *job) +void block_job_iostatus_reset_locked(BlockJob *job) { GLOBAL_STATE_CODE(); if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) { @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job) job->iostatus = BLOCK_DEVICE_IO_STATUS_OK; } +void block_job_iostatus_reset(BlockJob *job) +{ + JOB_LOCK_GUARD(); + block_job_iostatus_reset_locked(job); +} + void block_job_user_resume(Job *job) { BlockJob *bjob = container_of(job, BlockJob, job); diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 6525e16fd5..3959a98612 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -92,6 +92,9 @@ typedef struct BlockJob { */ BlockJob *block_job_next(BlockJob *job); +/* Same as block_job_next(), but called with job lock held. */ +BlockJob *block_job_next_locked(BlockJob *job); + /** * block_job_get: * @id: The id of the block job. @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job); */ BlockJob *block_job_get(const char *id); +/* Same as block_job_get(), but called with job lock held. */ +BlockJob *block_job_get_locked(const char *id); + /** * block_job_add_bdrv: * @job: A block job @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs); */ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); +/* Same as block_job_set_speed(), but called with job lock held. */ +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp); + /** * block_job_query: * @job: The job to get information about. @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp); */ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); +/* Same as block_job_query(), but called with job lock held. */ +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp); + /** * block_job_iostatus_reset: * @job: The job whose I/O status should be reset. @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp); */ void block_job_iostatus_reset(BlockJob *job); +/* Same as block_job_iostatus_reset(), but called with job lock held. */ +void block_job_iostatus_reset_locked(BlockJob *job); + /* * block_job_get_aio_context: *
Just as done with job.h, create _locked() functions in blockjob.h These functions will be later useful when caller has already taken the lock. All blockjob _locked functions call job _locked functions. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- blockjob.c | 52 ++++++++++++++++++++++++++++++++-------- include/block/blockjob.h | 15 ++++++++++++ 2 files changed, 57 insertions(+), 10 deletions(-)