diff mbox series

[v5,09/20] jobs: add job lock in find_* functions

Message ID 20220208143513.1077229-10-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series job: replace AioContext lock with job_mutex | expand

Commit Message

Emanuele Giuseppe Esposito Feb. 8, 2022, 2:35 p.m. UTC 
Both blockdev.c and job-qmp.c have TOC/TOU conditions, because
they first search for the job and then perform an action on it.
Therefore, we need to do the search + action under the same
job mutex critical section.

Note: at this stage, job_{lock/unlock} and job lock guard macros
are *nop*.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 blockdev.c | 14 +++++++++++++-
 job-qmp.c  | 13 ++++++++++++-
 2 files changed, 25 insertions(+), 2 deletions(-)

Comments

Stefan Hajnoczi Feb. 17, 2022, 3 p.m. UTC | #1 
On Tue, Feb 08, 2022 at 09:35:02AM -0500, Emanuele Giuseppe Esposito wrote:
> diff --git a/blockdev.c b/blockdev.c
> index c5fba4d157..08408cd44b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3311,7 +3311,10 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> -/* Get a block job using its ID and acquire its AioContext */
> +/*
> + * Get a block job using its ID and acquire its AioContext.
> + * Returns with job_lock held on success.

The caller needs to deal with unlocking anyway, so maybe ask the caller
to acquire the lock too? That would make the function simpler to reason
about.

> @@ -60,6 +65,7 @@ void qmp_job_cancel(const char *id, Error **errp)
>      trace_qmp_job_cancel(job);
>      job_user_cancel(job, true, errp);
>      aio_context_release(aio_context);
> +    job_unlock();
>  }

Is job_mutex -> AioContext lock ordering correct? I thought the
AioContext must be held before taking the job lock according to "jobs:
remove aiocontext locks since the functions are under BQL"?
Emanuele Giuseppe Esposito Feb. 24, 2022, 12:36 p.m. UTC | #2 
On 17/02/2022 16:00, Stefan Hajnoczi wrote:
> On Tue, Feb 08, 2022 at 09:35:02AM -0500, Emanuele Giuseppe Esposito wrote:
>> diff --git a/blockdev.c b/blockdev.c
>> index c5fba4d157..08408cd44b 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3311,7 +3311,10 @@ out:
>>      aio_context_release(aio_context);
>>  }
>>  
>> -/* Get a block job using its ID and acquire its AioContext */
>> +/*
>> + * Get a block job using its ID and acquire its AioContext.
>> + * Returns with job_lock held on success.
> 
> The caller needs to deal with unlocking anyway, so maybe ask the caller
> to acquire the lock too? That would make the function simpler to reason
> about.

Those aiocontext locks there are going to be removed when job
lock/unlock are enabled, so it is useless to modify the logic now.
It makes sense to apply this logic with job_lock/unlock though.

> 
>> @@ -60,6 +65,7 @@ void qmp_job_cancel(const char *id, Error **errp)
>>      trace_qmp_job_cancel(job);
>>      job_user_cancel(job, true, errp);
>>      aio_context_release(aio_context);
>> +    job_unlock();
>>  }
> 
> Is job_mutex -> AioContext lock ordering correct? I thought the
> AioContext must be held before taking the job lock according to "jobs:
> remove aiocontext locks since the functions are under BQL"?
> 

I think you got it at this point, but anyways: job_lock is nop and when
it will be enabled, aio_context_acquire will go away in the same patch.

Thank you,
Emanuele
diff mbox series

Patch

diff --git a/blockdev.c b/blockdev.c
index c5fba4d157..08408cd44b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3311,7 +3311,10 @@  out:
     aio_context_release(aio_context);
 }
 
-/* Get a block job using its ID and acquire its AioContext */
+/*
+ * Get a block job using its ID and acquire its AioContext.
+ * Returns with job_lock held on success.
+ */
 static BlockJob *find_block_job(const char *id, AioContext **aio_context,
                                 Error **errp)
 {
@@ -3320,12 +3323,14 @@  static BlockJob *find_block_job(const char *id, AioContext **aio_context,
     assert(id != NULL);
 
     *aio_context = NULL;
+    job_lock();
 
     job = block_job_get(id);
 
     if (!job) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
                   "Block job '%s' not found", id);
+        job_unlock();
         return NULL;
     }
 
@@ -3346,6 +3351,7 @@  void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 
     block_job_set_speed(job, speed, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_block_job_cancel(const char *device,
@@ -3372,6 +3378,7 @@  void qmp_block_job_cancel(const char *device,
     job_user_cancel(&job->job, force, errp);
 out:
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_block_job_pause(const char *device, Error **errp)
@@ -3386,6 +3393,7 @@  void qmp_block_job_pause(const char *device, Error **errp)
     trace_qmp_block_job_pause(job);
     job_user_pause(&job->job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_block_job_resume(const char *device, Error **errp)
@@ -3400,6 +3408,7 @@  void qmp_block_job_resume(const char *device, Error **errp)
     trace_qmp_block_job_resume(job);
     job_user_resume(&job->job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_block_job_complete(const char *device, Error **errp)
@@ -3414,6 +3423,7 @@  void qmp_block_job_complete(const char *device, Error **errp)
     trace_qmp_block_job_complete(job);
     job_complete(&job->job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_block_job_finalize(const char *id, Error **errp)
@@ -3437,6 +3447,7 @@  void qmp_block_job_finalize(const char *id, Error **errp)
     aio_context = block_job_get_aio_context(job);
     job_unref(&job->job);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_block_job_dismiss(const char *id, Error **errp)
@@ -3453,6 +3464,7 @@  void qmp_block_job_dismiss(const char *id, Error **errp)
     job = &bjob->job;
     job_dismiss(&job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_change_backing_file(const char *device,
diff --git a/job-qmp.c b/job-qmp.c
index 06f970f6cf..a08cd7dd36 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -29,16 +29,21 @@ 
 #include "qapi/error.h"
 #include "trace/trace-root.h"
 
-/* Get a job using its ID and acquire its AioContext */
+/*
+ * Get a block job using its ID and acquire its AioContext.
+ * Returns with job_lock held on success.
+ */
 static Job *find_job(const char *id, AioContext **aio_context, Error **errp)
 {
     Job *job;
 
     *aio_context = NULL;
+    job_lock();
 
     job = job_get(id);
     if (!job) {
         error_setg(errp, "Job not found");
+        job_unlock();
         return NULL;
     }
 
@@ -60,6 +65,7 @@  void qmp_job_cancel(const char *id, Error **errp)
     trace_qmp_job_cancel(job);
     job_user_cancel(job, true, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_job_pause(const char *id, Error **errp)
@@ -74,6 +80,7 @@  void qmp_job_pause(const char *id, Error **errp)
     trace_qmp_job_pause(job);
     job_user_pause(job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_job_resume(const char *id, Error **errp)
@@ -88,6 +95,7 @@  void qmp_job_resume(const char *id, Error **errp)
     trace_qmp_job_resume(job);
     job_user_resume(job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_job_complete(const char *id, Error **errp)
@@ -102,6 +110,7 @@  void qmp_job_complete(const char *id, Error **errp)
     trace_qmp_job_complete(job);
     job_complete(job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_job_finalize(const char *id, Error **errp)
@@ -125,6 +134,7 @@  void qmp_job_finalize(const char *id, Error **errp)
     aio_context = job->aio_context;
     job_unref(job);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 void qmp_job_dismiss(const char *id, Error **errp)
@@ -139,6 +149,7 @@  void qmp_job_dismiss(const char *id, Error **errp)
     trace_qmp_job_dismiss(job);
     job_dismiss(&job, errp);
     aio_context_release(aio_context);
+    job_unlock();
 }
 
 static JobInfo *job_query_single(Job *job, Error **errp)