Re: [PATCH v5 06/20] jobs: remove aiocontext locks since the functions are under BQL

[Stefan Hajnoczi]

On Tue, Feb 08, 2022 at 09:34:59AM -0500, 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 must be taken inside the AioContext
> lock, and taking it outside would cause 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 8cac3d739c..e315466914 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3713,15 +3713,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>  
>      for (job = block_job_next(NULL); job; job = block_job_next(job)) {

I'm confused. block_job_next() is supposed to be called with job_mutex
held since it iterates the jobs list.

The patch series might fix this later on but it's hard to review patches
with broken invariants.

Does this mean git-bisect(1) is broken since intermediate commits are
not thread-safe?

>          BlockJobInfo *value;
> -        AioContext *aio_context;
>  
>          if (block_job_is_internal(job)) {
>              continue;
>          }
> -        aio_context = block_job_get_aio_context(job);
> -        aio_context_acquire(aio_context);
>          value = block_job_query(job, errp);

This function accesses fields that are protected by job_mutex, which we
don't hold.

signature.asc
Description: PGP signature