| Message ID | 20180306105354.15197-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: make BDRV_POLL_WHILE() re-entrancy safe | expand |
On 06/03/2018 11:53, Stefan Hajnoczi wrote: > Nested BDRV_POLL_WHILE() calls can occur. Currently > assert(!bs_->wakeup) will fail when this happens. > > This patch converts bs->wakeup from bool to a counter. > > Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate > the condition again after the inner caller completes (invoking the inner > caller counts as aio_poll() progress). > > Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > Fu Weiwei: Please retest and let us know if this fixes the assertion > failure you hit. Thanks! > --- > include/block/block.h | 7 +++---- > include/block/block_int.h | 2 +- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/include/block/block.h b/include/block/block.h > index fac401ba3e..990b97f0ad 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -385,9 +385,8 @@ void bdrv_drain_all(void); > * other I/O threads' AioContexts (see for example \ > * block_job_defer_to_main_loop for how to do it). \ > */ \ > - assert(!bs_->wakeup); \ > - /* Set bs->wakeup before evaluating cond. */ \ > - atomic_mb_set(&bs_->wakeup, true); \ > + /* Increment bs->wakeup before evaluating cond. */ \ > + atomic_inc(&bs_->wakeup); \ > while (busy_) { \ > if ((cond)) { \ > waited_ = busy_ = true; \ > @@ -399,7 +398,7 @@ void bdrv_drain_all(void); > waited_ |= busy_; \ > } \ > } \ > - atomic_set(&bs_->wakeup, false); \ > + atomic_dec(&bs_->wakeup); \ > } \ > waited_; }) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 5ea63f8fa8..0f360c0ed5 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -712,7 +712,7 @@ struct BlockDriverState { > /* Internal to BDRV_POLL_WHILE and bdrv_wakeup. Accessed with atomic > * ops. > */ > - bool wakeup; > + unsigned wakeup; > > /* counter for nested bdrv_io_plug. > * Accessed with atomic ops. > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> At least, the assertion made it fail fast. :) Paolo
Am 06.03.2018 um 11:53 hat Stefan Hajnoczi geschrieben: > Nested BDRV_POLL_WHILE() calls can occur. Currently > assert(!bs_->wakeup) will fail when this happens. > > This patch converts bs->wakeup from bool to a counter. > > Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate > the condition again after the inner caller completes (invoking the inner > caller counts as aio_poll() progress). > > Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> Doesn't this conflict with your own AIO_WAIT_WHILE() patch? Kevin
On Tue, Mar 06, 2018 at 12:08:44PM +0100, Kevin Wolf wrote: > Am 06.03.2018 um 11:53 hat Stefan Hajnoczi geschrieben: > > Nested BDRV_POLL_WHILE() calls can occur. Currently > > assert(!bs_->wakeup) will fail when this happens. > > > > This patch converts bs->wakeup from bool to a counter. > > > > Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate > > the condition again after the inner caller completes (invoking the inner > > caller counts as aio_poll() progress). > > > > Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > Doesn't this conflict with your own AIO_WAIT_WHILE() patch? Yes, I wanted this patch to be easy for Weiwei to test without dependencies. AIO_WAIT_WHILE() has just hit qemu.git/master, so I'll rebase and send a v2. Stefan
diff --git a/include/block/block.h b/include/block/block.h index fac401ba3e..990b97f0ad 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -385,9 +385,8 @@ void bdrv_drain_all(void); * other I/O threads' AioContexts (see for example \ * block_job_defer_to_main_loop for how to do it). \ */ \ - assert(!bs_->wakeup); \ - /* Set bs->wakeup before evaluating cond. */ \ - atomic_mb_set(&bs_->wakeup, true); \ + /* Increment bs->wakeup before evaluating cond. */ \ + atomic_inc(&bs_->wakeup); \ while (busy_) { \ if ((cond)) { \ waited_ = busy_ = true; \ @@ -399,7 +398,7 @@ void bdrv_drain_all(void); waited_ |= busy_; \ } \ } \ - atomic_set(&bs_->wakeup, false); \ + atomic_dec(&bs_->wakeup); \ } \ waited_; }) diff --git a/include/block/block_int.h b/include/block/block_int.h index 5ea63f8fa8..0f360c0ed5 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -712,7 +712,7 @@ struct BlockDriverState { /* Internal to BDRV_POLL_WHILE and bdrv_wakeup. Accessed with atomic * ops. */ - bool wakeup; + unsigned wakeup; /* counter for nested bdrv_io_plug. * Accessed with atomic ops.
Nested BDRV_POLL_WHILE() calls can occur. Currently assert(!bs_->wakeup) will fail when this happens. This patch converts bs->wakeup from bool to a counter. Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate the condition again after the inner caller completes (invoking the inner caller counts as aio_poll() progress). Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- Fu Weiwei: Please retest and let us know if this fixes the assertion failure you hit. Thanks! --- include/block/block.h | 7 +++---- include/block/block_int.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-)