[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from Blo
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/4] block: extract AIO_WAIT_WHILE() from BlockDriverState |
Date: |
Wed, 14 Feb 2018 14:06:47 +0000 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
On Tue, Feb 13, 2018 at 10:01:06AM -0600, Eric Blake wrote:
> Trying to understand here:
>
>
> > +#define AIO_WAIT_WHILE(wait, ctx, cond) ({ \
> > + bool waited_ = false; \
> > + bool busy_ = true; \
> > + AioWait *wait_ = (wait); \
> > + AioContext *ctx_ = (ctx); \
> > + if (aio_context_in_iothread(ctx_)) { \
> > + while ((cond) || busy_) { \
> > + busy_ = aio_poll(ctx_, (cond)); \
> > + waited_ |= !!(cond) | busy_; \
> > + } \
>
> If we are in an iothread already, we never dereference wait,
No, the name and documentation for aio_context_in_iothread() is
misleading. It actually means "does this AioContext belong to the
current thread?", which is more general than just the IOThread case.
aio_context_in_iothread() returns true when:
1. We are the IOThread that owns ctx. <-- the case you thought of
2. We are the main loop and ctx == qemu_get_aio_context().
^--- the sneaky case that BDRV_POLL_WHILE() has always relied on
> > + } else { \
> > + assert(qemu_get_current_aio_context() == \
> > + qemu_get_aio_context()); \
> > + assert(!wait_->need_kick); \
>
> but if we are in the main loop, wait must be non-NULL.
The else statement only handles the case where the main loop is waiting
for an IOThread AioContext.
s/in the main loop/in the main loop and ctx is an IOThread AioContext/
>
> > +++ b/include/block/block.h
> > @@ -2,6 +2,7 @@
> > #define BLOCK_H
> > #include "block/aio.h"
> > +#include "block/aio-wait.h"
> > #include "qapi-types.h"
> > #include "qemu/iov.h"
> > #include "qemu/coroutine.h"
> > @@ -367,41 +368,14 @@ void bdrv_drain_all_begin(void);
> > void bdrv_drain_all_end(void);
> > void bdrv_drain_all(void);
> > +/* Returns NULL when bs == NULL */
> > +AioWait *bdrv_get_aio_wait(BlockDriverState *bs);
>
> This can return NULL, so it is only ever safe to use in an iothread; because
> if it is used in the main loop,...
>
> > +
> > #define BDRV_POLL_WHILE(bs, cond) ({ \
>
> > + AIO_WAIT_WHILE(bdrv_get_aio_wait(bs_), \
> > + bdrv_get_aio_context(bs_), \
> > + cond); })
>
> ...we can pass NULL as the wait parameter, which will crash.
It won't crash since if (aio_context_in_iothread(ctx_)) will take the true
case when bs_ == NULL.
> > +++ b/block/io.c
>
> > void bdrv_wakeup(BlockDriverState *bs)
> > {
> > - /* The barrier (or an atomic op) is in the caller. */
> > - if (atomic_read(&bs->wakeup)) {
> > - aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, NULL);
> > - }
> > + aio_wait_kick(bdrv_get_aio_wait(bs));
>
> this is another case where passing NULL...
bdrv_wakeup() is only called when bs != NULL.
I hope this explains things! The main issue that raised these questions
was that aio_context_in_iothread() has a misleading name. Shall we
rename it?
I'm having a hard time picking a new name because it must not be
confused with AioContext acquire/release, which doesn't influence the
"native" AioContext that the current thread has an affinity with.
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH v2 3/4] block: test blk_aio_flush() with blk->root == NULL, Stefan Hajnoczi, 2018/02/13
[Qemu-devel] [PATCH v2 4/4] Revert "IDE: Do not flush empty CDROM drives", Stefan Hajnoczi, 2018/02/13
Re: [Qemu-devel] [PATCH v2 0/4] block: fix blk_aio_*() segfault when blk->root == NULL, Stefan Hajnoczi, 2018/02/15