diff mbox series

[PULL,1/1] virtio-blk: don't start dataplane during the stop of dataplane

Message ID 20231016194028.163610-2-stefanha@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,1/1] virtio-blk: don't start dataplane during the stop of dataplane | expand

Commit Message

Stefan Hajnoczi Oct. 16, 2023, 7:40 p.m. UTC 
From: hujian <hu.jian@zte.com.cn>

During the stop of dataplane for virtio-blk, virtio_bus_cleanup_host_notifier() is be
called to clean up notifier at the end, if polled ioeventfd, virtio_blk_handle_output()
is used to handle io request. But due to s->dataplane_disabled is false, it will be
returned directly, which drops io request.
Backtrace:
->virtio_blk_data_plane_stop
  ->virtio_bus_cleanup_host_notifier
    ->virtio_queue_host_notifier_read
      ->virtio_queue_notify_vq
        ->vq->handle_output
          ->virtio_blk_handle_output
            ->if (s->dataplane  && !s->dataplane_stoped)
              ->if (!s->dataplane_disabled)
                ->return *
            ->virtio_blk_handle_output_do
The above problem can occur when using "virsh reset" cmdline to reset guest, while
guest does io.
To fix this problem, don't try to start dataplane if s->stopping is true, and io would
be handled by virtio_blk_handle_vq().

Signed-off-by: hujian <hu.jian@zte.com.cn>
Message-id: 202310111414266586398@zte.com.cn
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Fiona Ebner Oct. 17, 2023, 9:01 a.m. UTC | #1 
Am 16.10.23 um 21:40 schrieb Stefan Hajnoczi:
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 39e7f23fab..c2d59389cb 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1166,7 +1166,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      VirtIOBlock *s = (VirtIOBlock *)vdev;
>  
> -    if (s->dataplane && !s->dataplane_started) {
> +    if (s->dataplane && !s->dataplane_started && !s->stopping) {

Isn't 'stopping' a property of VirtIOBlockDataPlane? Here, s is VirtIOBlock.

Best Regards,
Fiona
Kevin Wolf Oct. 17, 2023, 10:02 a.m. UTC | #2 
Am 17.10.2023 um 11:01 hat Fiona Ebner geschrieben:
> Am 16.10.23 um 21:40 schrieb Stefan Hajnoczi:
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 39e7f23fab..c2d59389cb 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1166,7 +1166,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      VirtIOBlock *s = (VirtIOBlock *)vdev;
> >  
> > -    if (s->dataplane && !s->dataplane_started) {
> > +    if (s->dataplane && !s->dataplane_started && !s->stopping) {
> 
> Isn't 'stopping' a property of VirtIOBlockDataPlane? Here, s is VirtIOBlock.

Indeed, this patch doesn't even build for me.

However, even if we wrote !s->dataplane->stopping, would it really be
right to be handling I/O in the main thread while the dataplane hasn't
stopped yet? At least without all the multiqueue changes, it's not
obvious to me that it can't cause problems. Unfortunately, the commit
message doesn't say anything about why it's safe.

Kevin
Stefan Hajnoczi Oct. 17, 2023, 1:46 p.m. UTC | #3 
On Tue, Oct 17, 2023 at 12:02:26PM +0200, Kevin Wolf wrote:
> Am 17.10.2023 um 11:01 hat Fiona Ebner geschrieben:
> > Am 16.10.23 um 21:40 schrieb Stefan Hajnoczi:
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 39e7f23fab..c2d59389cb 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -1166,7 +1166,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > >  {
> > >      VirtIOBlock *s = (VirtIOBlock *)vdev;
> > >  
> > > -    if (s->dataplane && !s->dataplane_started) {
> > > +    if (s->dataplane && !s->dataplane_started && !s->stopping) {
> > 
> > Isn't 'stopping' a property of VirtIOBlockDataPlane? Here, s is VirtIOBlock.
> 
> Indeed, this patch doesn't even build for me.
> 
> However, even if we wrote !s->dataplane->stopping, would it really be
> right to be handling I/O in the main thread while the dataplane hasn't
> stopped yet? At least without all the multiqueue changes, it's not
> obvious to me that it can't cause problems. Unfortunately, the commit
> message doesn't say anything about why it's safe.

Thanks for pointing these things out, Fiona and Kevin. I've dropped the
patch for now.

Stefan
diff mbox series

Patch

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 39e7f23fab..c2d59389cb 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1166,7 +1166,7 @@  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBlock *s = (VirtIOBlock *)vdev;
 
-    if (s->dataplane && !s->dataplane_started) {
+    if (s->dataplane && !s->dataplane_started && !s->stopping) {
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
          * dataplane here instead of waiting for .set_status().
          */