| Message ID | 20220706201533.289775-18-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: > The same job lock is being used also to protect some of blockjob fields. > Categorize them just as done in job.h. Thanks! > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/block/blockjob.h | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/include/block/blockjob.h b/include/block/blockjob.h > index 8b65d3949d..912e10b083 100644 > --- a/include/block/blockjob.h > +++ b/include/block/blockjob.h > @@ -40,10 +40,16 @@ typedef struct BlockJobDriver BlockJobDriver; > * Long-running operation on a BlockDriverState. > */ > typedef struct BlockJob { > - /** Data belonging to the generic Job infrastructure */ > + /** > + * Data belonging to the generic Job infrastructure. > + * Protected by job mutex. > + */ > Job job; > > - /** Status that is published by the query-block-jobs QMP API */ > + /** > + * Status that is published by the query-block-jobs QMP API. > + * Protected by job mutex. > + */ > BlockDeviceIoStatus iostatus; > > /** Speed that was set with @block_job_set_speed. */ > @@ -55,6 +61,8 @@ typedef struct BlockJob { > /** Block other operations when block job is running */ > Error *blocker; > > + /** All notifiers are set once in block_job_create() and never modified. */ > + > /** Called when a cancelled job is finalised. */ > Notifier finalize_cancelled_notifier; > > @@ -70,7 +78,10 @@ typedef struct BlockJob { > /** Called when the job coroutine yields or terminates */ > Notifier idle_notifier; > > - /** BlockDriverStates that are involved in this block job */ > + /** > + * BlockDriverStates that are involved in this block job. > + * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE) > + */ > GSList *nodes; > } BlockJob; > Can we also add GLOBAL_STATE_CODE(); marker into child_job_can_set_aio_ctx() and child_job_set_aio_ctx() ? Anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Am 11/07/2022 um 16:44 schrieb Vladimir Sementsov-Ogievskiy: > On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote: >> The same job lock is being used also to protect some of blockjob fields. >> Categorize them just as done in job.h. > > Thanks! > >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> include/block/blockjob.h | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/include/block/blockjob.h b/include/block/blockjob.h >> index 8b65d3949d..912e10b083 100644 >> --- a/include/block/blockjob.h >> +++ b/include/block/blockjob.h >> @@ -40,10 +40,16 @@ typedef struct BlockJobDriver BlockJobDriver; >> * Long-running operation on a BlockDriverState. >> */ >> typedef struct BlockJob { >> - /** Data belonging to the generic Job infrastructure */ >> + /** >> + * Data belonging to the generic Job infrastructure. >> + * Protected by job mutex. >> + */ >> Job job; >> - /** Status that is published by the query-block-jobs QMP API */ >> + /** >> + * Status that is published by the query-block-jobs QMP API. >> + * Protected by job mutex. >> + */ >> BlockDeviceIoStatus iostatus; >> /** Speed that was set with @block_job_set_speed. */ >> @@ -55,6 +61,8 @@ typedef struct BlockJob { >> /** Block other operations when block job is running */ >> Error *blocker; >> + /** All notifiers are set once in block_job_create() and never >> modified. */ >> + >> /** Called when a cancelled job is finalised. */ >> Notifier finalize_cancelled_notifier; >> @@ -70,7 +78,10 @@ typedef struct BlockJob { >> /** Called when the job coroutine yields or terminates */ >> Notifier idle_notifier; >> - /** BlockDriverStates that are involved in this block job */ >> + /** >> + * BlockDriverStates that are involved in this block job. >> + * Always modified and read under QEMU global mutex >> (GLOBAL_STATE_CODE) >> + */ >> GSList *nodes; >> } BlockJob; >> > > Can we also add GLOBAL_STATE_CODE(); marker into > child_job_can_set_aio_ctx() and child_job_set_aio_ctx() ? Usually for callbacks I feel redundant to add assertions there. It is enough to look at their definition in the header to understand which category are they in. Emanuele > > Anyway: > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >
diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 8b65d3949d..912e10b083 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -40,10 +40,16 @@ typedef struct BlockJobDriver BlockJobDriver; * Long-running operation on a BlockDriverState. */ typedef struct BlockJob { - /** Data belonging to the generic Job infrastructure */ + /** + * Data belonging to the generic Job infrastructure. + * Protected by job mutex. + */ Job job; - /** Status that is published by the query-block-jobs QMP API */ + /** + * Status that is published by the query-block-jobs QMP API. + * Protected by job mutex. + */ BlockDeviceIoStatus iostatus; /** Speed that was set with @block_job_set_speed. */ @@ -55,6 +61,8 @@ typedef struct BlockJob { /** Block other operations when block job is running */ Error *blocker; + /** All notifiers are set once in block_job_create() and never modified. */ + /** Called when a cancelled job is finalised. */ Notifier finalize_cancelled_notifier; @@ -70,7 +78,10 @@ typedef struct BlockJob { /** Called when the job coroutine yields or terminates */ Notifier idle_notifier; - /** BlockDriverStates that are involved in this block job */ + /** + * BlockDriverStates that are involved in this block job. + * Always modified and read under QEMU global mutex (GLOBAL_STATE_CODE) + */ GSList *nodes; } BlockJob;
The same job lock is being used also to protect some of blockjob fields. Categorize them just as done in job.h. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/block/blockjob.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)