Just as done with job.h, create _locked() functions in blockjob.h

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:
  *
-- 
2.31.1

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

Am 05/07/2022 um 17:01 schrieb 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.

There is nothing in the struct (apart from Job) that is protected by the
job lock. I can add a comment "Protected by job mutex" on top of Job job
field?

> 
> 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.

Where did I miss the mutex lock/unlock comments? Yes I forgot the
"temporary unlock" thing but apart from that all functions have a
comment saying if they take the lock or not.

> 
>>
>> 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:
>>    *
> 
> 

On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 05/07/2022 um 17:01 schrieb 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.
> 
> There is nothing in the struct (apart from Job) that is protected by the
> job lock. I can add a comment "Protected by job mutex" on top of Job job
> field?

Yes, I think that's worth doing.

Other fields doesn't need the lock?

> 
>>
>> 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.
> 
> Where did I miss the mutex lock/unlock comments? Yes I forgot the
> "temporary unlock" thing but apart from that all functions have a
> comment saying if they take the lock or not.

Probably that's my impression because you add some comments in separate patches. OK.

> 
>>
>>>
>>> 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

Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy:
> On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 05/07/2022 um 17:01 schrieb 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.
>>
>> There is nothing in the struct (apart from Job) that is protected by the
>> job lock. I can add a comment "Protected by job mutex" on top of Job job
>> field?
> 
> Yes, I think that's worth doing.
> 
> Other fields doesn't need the lock?
> 
Well I didn't plan to actually look at it but now that you ask:

/** needs protection, so it can go under job lock */
BlockDeviceIoStatus iostatus;

/** mostly under lock, not sure when it is called as notifier callback
though. I think they are GLOBAL_STATE, what do you think?  */
int64_t speed;

/** thread safe API */
RateLimit limit;

/** I think it's also thread safe */
Error *blocker;

/* always under job lock */
Notifier finalize_cancelled_notifier;
Notifier finalize_completed_notifier;
Notifier pending_notifier;
Notifier ready_notifier;
Notifier idle_notifier;

Not sure about blockjob->speed though.

Emanuele

Am 06/07/2022 um 14:36 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy:
>> On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote:
>>>
>>>
>>> Am 05/07/2022 um 17:01 schrieb 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.
>>>
>>> There is nothing in the struct (apart from Job) that is protected by the
>>> job lock. I can add a comment "Protected by job mutex" on top of Job job
>>> field?
>>
>> Yes, I think that's worth doing.
>>
>> Other fields doesn't need the lock?
>>
> Well I didn't plan to actually look at it but now that you ask:
> 
> /** needs protection, so it can go under job lock */
> BlockDeviceIoStatus iostatus;
> 
> /** mostly under lock, not sure when it is called as notifier callback
> though. I think they are GLOBAL_STATE, what do you think?  */
> int64_t speed;
> 
> /** thread safe API */
> RateLimit limit;
> 
> /** I think it's also thread safe */
> Error *blocker;
> 
> /* always under job lock */
Actually that's wrong, they are just set once and never modified.

And GSList *nodes; is also always called under GS.

So there's only iostatus to protect and maybe speed.

Emanuele

> Notifier finalize_cancelled_notifier;
> Notifier finalize_completed_notifier;
> Notifier pending_notifier;
> Notifier ready_notifier;
> Notifier idle_notifier;
> 
> Not sure about blockjob->speed though.
> 
> Emanuele
>

On 7/6/22 15:59, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 06/07/2022 um 14:36 schrieb Emanuele Giuseppe Esposito:
>>
>>
>> Am 06/07/2022 um 14:23 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 7/6/22 15:05, Emanuele Giuseppe Esposito wrote:
>>>>
>>>>
>>>> Am 05/07/2022 um 17:01 schrieb 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.
>>>>
>>>> There is nothing in the struct (apart from Job) that is protected by the
>>>> job lock. I can add a comment "Protected by job mutex" on top of Job job
>>>> field?
>>>
>>> Yes, I think that's worth doing.
>>>
>>> Other fields doesn't need the lock?
>>>
>> Well I didn't plan to actually look at it but now that you ask:
>>
>> /** needs protection, so it can go under job lock */
>> BlockDeviceIoStatus iostatus;
>>
>> /** mostly under lock, not sure when it is called as notifier callback
>> though. I think they are GLOBAL_STATE, what do you think?  */
>> int64_t speed;

Hmm I doubt that notifier callbacks are always called from GS code.. But reading .speed to send an event doesn't seem to worth any locking.

>>
>> /** thread safe API */
>> RateLimit limit;
>>
>> /** I think it's also thread safe */
>> Error *blocker;
>>
>> /* always under job lock */
> Actually that's wrong, they are just set once and never modified.
> 
> And GSList *nodes; is also always called under GS.
> 
> So there's only iostatus to protect and maybe speed.
> 




-- 
Best regards,
Vladimir

On Wed, Jun 29, 2022 at 10:15:26AM -0400, Emanuele Giuseppe Esposito wrote:
> +BlockJob *block_job_next(BlockJob *bjob)
>  {
> -    Job *job = job_get(id);
> +    JOB_LOCK_GUARD();
> +    return block_job_next_locked(bjob);
> +}

This seems unsafe for the same reason as job_ref(). How can the caller
be sure bjob is still valid if it doesn't hold the mutex and has no
reference to it?

Maybe the assumption is that the next()/get()/unref() APIs are
GLOBAL_STATE_CODE(), so there can be no race between them?

Am 05/07/2022 um 09:58 schrieb Stefan Hajnoczi:
> On Wed, Jun 29, 2022 at 10:15:26AM -0400, Emanuele Giuseppe Esposito wrote:
>> +BlockJob *block_job_next(BlockJob *bjob)
>>  {
>> -    Job *job = job_get(id);
>> +    JOB_LOCK_GUARD();
>> +    return block_job_next_locked(bjob);
>> +}
> 
> This seems unsafe for the same reason as job_ref(). How can the caller
> be sure bjob is still valid if it doesn't hold the mutex and has no
> reference to it?
> 
> Maybe the assumption is that the next()/get()/unref() APIs are
> GLOBAL_STATE_CODE(), so there can be no race between them?
> 

Same answer as job_ref. Unfortunately if we want to keep this logic in
this serie that's the price to pay (even though it's just till patch 13).
No assumption I would say.

Emanuele