Re: [PATCH v8 08/20] blockjob.h: introduce block_job _locked() APIs

[Vladimir Sementsov-Ogievskiy]

On 6/29/22 17:15, Emanuele Giuseppe Esposito wrote:
> Just as done with job.h, create _locked() functions in blockjob.h

We modify not only blockjob.h, I'd s/blockjob.h/blockjob/ in subject.

Also, we start to introduce _locked block_job_* APIs.

Does it mean that BlockJob and Job share the global mutex to protect 
themselves? Than I think we should document in BlockJob struct what is 
protected by job_mutex.

And please, let's be consistent on whether we add or not add "with mutex held" / "with mutex 
not held" comments. For job API you mostly add it for each function.. Let's do same here? Same for 
"temporary unlock" comments.

> These functions will be later useful when caller has already taken
> the lock. All blockjob _locked functions call job _locked functions.
>
> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   blockjob.c               | 52 ++++++++++++++++++++++++++++++++--------
>   include/block/blockjob.h | 15 ++++++++++++
>   2 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 7da59a1f1c..0d59aba439 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -44,21 +44,27 @@ static bool is_block_job(Job *job)
>              job_type(job) == JOB_TYPE_STREAM;
>   }
>   
> -BlockJob *block_job_next(BlockJob *bjob)
> +BlockJob *block_job_next_locked(BlockJob *bjob)
>   {
>       Job *job = bjob ? &bjob->job : NULL;
>       GLOBAL_STATE_CODE();
>   
>       do {
> -        job = job_next(job);
> +        job = job_next_locked(job);
>       } while (job && !is_block_job(job));
>   
>       return job ? container_of(job, BlockJob, job) : NULL;
>   }
>   
> -BlockJob *block_job_get(const char *id)
> +BlockJob *block_job_next(BlockJob *bjob)
>   {
> -    Job *job = job_get(id);
> +    JOB_LOCK_GUARD();
> +    return block_job_next_locked(bjob);
> +}
> +
> +BlockJob *block_job_get_locked(const char *id)
> +{
> +    Job *job = job_get_locked(id);
>       GLOBAL_STATE_CODE();
>   
>       if (job && is_block_job(job)) {
> @@ -68,6 +74,12 @@ BlockJob *block_job_get(const char *id)
>       }
>   }
>   
> +BlockJob *block_job_get(const char *id)
> +{
> +    JOB_LOCK_GUARD();
> +    return block_job_get_locked(id);
> +}
> +
>   void block_job_free(Job *job)
>   {
>       BlockJob *bjob = container_of(job, BlockJob, job);
> @@ -256,14 +268,14 @@ static bool job_timer_pending(Job *job)
>       return timer_pending(&job->sleep_timer);
>   }
>   
> -bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp)
>   {
>       const BlockJobDriver *drv = block_job_driver(job);
>       int64_t old_speed = job->speed;
>   
>       GLOBAL_STATE_CODE();
>   
> -    if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
> +    if (job_apply_verb_locked(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
>           return false;
>       }
>       if (speed < 0) {
> @@ -277,7 +289,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, 
> Error **errp)
>       job->speed = speed;
>   
>       if (drv->set_speed) {
> +        job_unlock();
>           drv->set_speed(job, speed);
> +        job_lock();
>       }
>   
>       if (speed && speed <= old_speed) {
> @@ -285,18 +299,24 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, 
> Error **errp)
>       }
>   
>       /* kick only if a timer is pending */
> -    job_enter_cond(&job->job, job_timer_pending);
> +    job_enter_cond_locked(&job->job, job_timer_pending);
>   
>       return true;
>   }
>   
> +bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
> +{
> +    JOB_LOCK_GUARD();
> +    return block_job_set_speed_locked(job, speed, errp);
> +}
> +
>   int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
>   {
>       IO_CODE();
>       return ratelimit_calculate_delay(&job->limit, n);
>   }
>   
> -BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp)
>   {
>       BlockJobInfo *info;
>       uint64_t progress_current, progress_total;
> @@ -320,7 +340,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>       info->len       = progress_total;
>       info->speed     = job->speed;
>       info->io_status = job->iostatus;
> -    info->ready     = job_is_ready(&job->job),
> +    info->ready     = job_is_ready_locked(&job->job),
>       info->status    = job->job.status;
>       info->auto_finalize = job->job.auto_finalize;
>       info->auto_dismiss  = job->job.auto_dismiss;
> @@ -333,6 +353,12 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>       return info;
>   }
>   
> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
> +{
> +    JOB_LOCK_GUARD();
> +    return block_job_query_locked(job, errp);
> +}
> +
>   static void block_job_iostatus_set_err(BlockJob *job, int error)
>   {
>       if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> @@ -478,7 +504,7 @@ fail:
>       return NULL;
>   }
>   
> -void block_job_iostatus_reset(BlockJob *job)
> +void block_job_iostatus_reset_locked(BlockJob *job)
>   {
>       GLOBAL_STATE_CODE();
>       if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> @@ -488,6 +514,12 @@ void block_job_iostatus_reset(BlockJob *job)
>       job->iostatus = BLOCK_DEVICE_IO_STATUS_OK;
>   }
>   
> +void block_job_iostatus_reset(BlockJob *job)
> +{
> +    JOB_LOCK_GUARD();
> +    block_job_iostatus_reset_locked(job);
> +}
> +
>   void block_job_user_resume(Job *job)
>   {
>       BlockJob *bjob = container_of(job, BlockJob, job);
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 6525e16fd5..3959a98612 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -92,6 +92,9 @@ typedef struct BlockJob {
>    */
>   BlockJob *block_job_next(BlockJob *job);
>   
> +/* Same as block_job_next(), but called with job lock held. */
> +BlockJob *block_job_next_locked(BlockJob *job);
> +
>   /**
>    * block_job_get:
>    * @id: The id of the block job.
> @@ -102,6 +105,9 @@ BlockJob *block_job_next(BlockJob *job);
>    */
>   BlockJob *block_job_get(const char *id);
>   
> +/* Same as block_job_get(), but called with job lock held. */
> +BlockJob *block_job_get_locked(const char *id);
> +
>   /**
>    * block_job_add_bdrv:
>    * @job: A block job
> @@ -145,6 +151,9 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState 
> *bs);
>    */
>   bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
>   
> +/* Same as block_job_set_speed(), but called with job lock held. */
> +bool block_job_set_speed_locked(BlockJob *job, int64_t speed, Error **errp);
> +
>   /**
>    * block_job_query:
>    * @job: The job to get information about.
> @@ -153,6 +162,9 @@ bool block_job_set_speed(BlockJob *job, int64_t speed, 
> Error **errp);
>    */
>   BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>   
> +/* Same as block_job_query(), but called with job lock held. */
> +BlockJobInfo *block_job_query_locked(BlockJob *job, Error **errp);
> +
>   /**
>    * block_job_iostatus_reset:
>    * @job: The job whose I/O status should be reset.
> @@ -162,6 +174,9 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>    */
>   void block_job_iostatus_reset(BlockJob *job);
>   
> +/* Same as block_job_iostatus_reset(), but called with job lock held. */
> +void block_job_iostatus_reset_locked(BlockJob *job);
> +
>   /*
>    * block_job_get_aio_context:
>    *


--
Best regards,
Vladimir