Re: [RFC PATCH 5/6] job: use global job_mutex to protect struct Job

[Emanuele Giuseppe Esposito]

On 08/07/2021 14:56, Stefan Hajnoczi wrote:
> On Wed, Jul 07, 2021 at 06:58:12PM +0200, Emanuele Giuseppe Esposito wrote:
> > This lock is going to replace most of the AioContext locks
> > in the job and blockjob, so that a Job can run in an arbitrary
> > AioContext.
> >
> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > ---
> >   include/block/blockjob_int.h |   1 +
> >   include/qemu/job.h           |   2 +
> >   block/backup.c               |   4 +
> >   block/mirror.c               |  11 +-
> >   blockdev.c                   |  62 ++++----
> >   blockjob.c                   |  67 +++++++--
> >   job-qmp.c                    |  55 +++----
> >   job.c                        | 284 +++++++++++++++++++++++++++--------
> >   qemu-img.c                   |  15 +-
> >   9 files changed, 350 insertions(+), 151 deletions(-)
> >
> > diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> > index 6633d83da2..8b91126506 100644
> > --- a/include/block/blockjob_int.h
> > +++ b/include/block/blockjob_int.h
> > @@ -53,6 +53,7 @@ struct BlockJobDriver {
> >        */
> >       void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> >   
> > +    /* Called with job mutex *not* held. */
> >       void (*set_speed)(BlockJob *job, int64_t speed);
> >   };
> >   
> > diff --git a/include/qemu/job.h b/include/qemu/job.h
> > index 4421d08d93..359f4e6b3a 100644
> > --- a/include/qemu/job.h
> > +++ b/include/qemu/job.h
> > @@ -49,6 +49,8 @@ typedef struct Job {
> >       /**
> >        * The type of this job.
> >        * Set it in job_create and just read.
> > +     * All calls to the driver function must be not locked by job_mutex,
> > +     * to avoid deadlocks.
> >        */
> >       const JobDriver *driver;
> >   
> > diff --git a/block/backup.c b/block/backup.c
> > index bd3614ce70..80ce956299 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -315,6 +315,10 @@ static void coroutine_fn backup_pause(Job *job)
> >       }
> >   }
> >   
> > +/*
> > + * Called with job mutex *not* held (we don't want to call block_copy_kick
> > + * with the lock held!)
> > + */
> >   static void coroutine_fn backup_set_speed(BlockJob *job, int64_t speed)
> >   {
> >       BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 49aaaafffa..deefaa6a39 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -1150,9 +1150,11 @@ 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 */
> > +    job_lock();
> >       if (!job_is_paused(job)) {
> > -        job_enter(job);
> > +        job_enter_locked(job);
> >       }
> > +    job_unlock();
> >   }
> >   
> >   static void coroutine_fn mirror_pause(Job *job)
> > @@ -1171,10 +1173,13 @@ static bool mirror_drained_poll(BlockJob *job)
> >        * from one of our own drain sections, to avoid a deadlock waiting for
> >        * ourselves.
> >        */
> > -    if (!job_is_paused(&s->common.job) && !job_is_cancelled(&s->common.job) &&
> > -        !s->in_drain) {
> > +    job_lock();
> > +    if (!job_is_paused(&s->common.job) &&
> > +        !job_is_cancelled_locked(&s->common.job) && !s->in_drain) {
> > +        job_unlock();
> >           return true;
> >       }
> > +    job_unlock();
> >   
> >       return !!s->in_flight;
> >   }
> > diff --git a/blockdev.c b/blockdev.c
> > index 8e2c15370e..9255aea6a2 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -150,9 +150,11 @@ void blockdev_mark_auto_del(BlockBackend *blk)
> >               AioContext *aio_context = job_get_aiocontext(&job->job);
> >               aio_context_acquire(aio_context);
> >   
> > +            job_lock();
> >               job_cancel(&job->job, false);
> >   
> >               aio_context_release(aio_context);
> > +            job_unlock();
>
> This looks strange. The way it's written suggests there is a reason why
> job_unlock() has to be called after aio_context_release(). Can
> job_unlock() be called immediately after job_cancel()?

Yes, another mistake I shouldn't have done.
> >           }
> >       }
> >   
> > @@ -3309,48 +3311,44 @@ out:
> >       aio_context_release(aio_context);
> >   }
> >   
> > -/* Get a block job using its ID and acquire its AioContext */
> > -static BlockJob *find_block_job(const char *id, AioContext **aio_context,
> > -                                Error **errp)
> > +/* Get a block job using its ID and acquire its job_lock */
>
> "its" suggests job_lock is per-Job. I suggest saying something like
> "Returns with job_lock held on success" instead.
Agree. Changed the same comment also for find_job() in job-qmp.c

Emanuele