Re: [RFC PATCH v2 05/14] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU

[Emanuele Giuseppe Esposito]

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