qemu-devel
[Top][All Lists]
Advanced

[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.

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]