| Message ID | 20220725073855.76049-11-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben: > 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*. > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> Can you explain in the commit message what the TOC/TOU case is that this patch is addressing? It's not obvious to me why you picked exactly these places to add locking. > diff --git a/block/mirror.c b/block/mirror.c > index d8ecb9efa2..b38676e19d 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -654,9 +654,13 @@ 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; > int ret = 0; > > + WITH_JOB_LOCK_GUARD() { > + abort = job->ret < 0; > + } This is the most mysterious hunk to me. The only thing that should modify job->ret is the caller of this function anyway, but let's assume for a moment that another thread could write to it. Then why is it only important that we hold the lock when we're reading the value, but not any more when we are actually using it? And what is the TOC/TOU that this fixes? Kevin
Am 04/08/2022 um 18:35 schrieb Kevin Wolf: > Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben: >> 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*. >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > > Can you explain in the commit message what the TOC/TOU case is that this > patch is addressing? It's not obvious to me why you picked exactly these > places to add locking. Well after thinking about it, mentioning TOC/TOU doesn't really make sense here. I'll remove the "to avoid TOC/TOU" in the title. > >> diff --git a/block/mirror.c b/block/mirror.c >> index d8ecb9efa2..b38676e19d 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -654,9 +654,13 @@ 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; >> int ret = 0; >> >> + WITH_JOB_LOCK_GUARD() { >> + abort = job->ret < 0; >> + } > > This is the most mysterious hunk to me. The only thing that should > modify job->ret is the caller of this function anyway, but let's assume > for a moment that another thread could write to it. > > Then why is it only important that we hold the lock when we're reading > the value, but not any more when we are actually using it? And what is > the TOC/TOU that this fixes? No TOC/TOU, no sense for this fix too. I'll remove this hunk too. Emanuele > > Kevin >
diff --git a/block/mirror.c b/block/mirror.c index d8ecb9efa2..b38676e19d 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -654,9 +654,13 @@ 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; int ret = 0; + WITH_JOB_LOCK_GUARD() { + abort = job->ret < 0; + } + if (s->prepared) { return 0; } @@ -1152,8 +1156,10 @@ 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); + WITH_JOB_LOCK_GUARD() { + if (!job->paused) { + job_enter_cond_locked(job, NULL); + } } } @@ -1173,8 +1179,11 @@ 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) { - return true; + WITH_JOB_LOCK_GUARD() { + if (!s->common.job.paused && !job_is_cancelled_locked(&job->job) + && !s->in_drain) { + return true; + } } return !!s->in_flight;
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*. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/mirror.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)