| Message ID | 20230301205801.2453491-2-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible | expand |
Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > There is no need for the AioContext lock in bdrv_drain_all() because > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > the future it can be removed. It can be removed for all callers that run in the main loop context. For code running in an iothread, it's still important to pass a non-NULL context. This makes me doubt that the ctx parameter can really be removed without changing more. Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and to poll qemu_get_current_aio_context() instead of ctx_ or the main context? > There is an assertion in > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and > we would lose that check by dropping the argument. However, that was a > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a > duplicate check. So I think we won't lose much by dropping it, but let's > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to > confirm this is the case. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Yes, it seems that we don't lose much, except maybe some consistency in the intermediate state. The commit message could state a bit more directly what we gain, though. Since you mention removing the parameter as a future possibility, I assume that's the goal with it, but I wouldn't be sure just from reading the commit message. Kevin
On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote: > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > > There is no need for the AioContext lock in bdrv_drain_all() because > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > > the future it can be removed. > > It can be removed for all callers that run in the main loop context. For > code running in an iothread, it's still important to pass a non-NULL > context. This makes me doubt that the ctx parameter can really be > removed without changing more. > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and > to poll qemu_get_current_aio_context() instead of ctx_ or the main > context? This is what I'd like once everything has been converted to AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it AIO_WAIT_WHILE() again: #define AIO_WAIT_WHILE(cond) ({ \ bool waited_ = false; \ AioWait *wait_ = &global_aio_wait; \ /* Increment wait_->num_waiters before evaluating cond. */ \ qatomic_inc(&wait_->num_waiters); \ /* Paired with smp_mb in aio_wait_kick(). */ \ smp_mb(); \ while ((cond)) { \ aio_poll(qemu_get_current_aio_context(), true); \ waited_ = true; \ } \ qatomic_dec(&wait_->num_waiters); \ waited_; }) However, I just realized this only works in the main loop thread because that's where aio_wait_kick() notifications are received. An IOThread running AIO_WAIT_WHILE() won't be woken when another thread (including the main loop thread) calls aio_wait_kick(). I would propose introducing a QemuCond for each condition that we wait on, but QemuCond lacks event loop integration. The current thread would be unable to run aio_poll() while also waiting on a QemuCond. Life outside coroutines is hard, man! I need to think about this more. Luckily this problem doesn't block this patch series. > > There is an assertion in > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and > > we would lose that check by dropping the argument. However, that was a > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a > > duplicate check. So I think we won't lose much by dropping it, but let's > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to > > confirm this is the case. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Yes, it seems that we don't lose much, except maybe some consistency in > the intermediate state. The commit message could state a bit more > directly what we gain, though. Since you mention removing the parameter > as a future possibility, I assume that's the goal with it, but I > wouldn't be sure just from reading the commit message. AIO_WAIT_WHILE() callers need to be weened of the AioContext lock. That's the main motivation and this patch series converts the easy cases where we already don't need the lock. Dropping the function argument eventually is a side benefit. Stefan
Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben: > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote: > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > > > There is no need for the AioContext lock in bdrv_drain_all() because > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > > > the future it can be removed. > > > > It can be removed for all callers that run in the main loop context. For > > code running in an iothread, it's still important to pass a non-NULL > > context. This makes me doubt that the ctx parameter can really be > > removed without changing more. > > > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and > > to poll qemu_get_current_aio_context() instead of ctx_ or the main > > context? > > This is what I'd like once everything has been converted to > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it > AIO_WAIT_WHILE() again: > > #define AIO_WAIT_WHILE(cond) ({ \ > bool waited_ = false; \ > AioWait *wait_ = &global_aio_wait; \ > /* Increment wait_->num_waiters before evaluating cond. */ \ > qatomic_inc(&wait_->num_waiters); \ > /* Paired with smp_mb in aio_wait_kick(). */ \ > smp_mb(); \ > while ((cond)) { \ > aio_poll(qemu_get_current_aio_context(), true); \ > waited_ = true; \ > } \ > qatomic_dec(&wait_->num_waiters); \ > waited_; }) Ok, yes, this is what I tried to describe above. > However, I just realized this only works in the main loop thread because > that's where aio_wait_kick() notifications are received. An IOThread > running AIO_WAIT_WHILE() won't be woken when another thread (including > the main loop thread) calls aio_wait_kick(). Which is of course a limitation we already have today. You can wait for things in your own iothread, or for all threads from the main loop. However, in the future multiqueue world, the first case probably becomes pretty much useless because even for the same node, you could get activity in any thread. So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is probably a good idea anyway, but I'm not entirely sure how many places we currently have where it's called from an iothread. I know the drain in mirror_run(), but Emanuele already had a patch in his queue where bdrv_co_yield_to_drain() schedules drain in the main context, so if that works, mirror_run() would be solved. https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad > I would propose introducing a QemuCond for each condition that we wait > on, but QemuCond lacks event loop integration. The current thread would > be unable to run aio_poll() while also waiting on a QemuCond. > > Life outside coroutines is hard, man! I need to think about this more. > Luckily this problem doesn't block this patch series. I hope that we don't really need all of this if we can limit running synchronous code to the main loop. > > > There is an assertion in > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and > > > we would lose that check by dropping the argument. However, that was a > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a > > > duplicate check. So I think we won't lose much by dropping it, but let's > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to > > > confirm this is the case. > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > Yes, it seems that we don't lose much, except maybe some consistency in > > the intermediate state. The commit message could state a bit more > > directly what we gain, though. Since you mention removing the parameter > > as a future possibility, I assume that's the goal with it, but I > > wouldn't be sure just from reading the commit message. > > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock. > That's the main motivation and this patch series converts the easy > cases where we already don't need the lock. Dropping the function > argument eventually is a side benefit. Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with ctx instead of NULL. So moving to NULL is a separate change that needs a separate explanation. You could even argue that it should be a separate patch if it's an independent change. Or am I missing something and keeping ctx would actually break things? Kevin
On Wed, Mar 08, 2023 at 09:48:17AM +0100, Kevin Wolf wrote: > Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben: > > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote: > > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > > > > There is no need for the AioContext lock in bdrv_drain_all() because > > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > > > > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > > > > the future it can be removed. > > > > > > It can be removed for all callers that run in the main loop context. For > > > code running in an iothread, it's still important to pass a non-NULL > > > context. This makes me doubt that the ctx parameter can really be > > > removed without changing more. > > > > > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and > > > to poll qemu_get_current_aio_context() instead of ctx_ or the main > > > context? > > > > This is what I'd like once everything has been converted to > > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it > > AIO_WAIT_WHILE() again: > > > > #define AIO_WAIT_WHILE(cond) ({ \ > > bool waited_ = false; \ > > AioWait *wait_ = &global_aio_wait; \ > > /* Increment wait_->num_waiters before evaluating cond. */ \ > > qatomic_inc(&wait_->num_waiters); \ > > /* Paired with smp_mb in aio_wait_kick(). */ \ > > smp_mb(); \ > > while ((cond)) { \ > > aio_poll(qemu_get_current_aio_context(), true); \ > > waited_ = true; \ > > } \ > > qatomic_dec(&wait_->num_waiters); \ > > waited_; }) > > Ok, yes, this is what I tried to describe above. > > > However, I just realized this only works in the main loop thread because > > that's where aio_wait_kick() notifications are received. An IOThread > > running AIO_WAIT_WHILE() won't be woken when another thread (including > > the main loop thread) calls aio_wait_kick(). > > Which is of course a limitation we already have today. You can wait for > things in your own iothread, or for all threads from the main loop. > > However, in the future multiqueue world, the first case probably becomes > pretty much useless because even for the same node, you could get > activity in any thread. > > So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is > probably a good idea anyway, but I'm not entirely sure how many places > we currently have where it's called from an iothread. I know the drain > in mirror_run(), but Emanuele already had a patch in his queue where > bdrv_co_yield_to_drain() schedules drain in the main context, so if that > works, mirror_run() would be solved. > > https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad > > > I would propose introducing a QemuCond for each condition that we wait > > on, but QemuCond lacks event loop integration. The current thread would > > be unable to run aio_poll() while also waiting on a QemuCond. > > > > Life outside coroutines is hard, man! I need to think about this more. > > Luckily this problem doesn't block this patch series. > > I hope that we don't really need all of this if we can limit running > synchronous code to the main loop. Great idea, I think you're right. I'll audit the code to find the IOThread AIO_WAIT_WHILE() callers and maybe a future patch series can work on that. > > > > There is an assertion in > > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and > > > > we would lose that check by dropping the argument. However, that was a > > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a > > > > duplicate check. So I think we won't lose much by dropping it, but let's > > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to > > > > confirm this is the case. > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > > > Yes, it seems that we don't lose much, except maybe some consistency in > > > the intermediate state. The commit message could state a bit more > > > directly what we gain, though. Since you mention removing the parameter > > > as a future possibility, I assume that's the goal with it, but I > > > wouldn't be sure just from reading the commit message. > > > > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock. > > That's the main motivation and this patch series converts the easy > > cases where we already don't need the lock. Dropping the function > > argument eventually is a side benefit. > > Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with > ctx instead of NULL. So moving to NULL is a separate change that needs a > separate explanation. You could even argue that it should be a separate > patch if it's an independent change. > > Or am I missing something and keeping ctx would actually break things? Yes, ctx argument does not need to be modified when converting from AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED(). Passing it bothers me because we don't really use it when unlock=false. Would you like me to keep ctx non-NULL for now? Stefan
Am 08.03.2023 um 15:26 hat Stefan Hajnoczi geschrieben: > On Wed, Mar 08, 2023 at 09:48:17AM +0100, Kevin Wolf wrote: > > Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben: > > > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote: > > > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > > > > > There is no need for the AioContext lock in bdrv_drain_all() because > > > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > > > > > > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > > > > > the future it can be removed. > > > > > > > > It can be removed for all callers that run in the main loop context. For > > > > code running in an iothread, it's still important to pass a non-NULL > > > > context. This makes me doubt that the ctx parameter can really be > > > > removed without changing more. > > > > > > > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and > > > > to poll qemu_get_current_aio_context() instead of ctx_ or the main > > > > context? > > > > > > This is what I'd like once everything has been converted to > > > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it > > > AIO_WAIT_WHILE() again: > > > > > > #define AIO_WAIT_WHILE(cond) ({ \ > > > bool waited_ = false; \ > > > AioWait *wait_ = &global_aio_wait; \ > > > /* Increment wait_->num_waiters before evaluating cond. */ \ > > > qatomic_inc(&wait_->num_waiters); \ > > > /* Paired with smp_mb in aio_wait_kick(). */ \ > > > smp_mb(); \ > > > while ((cond)) { \ > > > aio_poll(qemu_get_current_aio_context(), true); \ > > > waited_ = true; \ > > > } \ > > > qatomic_dec(&wait_->num_waiters); \ > > > waited_; }) > > > > Ok, yes, this is what I tried to describe above. > > > > > However, I just realized this only works in the main loop thread because > > > that's where aio_wait_kick() notifications are received. An IOThread > > > running AIO_WAIT_WHILE() won't be woken when another thread (including > > > the main loop thread) calls aio_wait_kick(). > > > > Which is of course a limitation we already have today. You can wait for > > things in your own iothread, or for all threads from the main loop. > > > > However, in the future multiqueue world, the first case probably becomes > > pretty much useless because even for the same node, you could get > > activity in any thread. > > > > So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is > > probably a good idea anyway, but I'm not entirely sure how many places > > we currently have where it's called from an iothread. I know the drain > > in mirror_run(), but Emanuele already had a patch in his queue where > > bdrv_co_yield_to_drain() schedules drain in the main context, so if that > > works, mirror_run() would be solved. > > > > https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad > > > > > I would propose introducing a QemuCond for each condition that we wait > > > on, but QemuCond lacks event loop integration. The current thread would > > > be unable to run aio_poll() while also waiting on a QemuCond. > > > > > > Life outside coroutines is hard, man! I need to think about this more. > > > Luckily this problem doesn't block this patch series. > > > > I hope that we don't really need all of this if we can limit running > > synchronous code to the main loop. > > Great idea, I think you're right. > > I'll audit the code to find the IOThread AIO_WAIT_WHILE() callers and > maybe a future patch series can work on that. > > > > > > There is an assertion in > > > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and > > > > > we would lose that check by dropping the argument. However, that was a > > > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a > > > > > duplicate check. So I think we won't lose much by dropping it, but let's > > > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to > > > > > confirm this is the case. > > > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > > > > > Yes, it seems that we don't lose much, except maybe some consistency in > > > > the intermediate state. The commit message could state a bit more > > > > directly what we gain, though. Since you mention removing the parameter > > > > as a future possibility, I assume that's the goal with it, but I > > > > wouldn't be sure just from reading the commit message. > > > > > > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock. > > > That's the main motivation and this patch series converts the easy > > > cases where we already don't need the lock. Dropping the function > > > argument eventually is a side benefit. > > > > Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with > > ctx instead of NULL. So moving to NULL is a separate change that needs a > > separate explanation. You could even argue that it should be a separate > > patch if it's an independent change. > > > > Or am I missing something and keeping ctx would actually break things? > > Yes, ctx argument does not need to be modified when converting from > AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED(). Passing it bothers me > because we don't really use it when unlock=false. > > Would you like me to keep ctx non-NULL for now? I don't really mind doing both changes in one commit because they are so small, but at least I'd like the commit message to be more explicit about the eventual goal we have with switching to NULL instead of just stating that it's odd, but harmless. Kevin
On Wed, Mar 08, 2023 at 06:25:43PM +0100, Kevin Wolf wrote: > Am 08.03.2023 um 15:26 hat Stefan Hajnoczi geschrieben: > > On Wed, Mar 08, 2023 at 09:48:17AM +0100, Kevin Wolf wrote: > > > Am 07.03.2023 um 20:20 hat Stefan Hajnoczi geschrieben: > > > > On Tue, Mar 07, 2023 at 06:17:22PM +0100, Kevin Wolf wrote: > > > > > Am 01.03.2023 um 21:57 hat Stefan Hajnoczi geschrieben: > > > > > > There is no need for the AioContext lock in bdrv_drain_all() because > > > > > > nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. > > > > > > > > > > > > Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In > > > > > > the future it can be removed. > > > > > > > > > > It can be removed for all callers that run in the main loop context. For > > > > > code running in an iothread, it's still important to pass a non-NULL > > > > > context. This makes me doubt that the ctx parameter can really be > > > > > removed without changing more. > > > > > > > > > > Is your plan to remove the if from AIO_WAIT_WHILE_INTERNAL(), too, and > > > > > to poll qemu_get_current_aio_context() instead of ctx_ or the main > > > > > context? > > > > > > > > This is what I'd like once everything has been converted to > > > > AIO_WAIT_WHILE_UNLOCKED() - and at this point we might as well call it > > > > AIO_WAIT_WHILE() again: > > > > > > > > #define AIO_WAIT_WHILE(cond) ({ \ > > > > bool waited_ = false; \ > > > > AioWait *wait_ = &global_aio_wait; \ > > > > /* Increment wait_->num_waiters before evaluating cond. */ \ > > > > qatomic_inc(&wait_->num_waiters); \ > > > > /* Paired with smp_mb in aio_wait_kick(). */ \ > > > > smp_mb(); \ > > > > while ((cond)) { \ > > > > aio_poll(qemu_get_current_aio_context(), true); \ > > > > waited_ = true; \ > > > > } \ > > > > qatomic_dec(&wait_->num_waiters); \ > > > > waited_; }) > > > > > > Ok, yes, this is what I tried to describe above. > > > > > > > However, I just realized this only works in the main loop thread because > > > > that's where aio_wait_kick() notifications are received. An IOThread > > > > running AIO_WAIT_WHILE() won't be woken when another thread (including > > > > the main loop thread) calls aio_wait_kick(). > > > > > > Which is of course a limitation we already have today. You can wait for > > > things in your own iothread, or for all threads from the main loop. > > > > > > However, in the future multiqueue world, the first case probably becomes > > > pretty much useless because even for the same node, you could get > > > activity in any thread. > > > > > > So essentially AIO_WAIT_WHILE() becomes GLOBAL_STATE_CODE(). Which is > > > probably a good idea anyway, but I'm not entirely sure how many places > > > we currently have where it's called from an iothread. I know the drain > > > in mirror_run(), but Emanuele already had a patch in his queue where > > > bdrv_co_yield_to_drain() schedules drain in the main context, so if that > > > works, mirror_run() would be solved. > > > > > > https://gitlab.com/eesposit/qemu/-/commit/63562351aca4fb05d5711eb410feb96e64b5d4ad > > > > > > > I would propose introducing a QemuCond for each condition that we wait > > > > on, but QemuCond lacks event loop integration. The current thread would > > > > be unable to run aio_poll() while also waiting on a QemuCond. > > > > > > > > Life outside coroutines is hard, man! I need to think about this more. > > > > Luckily this problem doesn't block this patch series. > > > > > > I hope that we don't really need all of this if we can limit running > > > synchronous code to the main loop. > > > > Great idea, I think you're right. > > > > I'll audit the code to find the IOThread AIO_WAIT_WHILE() callers and > > maybe a future patch series can work on that. > > > > > > > > There is an assertion in > > > > > > AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and > > > > > > we would lose that check by dropping the argument. However, that was a > > > > > > precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a > > > > > > duplicate check. So I think we won't lose much by dropping it, but let's > > > > > > do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to > > > > > > confirm this is the case. > > > > > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > > > > > > > Yes, it seems that we don't lose much, except maybe some consistency in > > > > > the intermediate state. The commit message could state a bit more > > > > > directly what we gain, though. Since you mention removing the parameter > > > > > as a future possibility, I assume that's the goal with it, but I > > > > > wouldn't be sure just from reading the commit message. > > > > > > > > AIO_WAIT_WHILE() callers need to be weened of the AioContext lock. > > > > That's the main motivation and this patch series converts the easy > > > > cases where we already don't need the lock. Dropping the function > > > > argument eventually is a side benefit. > > > > > > Yes, but the conversion to AIO_WAIT_WHILE_UNLOCKED() could be done with > > > ctx instead of NULL. So moving to NULL is a separate change that needs a > > > separate explanation. You could even argue that it should be a separate > > > patch if it's an independent change. > > > > > > Or am I missing something and keeping ctx would actually break things? > > > > Yes, ctx argument does not need to be modified when converting from > > AIO_WAIT_WHILE() to AIO_WAIT_WHILE_UNLOCKED(). Passing it bothers me > > because we don't really use it when unlock=false. > > > > Would you like me to keep ctx non-NULL for now? > > I don't really mind doing both changes in one commit because they are so > small, but at least I'd like the commit message to be more explicit > about the eventual goal we have with switching to NULL instead of just > stating that it's odd, but harmless. Got it, I'll send another revision. Stefan
diff --git a/block/block-backend.c b/block/block-backend.c index 278b04ce69..d2b6b3652d 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -1835,14 +1835,8 @@ void blk_drain_all(void) bdrv_drain_all_begin(); while ((blk = blk_all_next(blk)) != NULL) { - AioContext *ctx = blk_get_aio_context(blk); - - aio_context_acquire(ctx); - /* We may have -ENOMEDIUM completions in flight */ - AIO_WAIT_WHILE(ctx, qatomic_mb_read(&blk->in_flight) > 0); - - aio_context_release(ctx); + AIO_WAIT_WHILE_UNLOCKED(NULL, qatomic_mb_read(&blk->in_flight) > 0); } bdrv_drain_all_end();
There is no need for the AioContext lock in bdrv_drain_all() because nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic. Note that the NULL AioContext argument to AIO_WAIT_WHILE() is odd. In the future it can be removed. There is an assertion in AIO_WAIT_WHILE() that checks that we're in the main loop AioContext and we would lose that check by dropping the argument. However, that was a precursor to the GLOBAL_STATE_CODE()/IO_CODE() macros and is now a duplicate check. So I think we won't lose much by dropping it, but let's do a few more AIO_WAIT_WHILE_UNLOCKED() coversions of this sort to confirm this is the case. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- block/block-backend.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)