| Message ID | 20211104145334.1346363-6-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: > Once job lock is used and aiocontext is removed, mirror has > to perform job operations under the same critical section, > using the helpers prepared in previous commit. > > Note: at this stage, job_{lock/unlock} and job lock guard macros > are *nop*. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/mirror.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/block/mirror.c b/block/mirror.c > index 00089e519b..f22fa7da6e 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job) > BlockDriverState *target_bs; > BlockDriverState *mirror_top_bs; > Error *local_err = NULL; > - bool abort = job->ret < 0; > + bool abort = job_has_failed(job); > int ret = 0; > > if (s->prepared) { > @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp) > s->should_complete = true; > > /* If the job is paused, it will be re-entered when it is resumed */ > - if (!job->paused) { > - job_enter(job); > - } > + job_enter_not_paused(job); > } > > static void coroutine_fn mirror_pause(Job *job) > @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job) > * from one of our own drain sections, to avoid a deadlock waiting for > * ourselves. > */ > - if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) { > + if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) { > return true; > } > > Why to introduce a separate API function for every use case? Could we instead just use WITH_JOB_LOCK_GUARD() ?
On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote: > 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: >> Once job lock is used and aiocontext is removed, mirror has >> to perform job operations under the same critical section, >> using the helpers prepared in previous commit. >> >> Note: at this stage, job_{lock/unlock} and job lock guard macros >> are *nop*. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> block/mirror.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index 00089e519b..f22fa7da6e 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job) >> BlockDriverState *target_bs; >> BlockDriverState *mirror_top_bs; >> Error *local_err = NULL; >> - bool abort = job->ret < 0; >> + bool abort = job_has_failed(job); >> int ret = 0; >> if (s->prepared) { >> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp) >> s->should_complete = true; >> /* If the job is paused, it will be re-entered when it is >> resumed */ >> - if (!job->paused) { >> - job_enter(job); >> - } >> + job_enter_not_paused(job); >> } >> static void coroutine_fn mirror_pause(Job *job) >> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job) >> * from one of our own drain sections, to avoid a deadlock >> waiting for >> * ourselves. >> */ >> - if (!s->common.job.paused && !job_is_cancelled(&job->job) && >> !s->in_drain) { >> + if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) { >> return true; >> } >> > > Why to introduce a separate API function for every use case? > > Could we instead just use WITH_JOB_LOCK_GUARD() ? > This implies making the struct job_mutex public. Is that ok for you? Thank you, Emanuele
20.12.2021 13:34, Emanuele Giuseppe Esposito wrote: > > > On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote: >> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: >>> Once job lock is used and aiocontext is removed, mirror has >>> to perform job operations under the same critical section, >>> using the helpers prepared in previous commit. >>> >>> Note: at this stage, job_{lock/unlock} and job lock guard macros >>> are *nop*. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> --- >>> block/mirror.c | 8 +++----- >>> 1 file changed, 3 insertions(+), 5 deletions(-) >>> >>> diff --git a/block/mirror.c b/block/mirror.c >>> index 00089e519b..f22fa7da6e 100644 >>> --- a/block/mirror.c >>> +++ b/block/mirror.c >>> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job) >>> BlockDriverState *target_bs; >>> BlockDriverState *mirror_top_bs; >>> Error *local_err = NULL; >>> - bool abort = job->ret < 0; >>> + bool abort = job_has_failed(job); >>> int ret = 0; >>> if (s->prepared) { >>> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp) >>> s->should_complete = true; >>> /* If the job is paused, it will be re-entered when it is resumed */ >>> - if (!job->paused) { >>> - job_enter(job); >>> - } >>> + job_enter_not_paused(job); >>> } >>> static void coroutine_fn mirror_pause(Job *job) >>> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job) >>> * from one of our own drain sections, to avoid a deadlock waiting for >>> * ourselves. >>> */ >>> - if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) { >>> + if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) { >>> return true; >>> } >>> >> >> Why to introduce a separate API function for every use case? >> >> Could we instead just use WITH_JOB_LOCK_GUARD() ? >> > > This implies making the struct job_mutex public. Is that ok for you? > Yes, I think it's OK. Alternatively, you can use job_lock() / job_unlock(), or even rewrite WITH_JOB_LOCK_GUARD() macro using job_lock/job_unlock, to keep mutex private.. But I don't think it really worth it now. Note that struct Job is already public, so if we'll use per-job mutex in future it still is not a problem. Only when we decide to make struct Job private, we'll have to decide something about JOB_LOCK_GUARD(), and at this point we'll just rewrite it to work through some helper function instead of directly touching the mutex.
On 20/12/2021 11:47, Vladimir Sementsov-Ogievskiy wrote: > 20.12.2021 13:34, Emanuele Giuseppe Esposito wrote: >> >> >> On 18/12/2021 12:53, Vladimir Sementsov-Ogievskiy wrote: >>> 04.11.2021 17:53, Emanuele Giuseppe Esposito wrote: >>>> Once job lock is used and aiocontext is removed, mirror has >>>> to perform job operations under the same critical section, >>>> using the helpers prepared in previous commit. >>>> >>>> Note: at this stage, job_{lock/unlock} and job lock guard macros >>>> are *nop*. >>>> >>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>>> --- >>>> block/mirror.c | 8 +++----- >>>> 1 file changed, 3 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/block/mirror.c b/block/mirror.c >>>> index 00089e519b..f22fa7da6e 100644 >>>> --- a/block/mirror.c >>>> +++ b/block/mirror.c >>>> @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job) >>>> BlockDriverState *target_bs; >>>> BlockDriverState *mirror_top_bs; >>>> Error *local_err = NULL; >>>> - bool abort = job->ret < 0; >>>> + bool abort = job_has_failed(job); >>>> int ret = 0; >>>> if (s->prepared) { >>>> @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error >>>> **errp) >>>> s->should_complete = true; >>>> /* If the job is paused, it will be re-entered when it is >>>> resumed */ >>>> - if (!job->paused) { >>>> - job_enter(job); >>>> - } >>>> + job_enter_not_paused(job); >>>> } >>>> static void coroutine_fn mirror_pause(Job *job) >>>> @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job) >>>> * from one of our own drain sections, to avoid a deadlock >>>> waiting for >>>> * ourselves. >>>> */ >>>> - if (!s->common.job.paused && !job_is_cancelled(&job->job) && >>>> !s->in_drain) { >>>> + if (job_not_paused_nor_cancelled(&s->common.job) && >>>> !s->in_drain) { >>>> return true; >>>> } >>>> >>> >>> Why to introduce a separate API function for every use case? >>> >>> Could we instead just use WITH_JOB_LOCK_GUARD() ? >>> >> >> This implies making the struct job_mutex public. Is that ok for you? >> > > Yes, I think it's OK. > > Alternatively, you can use job_lock() / job_unlock(), or even rewrite > WITH_JOB_LOCK_GUARD() macro using job_lock/job_unlock, to keep mutex > private.. But I don't think it really worth it now. > > Note that struct Job is already public, so if we'll use per-job mutex in > future it still is not a problem. Only when we decide to make struct Job > private, we'll have to decide something about JOB_LOCK_GUARD(), and at > this point we'll just rewrite it to work through some helper function > instead of directly touching the mutex. > > Ok I will do that. Just FYI the initial idea was that drivers like monitor would not need to know about job_mutex lock, that is why I made the helpers in mirror.c. Thank you, Emanuele
diff --git a/block/mirror.c b/block/mirror.c index 00089e519b..f22fa7da6e 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job) BlockDriverState *target_bs; BlockDriverState *mirror_top_bs; Error *local_err = NULL; - bool abort = job->ret < 0; + bool abort = job_has_failed(job); int ret = 0; if (s->prepared) { @@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp) s->should_complete = true; /* If the job is paused, it will be re-entered when it is resumed */ - if (!job->paused) { - job_enter(job); - } + job_enter_not_paused(job); } static void coroutine_fn mirror_pause(Job *job) @@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job) * from one of our own drain sections, to avoid a deadlock waiting for * ourselves. */ - if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain) { + if (job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) { return true; }
Once job lock is used and aiocontext is removed, mirror has to perform job operations under the same critical section, using the helpers prepared in previous commit. Note: at this stage, job_{lock/unlock} and job lock guard macros are *nop*. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/mirror.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)