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.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/export/export.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/export/export.c b/block/export/export.c
index 28a91c9c42..e3fee60611 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -306,7 +306,7 @@ void blk_exp_close_all_type(BlockExportType type)
blk_exp_request_shutdown(exp);
}
- AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
+ AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type));
}
void blk_exp_close_all(void)
--
2.39.2On 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. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > block/export/export.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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? > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > block/export/export.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/export/export.c b/block/export/export.c > index 28a91c9c42..e3fee60611 100644 > --- a/block/export/export.c > +++ b/block/export/export.c > @@ -306,7 +306,7 @@ void blk_exp_close_all_type(BlockExportType type) > blk_exp_request_shutdown(exp); > } > > - AIO_WAIT_WHILE(NULL, blk_exp_has_type(type)); > + AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type)); > } > > void blk_exp_close_all(void)
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. Stefan
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(). */ \
---
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()
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()
>
© 2016 - 2026 Red Hat, Inc.