In preparation to the job_lock/unlock patch, remove these
aiocontext locks.
The main reason these two locks are removed here is because
they are inside a loop iterating on the jobs list. Once the
job_lock is added, it will have to protect the whole loop,
wrapping also the aiocontext acquire/release.

We don't want this, as job_lock can only be *wrapped by*
the aiocontext lock, and not vice-versa, to avoid deadlocks.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 blockdev.c | 4 ----
 job-qmp.c  | 4 ----
 2 files changed, 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d9bf842a81..67b55eec11 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3719,15 +3719,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
 
     for (job = block_job_next(NULL); job; job = block_job_next(job)) {
         BlockJobInfo *value;
-        AioContext *aio_context;
 
         if (block_job_is_internal(job)) {
             continue;
         }
-        aio_context = blk_get_aio_context(job->blk);
-        aio_context_acquire(aio_context);
         value = block_job_query(job, errp);
-        aio_context_release(aio_context);
         if (!value) {
             qapi_free_BlockJobInfoList(head);
             return NULL;
diff --git a/job-qmp.c b/job-qmp.c
index 829a28aa70..a6774aaaa5 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -173,15 +173,11 @@ JobInfoList *qmp_query_jobs(Error **errp)
 
     for (job = job_next(NULL); job; job = job_next(job)) {
         JobInfo *value;
-        AioContext *aio_context;
 
         if (job_is_internal(job)) {
             continue;
         }
-        aio_context = job->aio_context;
-        aio_context_acquire(aio_context);
         value = job_query_single(job, errp);
-        aio_context_release(aio_context);
         if (!value) {
             qapi_free_JobInfoList(head);
             return NULL;
-- 
2.27.0

Hmm. subject looks like it's safe to remove aiocontext locks from any qmp command?

For example, commit 91005a495e228eb added aiocontext locks back to qmp bitmap add/remove commands because otherwise they crashed.

So, I'm not sure that being under BQL is enough to drop aiocontext locking..


29.10.2021 19:39, Emanuele Giuseppe Esposito wrote:
> In preparation to the job_lock/unlock patch, remove these
> aiocontext locks.
> The main reason these two locks are removed here is because
> they are inside a loop iterating on the jobs list. Once the
> job_lock is added, it will have to protect the whole loop,
> wrapping also the aiocontext acquire/release.
> 
> We don't want this, as job_lock can only be *wrapped by*
> the aiocontext lock, and not vice-versa, to avoid deadlocks.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   blockdev.c | 4 ----
>   job-qmp.c  | 4 ----
>   2 files changed, 8 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index d9bf842a81..67b55eec11 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3719,15 +3719,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>   
>       for (job = block_job_next(NULL); job; job = block_job_next(job)) {
>           BlockJobInfo *value;
> -        AioContext *aio_context;
>   
>           if (block_job_is_internal(job)) {
>               continue;
>           }
> -        aio_context = blk_get_aio_context(job->blk);
> -        aio_context_acquire(aio_context);
>           value = block_job_query(job, errp);
> -        aio_context_release(aio_context);
>           if (!value) {
>               qapi_free_BlockJobInfoList(head);
>               return NULL;
> diff --git a/job-qmp.c b/job-qmp.c
> index 829a28aa70..a6774aaaa5 100644
> --- a/job-qmp.c
> +++ b/job-qmp.c
> @@ -173,15 +173,11 @@ JobInfoList *qmp_query_jobs(Error **errp)
>   
>       for (job = job_next(NULL); job; job = job_next(job)) {
>           JobInfo *value;
> -        AioContext *aio_context;
>   
>           if (job_is_internal(job)) {
>               continue;
>           }
> -        aio_context = job->aio_context;
> -        aio_context_acquire(aio_context);
>           value = job_query_single(job, errp);
> -        aio_context_release(aio_context);
>           if (!value) {
>               qapi_free_JobInfoList(head);
>               return NULL;
> 


-- 
Best regards,
Vladimir

On 02/11/2021 13:41, Vladimir Sementsov-Ogievskiy wrote:
> Hmm. subject looks like it's safe to remove aiocontext locks from any 
> qmp command?
> 
> For example, commit 91005a495e228eb added aiocontext locks back to qmp 
> bitmap add/remove commands because otherwise they crashed.
> 
> So, I'm not sure that being under BQL is enough to drop aiocontext 
> locking..
> 
> 

In this specific case the aiocontext is useless there. I am not sure 
what it was used for before, but if you look at what it protects in this 
patch we have:

- block_job_query: not called by anyone else, and internally calls:
	* block_job_is_internal = checks struct job id, that is only written at 
job initialization
	* progress_get_snapshot = we made it thread safe, as you might remember
	* accesses struct job fields, that we protect anyways with job_mutex. 
Anyways the aiocontext is not used to protect jobs fields.

- job_query_single: same as block_job_query, calls progress_get_snapshot 
and accesses job fields.

So yes, I am positive that here removing the aiocontext lock does not 
break anything.

Thank you,
Emanuele