[PULL 0/1] Block patches

[PULL 0/1] Block patches

[Stefan Hajnoczi]

The following changes since commit 63011373ad22c794a013da69663c03f1297a5c56:

  Merge tag 'pull-riscv-to-apply-20231012-1' of https://github.com/alistair23/qemu into staging (2023-10-12 10:24:44 -0400)

are available in the Git repository at:

  https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 071d6d107db2e26dde9bb15457c74956c88ec5b4:

  virtio-blk: don't start dataplane during the stop of dataplane (2023-10-16 15:39:13 -0400)

----------------------------------------------------------------
Pull request

Contains a virtio-blk IOThread fix.

----------------------------------------------------------------

hujian (1):
  virtio-blk: don't start dataplane during the stop of dataplane

 hw/block/virtio-blk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.41.0

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

[Stefan Hajnoczi]

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(-)

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().
          */
-- 
2.41.0

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

[Fiona Ebner]

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

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

[Kevin Wolf]

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

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]