| Message ID | 1434537440-28236-3-git-send-email-yarygin@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Just spotted this in my git-pull... Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while draining only > one is needed. > > This patch replaces blk_drain_all() by blk_drain() in > virtio_blk_reset(). virtio_blk_data_plane_stop() should be called > after draining because it restores vblk->complete_request. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com> > --- > hw/block/virtio-blk.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..d8a906f 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -651,16 +651,21 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running, > static void virtio_blk_reset(VirtIODevice *vdev) > { > VirtIOBlock *s = VIRTIO_BLK(vdev); > - > - if (s->dataplane) { > - virtio_blk_data_plane_stop(s->dataplane); > - } > + AioContext *ctx; > > /* > * This should cancel pending requests, but can't do nicely until there > * are per-device request lists. > */ > - blk_drain_all(); > + ctx = blk_get_aio_context(s->blk); > + aio_context_acquire(ctx); > + blk_drain(s->blk); > + > + if (s->dataplane) { > + virtio_blk_data_plane_stop(s->dataplane); > + } > + aio_context_release(ctx); > + > blk_set_enable_write_cache(s->blk, s->original_wce); > } From bdrv_drain_all()'s comment: * Note that completion of an asynchronous I/O operation can trigger any * number of other I/O operations on other devices---for example a coroutine * can be arbitrarily complex and a constant flow of I/O can come until the * coroutine is complete. Because of this, it is not possible to have a * function to drain a single device's I/O queue. From bdrv_drain()'s comment: * See the warning in bdrv_drain_all(). This function can only be called if * you are sure nothing can generate I/O because you have op blockers * installed. blk_drain() and blk_drain_all() are trivial wrappers. Ignorant questions: * Why does blk_drain() suffice here? * Is blk_drain() (created in PATCH 1) even a safe interface?
Markus Armbruster <armbru@redhat.com> writes: > Just spotted this in my git-pull... > > Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: > >> Each call of the virtio_blk_reset() function calls blk_drain_all(), >> which works for all existing BlockDriverStates, while draining only >> one is needed. >> >> This patch replaces blk_drain_all() by blk_drain() in >> virtio_blk_reset(). virtio_blk_data_plane_stop() should be called >> after draining because it restores vblk->complete_request. >> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Christian Borntraeger <borntraeger@de.ibm.com> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >> Cc: Kevin Wolf <kwolf@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com> >> --- >> hw/block/virtio-blk.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index e6afe97..d8a906f 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -651,16 +651,21 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running, >> static void virtio_blk_reset(VirtIODevice *vdev) >> { >> VirtIOBlock *s = VIRTIO_BLK(vdev); >> - >> - if (s->dataplane) { >> - virtio_blk_data_plane_stop(s->dataplane); >> - } >> + AioContext *ctx; >> >> /* >> * This should cancel pending requests, but can't do nicely until there >> * are per-device request lists. >> */ >> - blk_drain_all(); >> + ctx = blk_get_aio_context(s->blk); >> + aio_context_acquire(ctx); >> + blk_drain(s->blk); >> + >> + if (s->dataplane) { >> + virtio_blk_data_plane_stop(s->dataplane); >> + } >> + aio_context_release(ctx); >> + >> blk_set_enable_write_cache(s->blk, s->original_wce); >> } > > From bdrv_drain_all()'s comment: > > * Note that completion of an asynchronous I/O operation can trigger any > * number of other I/O operations on other devices---for example a coroutine > * can be arbitrarily complex and a constant flow of I/O can come until the > * coroutine is complete. Because of this, it is not possible to have a > * function to drain a single device's I/O queue. > > From bdrv_drain()'s comment: > > * See the warning in bdrv_drain_all(). This function can only be called if > * you are sure nothing can generate I/O because you have op blockers > * installed. > > blk_drain() and blk_drain_all() are trivial wrappers. > > Ignorant questions: > > * Why does blk_drain() suffice here? > > * Is blk_drain() (created in PATCH 1) even a safe interface? * We want to drain requests from only one bdrv and blk_drain() can do that. * Ignorant answer: I was told that the bdrv_drain_all()'s comment is obsolete and we can use bdrv_drain(). Here is a link to the old thread: http://marc.info/?l=qemu-devel&m=143154211017926&w=2. Since I don't see the full picture of this area yet, I'm just relying on other people's opinion.
Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Just spotted this in my git-pull... >> >> Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: >> >>> Each call of the virtio_blk_reset() function calls blk_drain_all(), >>> which works for all existing BlockDriverStates, while draining only >>> one is needed. >>> >>> This patch replaces blk_drain_all() by blk_drain() in >>> virtio_blk_reset(). virtio_blk_data_plane_stop() should be called >>> after draining because it restores vblk->complete_request. >>> >>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>> Cc: Christian Borntraeger <borntraeger@de.ibm.com> >>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> >>> Cc: Kevin Wolf <kwolf@redhat.com> >>> Cc: Paolo Bonzini <pbonzini@redhat.com> >>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com> >>> --- >>> hw/block/virtio-blk.c | 15 ++++++++++----- >>> 1 file changed, 10 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >>> index e6afe97..d8a906f 100644 >>> --- a/hw/block/virtio-blk.c >>> +++ b/hw/block/virtio-blk.c >>> @@ -651,16 +651,21 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running, >>> static void virtio_blk_reset(VirtIODevice *vdev) >>> { >>> VirtIOBlock *s = VIRTIO_BLK(vdev); >>> - >>> - if (s->dataplane) { >>> - virtio_blk_data_plane_stop(s->dataplane); >>> - } >>> + AioContext *ctx; >>> >>> /* >>> * This should cancel pending requests, but can't do nicely until there >>> * are per-device request lists. >>> */ >>> - blk_drain_all(); >>> + ctx = blk_get_aio_context(s->blk); >>> + aio_context_acquire(ctx); >>> + blk_drain(s->blk); >>> + >>> + if (s->dataplane) { >>> + virtio_blk_data_plane_stop(s->dataplane); >>> + } >>> + aio_context_release(ctx); >>> + >>> blk_set_enable_write_cache(s->blk, s->original_wce); >>> } >> >> From bdrv_drain_all()'s comment: >> >> * Note that completion of an asynchronous I/O operation can trigger any >> * number of other I/O operations on other devices---for example a coroutine >> * can be arbitrarily complex and a constant flow of I/O can come until the >> * coroutine is complete. Because of this, it is not possible to have a >> * function to drain a single device's I/O queue. >> >> From bdrv_drain()'s comment: >> >> * See the warning in bdrv_drain_all(). This function can only be called if >> * you are sure nothing can generate I/O because you have op blockers >> * installed. >> >> blk_drain() and blk_drain_all() are trivial wrappers. >> >> Ignorant questions: >> >> * Why does blk_drain() suffice here? >> >> * Is blk_drain() (created in PATCH 1) even a safe interface? > > * We want to drain requests from only one bdrv and blk_drain() can do > that. It's never been a question of not wanting to drain just one device, it's been a problem of it not working. But point taken. > * Ignorant answer: I was told that the bdrv_drain_all()'s comment is > obsolete and we can use bdrv_drain(). Here is a link to the old > thread: http://marc.info/?l=qemu-devel&m=143154211017926&w=2. Kevin, Stefan, if the comment has become wrong, it needs to be redone. Who's going to take care of it? > Since I > don't see the full picture of this area yet, I'm just relying on other > people's opinion. That's fair, we all do :)
On Mon, Jun 29, 2015 at 08:10:20AM +0200, Markus Armbruster wrote: > Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: > > Markus Armbruster <armbru@redhat.com> writes: > > * Ignorant answer: I was told that the bdrv_drain_all()'s comment is > > obsolete and we can use bdrv_drain(). Here is a link to the old > > thread: http://marc.info/?l=qemu-devel&m=143154211017926&w=2. > > Kevin, Stefan, if the comment has become wrong, it needs to be redone. > Who's going to take care of it? I couldn't think of a scenario where this patch is unsafe. The danger is that the I/O code path depends on something outside the AioContext. In that case you block in aio_poll(aio_context, true) forever without making progress. The thing the I/O request depends on will never finish. Code that accesses multiple BDSes puts them into the same AioContext, so this should not be a problem in practice. Stefan
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Mon, Jun 29, 2015 at 08:10:20AM +0200, Markus Armbruster wrote: >> Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: >> > Markus Armbruster <armbru@redhat.com> writes: >> > * Ignorant answer: I was told that the bdrv_drain_all()'s comment is >> > obsolete and we can use bdrv_drain(). Here is a link to the old >> > thread: http://marc.info/?l=qemu-devel&m=143154211017926&w=2. >> >> Kevin, Stefan, if the comment has become wrong, it needs to be redone. >> Who's going to take care of it? > > I couldn't think of a scenario where this patch is unsafe. > > The danger is that the I/O code path depends on something outside the > AioContext. In that case you block in aio_poll(aio_context, true) > forever without making progress. The thing the I/O request depends on > will never finish. > > Code that accesses multiple BDSes puts them into the same AioContext, so > this should not be a problem in practice. Stefan, could you rework the bdrv_drain()'s to spell out how it can be used safely?
On Wed, Jul 1, 2015 at 4:52 PM, Markus Armbruster <armbru@redhat.com> wrote: > Stefan Hajnoczi <stefanha@redhat.com> writes: > >> On Mon, Jun 29, 2015 at 08:10:20AM +0200, Markus Armbruster wrote: >>> Alexander Yarygin <yarygin@linux.vnet.ibm.com> writes: >>> > Markus Armbruster <armbru@redhat.com> writes: >>> > * Ignorant answer: I was told that the bdrv_drain_all()'s comment is >>> > obsolete and we can use bdrv_drain(). Here is a link to the old >>> > thread: http://marc.info/?l=qemu-devel&m=143154211017926&w=2. >>> >>> Kevin, Stefan, if the comment has become wrong, it needs to be redone. >>> Who's going to take care of it? >> >> I couldn't think of a scenario where this patch is unsafe. >> >> The danger is that the I/O code path depends on something outside the >> AioContext. In that case you block in aio_poll(aio_context, true) >> forever without making progress. The thing the I/O request depends on >> will never finish. >> >> Code that accesses multiple BDSes puts them into the same AioContext, so >> this should not be a problem in practice. > > Stefan, could you rework the bdrv_drain()'s to spell out how it can be > used safely? Good idea. I will send a patch.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index e6afe97..d8a906f 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -651,16 +651,21 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running, static void virtio_blk_reset(VirtIODevice *vdev) { VirtIOBlock *s = VIRTIO_BLK(vdev); - - if (s->dataplane) { - virtio_blk_data_plane_stop(s->dataplane); - } + AioContext *ctx; /* * This should cancel pending requests, but can't do nicely until there * are per-device request lists. */ - blk_drain_all(); + ctx = blk_get_aio_context(s->blk); + aio_context_acquire(ctx); + blk_drain(s->blk); + + if (s->dataplane) { + virtio_blk_data_plane_stop(s->dataplane); + } + aio_context_release(ctx); + blk_set_enable_write_cache(s->blk, s->original_wce); }
Each call of the virtio_blk_reset() function calls blk_drain_all(), which works for all existing BlockDriverStates, while draining only one is needed. This patch replaces blk_drain_all() by blk_drain() in virtio_blk_reset(). virtio_blk_data_plane_stop() should be called after draining because it restores vblk->complete_request. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: Cornelia Huck <cornelia.huck@de.ibm.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Alexander Yarygin <yarygin@linux.vnet.ibm.com> --- hw/block/virtio-blk.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)