Re: [PATCH 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

On Thu, Mar 02, 2023 at 03:16:32PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/3/23 14:08, Stefan Hajnoczi wrote:
> > On Thu, Mar 02, 2023 at 11:36:03AM +0100, Philippe Mathieu-Daudé wrote:
> > > On 1/3/23 21:57, Stefan Hajnoczi wrote:
> > > > There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
> > > > instead of AIO_WAIT_WHILE() to document that this code has already been
> > > > audited and converted. The AioContext argument is already NULL so
> > > > aio_context_release() is never called anyway.
> > > 
> > > Shouldn't we assert(ctx && unlock) in AIO_WAIT_WHILE_INTERNAL() then?
> > 
> > Can you show where you'd add that assertion? It's not clear to me what
> > the purpose is.
> 
> Without your series applied, using:
> 
> -- >8 --
> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> index dd9a7f6461..dc372e4c16 100644
> --- a/include/block/aio-wait.h
> +++ b/include/block/aio-wait.h
> @@ -82,6 +82,8 @@ extern AioWait global_aio_wait;
>      bool waited_ = false;                                          \
>      AioWait *wait_ = &global_aio_wait;                             \
>      AioContext *ctx_ = (ctx);                                      \
> +    assert("Use AIO_WAIT_WHILE_UNLOCKED()" && !unlock              \
> +           || (ctx && strcmp(#ctx, "qemu_get_aio_context()")));    \
>      /* Increment wait_->num_waiters before evaluating cond. */     \
>      qatomic_inc(&wait_->num_waiters);                              \
>      /* Paired with smp_mb in aio_wait_kick(). */                   \
> ---

Ah, I see. You are suggesting adding an assertion to catch
AIO_WAIT_WHILE() usage in cases where AIO_WAIT_WHILE_UNLOCKED() should
be used instead.

I think it's a bit too clever, especially the strcmp trick, but we can
add it as the final patch. I have grepped the code and don't think there
are any remaining instances where the assertion fails.

Stefan

> I get:
> 
> Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || (((void*)0) &&
> strcmp("((void*)0)", "qemu_get_aio_context()"))), function
> blk_exp_close_all_type, file export.c, line 309.
> 
> -> [PATCH 2/6] block: convert blk_exp_close_all_type() to
> AIO_WAIT_WHILE_UNLOCKED()
> 
> Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 ||
> (qemu_get_aio_context() && strcmp("qemu_get_aio_context()",
> "qemu_get_aio_context()"))), function bdrv_graph_wrlock, file graph-lock.c,
> line 130.
> 
> -> [PATCH 3/6] block: convert bdrv_graph_wrlock() to
> AIO_WAIT_WHILE_UNLOCKED()
> 
> Assertion failed: ("Use AIO_WAIT_WHILE_UNLOCKED()" && !1 || (((void*)0) &&
> strcmp("((void*)0)", "qemu_get_aio_context()"))), function
> bdrv_drain_all_begin, file io.c, line 523.
> 
> -> [PATCH 4/6] block: convert bdrv_drain_all_begin() to
> AIO_WAIT_WHILE_UNLOCKED()
>

signature.asc
Description: PGP signature