| Message ID | 20230301205801.2453491-3-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible | expand |
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() >
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. > > 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>
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)
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(-)