| Message ID | 1433339175-12300-2-git-send-email-yarygin@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 03/06/2015 15:46, Alexander Yarygin wrote: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. > > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). > > 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> > --- > block/block-backend.c | 5 +++++ > hw/block/virtio-blk.c | 2 +- > include/sysemu/block-backend.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 93e46f3..aee8a12 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -700,6 +700,11 @@ int blk_flush_all(void) > return bdrv_flush_all(); > } > > +void blk_drain(BlockBackend *blk) > +{ > + bdrv_drain(blk->bs); > +} > + > void blk_drain_all(void) > { > bdrv_drain_all(); > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..abaca58 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > * This should cancel pending requests, but can't do nicely until there > * are per-device request lists. > */ > - blk_drain_all(); > + blk_drain(s->blk); > blk_set_enable_write_cache(s->blk, s->original_wce); > } > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index b4a4d5e..8fc960f 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors); > int blk_co_flush(BlockBackend *blk); > int blk_flush(BlockBackend *blk); > int blk_flush_all(void); > +void blk_drain(BlockBackend *blk); > void blk_drain_all(void); > BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read); > BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
Am 03.06.2015 um 15:46 schrieb Alexander Yarygin: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. > > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). > > 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> Seems to solve the main issue and causes no visible regression on a quick shot with my testcase. Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > block/block-backend.c | 5 +++++ > hw/block/virtio-blk.c | 2 +- > include/sysemu/block-backend.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 93e46f3..aee8a12 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -700,6 +700,11 @@ int blk_flush_all(void) > return bdrv_flush_all(); > } > > +void blk_drain(BlockBackend *blk) > +{ > + bdrv_drain(blk->bs); > +} > + > void blk_drain_all(void) > { > bdrv_drain_all(); > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..abaca58 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > * This should cancel pending requests, but can't do nicely until there > * are per-device request lists. > */ > - blk_drain_all(); > + blk_drain(s->blk); > blk_set_enable_write_cache(s->blk, s->original_wce); > } > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index b4a4d5e..8fc960f 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors); > int blk_co_flush(BlockBackend *blk); > int blk_flush(BlockBackend *blk); > int blk_flush_all(void); > +void blk_drain(BlockBackend *blk); > void blk_drain_all(void); > BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read); > BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, >
Am 03.06.2015 um 15:46 schrieb Alexander Yarygin: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. > > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). > > 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> Stefan/Kevin, any opinion on these 2 patches? I would like to have this issue fixed for s390 as having many disks is quite common on s390. > --- > block/block-backend.c | 5 +++++ > hw/block/virtio-blk.c | 2 +- > include/sysemu/block-backend.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 93e46f3..aee8a12 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -700,6 +700,11 @@ int blk_flush_all(void) > return bdrv_flush_all(); > } > > +void blk_drain(BlockBackend *blk) > +{ > + bdrv_drain(blk->bs); > +} > + > void blk_drain_all(void) > { > bdrv_drain_all(); > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..abaca58 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > * This should cancel pending requests, but can't do nicely until there > * are per-device request lists. > */ > - blk_drain_all(); > + blk_drain(s->blk); > blk_set_enable_write_cache(s->blk, s->original_wce); > } > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index b4a4d5e..8fc960f 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors); > int blk_co_flush(BlockBackend *blk); > int blk_flush(BlockBackend *blk); > int blk_flush_all(void); > +void blk_drain(BlockBackend *blk); > void blk_drain_all(void); > BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read); > BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, >
On Wed, Jun 03, 2015 at 04:46:14PM +0300, Alexander Yarygin wrote: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. > > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). > > 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> > --- > block/block-backend.c | 5 +++++ > hw/block/virtio-blk.c | 2 +- > include/sysemu/block-backend.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 93e46f3..aee8a12 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -700,6 +700,11 @@ int blk_flush_all(void) > return bdrv_flush_all(); > } > > +void blk_drain(BlockBackend *blk) > +{ > + bdrv_drain(blk->bs); > +} > + > void blk_drain_all(void) > { > bdrv_drain_all(); > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..abaca58 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > * This should cancel pending requests, but can't do nicely until there > * are per-device request lists. > */ > - blk_drain_all(); > + blk_drain(s->blk); > blk_set_enable_write_cache(s->blk, s->original_wce); > } I noticed a bug in the current code when reviewing this: If the 'backup' block job is active when the virtio-blk device is reset, bs is moved to the global AioContext while the target stays in the iothread AioContext. Accesses will be made to target without acquiring its AioContext. This is not your fault but calling blk_drain() increases the chance that this will deadlock. The bs request cannot make progress until target completes the I/O request. Since target doesn't pump events when we call aio_poll() on bs, draining bs will result in a hang if there are write requests pending on bs. I'll write a patch to address this case. Stefan
Am 03.06.2015 um 15:46 hat Alexander Yarygin geschrieben: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. > > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). > > 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> For the BlockBackend part: Acked-by: Kevin Wolf <kwolf@redhat.com> The only thing that I could suggest is that if you respin, it would be cleaner to split this into two parts (as small as they may be). That you have an "and" in your subject line is a strong hint for this. Kevin
On Wed, 06/03 16:46, Alexander Yarygin wrote: > Each call of the virtio_blk_reset() function calls blk_drain_all(), > which works for all existing BlockDriverStates, while only one > BlockDriverState needs to be drained. > > This patch introduces the blk_drain() function and replaces > blk_drain_all() on it in virtio_blk_reset(). > > 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> I'd like to ask for a split of this patch to two. One for introducing blk_drain and one for virtio-blk. For the latter please also move the blk_drain to above virtio_blk_data_plane_stop(), otherwise the restoring to vblk->complete_request will be wrong. Fam > --- > block/block-backend.c | 5 +++++ > hw/block/virtio-blk.c | 2 +- > include/sysemu/block-backend.h | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 93e46f3..aee8a12 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -700,6 +700,11 @@ int blk_flush_all(void) > return bdrv_flush_all(); > } > > +void blk_drain(BlockBackend *blk) > +{ > + bdrv_drain(blk->bs); > +} > + > void blk_drain_all(void) > { > bdrv_drain_all(); > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e6afe97..abaca58 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) > * This should cancel pending requests, but can't do nicely until there > * are per-device request lists. > */ > - blk_drain_all(); > + blk_drain(s->blk); > blk_set_enable_write_cache(s->blk, s->original_wce); > } > > diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h > index b4a4d5e..8fc960f 100644 > --- a/include/sysemu/block-backend.h > +++ b/include/sysemu/block-backend.h > @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors); > int blk_co_flush(BlockBackend *blk); > int blk_flush(BlockBackend *blk); > int blk_flush_all(void); > +void blk_drain(BlockBackend *blk); > void blk_drain_all(void); > BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read); > BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read, > -- > 1.9.1 > >
diff --git a/block/block-backend.c b/block/block-backend.c index 93e46f3..aee8a12 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -700,6 +700,11 @@ int blk_flush_all(void) return bdrv_flush_all(); } +void blk_drain(BlockBackend *blk) +{ + bdrv_drain(blk->bs); +} + void blk_drain_all(void) { bdrv_drain_all(); diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index e6afe97..abaca58 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -660,7 +660,7 @@ static void virtio_blk_reset(VirtIODevice *vdev) * This should cancel pending requests, but can't do nicely until there * are per-device request lists. */ - blk_drain_all(); + blk_drain(s->blk); blk_set_enable_write_cache(s->blk, s->original_wce); } diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index b4a4d5e..8fc960f 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -118,6 +118,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors); int blk_co_flush(BlockBackend *blk); int blk_flush(BlockBackend *blk); int blk_flush_all(void); +void blk_drain(BlockBackend *blk); void blk_drain_all(void); BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read); BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
Each call of the virtio_blk_reset() function calls blk_drain_all(), which works for all existing BlockDriverStates, while only one BlockDriverState needs to be drained. This patch introduces the blk_drain() function and replaces blk_drain_all() on it in virtio_blk_reset(). 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> --- block/block-backend.c | 5 +++++ hw/block/virtio-blk.c | 2 +- include/sysemu/block-backend.h | 1 + 3 files changed, 7 insertions(+), 1 deletion(-)