[PATCH 0/2] virtio-blk and scsi: replace dataplane_{start/stopping/started}

[PATCH 0/2] virtio-blk and scsi: replace dataplane_{start/stopping/started}

[Emanuele Giuseppe Esposito]

The way the dataplane stages at startup and stop are monitored is unnecessary
complicated. In virtio-scsi we have dataplane_started, dataplane_starting and
dataplane_stopping in VirtIOSCSI.
In virtio-blk we have dataplene_started in VirtIOBlock, and starting and stopping
in VirtIOBlockDataPlane.

Just replace all these flags with an atomic enum.

Based-on: 20220803162824.948023-1-stefanha@redhat.com

Emanuele Giuseppe Esposito (2):
  virtio-scsi: replace VirtIOBlock dataplane_{start/starting/stopped}
    with enum
  virtio-blk: replace dataplane_start/stopping/started with enum

 hw/block/dataplane/virtio-blk.c | 24 +++++++++---------------
 hw/block/virtio-blk.c           |  9 +++++----
 hw/scsi/virtio-scsi-dataplane.c | 21 +++++++++------------
 hw/scsi/virtio-scsi.c           | 10 ++++++----
 include/hw/virtio/virtio-blk.h  |  2 +-
 include/hw/virtio/virtio-scsi.h |  5 ++---
 include/hw/virtio/virtio.h      |  7 +++++++
 7 files changed, 39 insertions(+), 39 deletions(-)

-- 
2.31.1

[PATCH 1/2] virtio-scsi: replace VirtIOBlock dataplane_{start/starting/stopped} with enum

[Emanuele Giuseppe Esposito]

Simplify the various dataplane stages in dataplane_start/stop by using
a single enum instead of having multiple flags.

Read/write the enum atomically, as it can be read also by iothread
callbacks.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 21 +++++++++------------
 hw/scsi/virtio-scsi.c           | 10 ++++++----
 include/hw/virtio/virtio-scsi.h |  5 ++---
 include/hw/virtio/virtio.h      |  7 +++++++
 4 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index a575c3f0cd..9ad73e3e19 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -106,13 +106,12 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
-    if (s->dataplane_started ||
-        s->dataplane_starting ||
+    if (qatomic_read(&s->dataplane_state) <= DATAPLANE_STARTED ||
         s->dataplane_fenced) {
         return 0;
     }
 
-    s->dataplane_starting = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STARTING);
 
     /* Set up guest notifier (irq) */
     rc = k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, true);
@@ -151,8 +150,7 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
-    s->dataplane_starting = false;
-    s->dataplane_started = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STARTED);
 
     /*
      * Attach notifiers from within the IOThread. It's possible to attach
@@ -183,8 +181,8 @@ fail_host_notifiers:
     k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
 fail_guest_notifiers:
     s->dataplane_fenced = true;
-    s->dataplane_starting = false;
-    s->dataplane_started = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STARTED);
+
     return -ENOSYS;
 }
 
@@ -197,17 +195,17 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     int i;
 
-    if (!s->dataplane_started || s->dataplane_stopping) {
+    if (qatomic_read(&s->dataplane_state) != DATAPLANE_STARTED) {
         return;
     }
 
     /* Better luck next time. */
     if (s->dataplane_fenced) {
         s->dataplane_fenced = false;
-        s->dataplane_started = false;
+        qatomic_set(&s->dataplane_state, DATAPLANE_STOPPED);
         return;
     }
-    s->dataplane_stopping = true;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STOPPING);
 
     aio_context_acquire(s->ctx);
     aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_stop_bh, s);
@@ -237,6 +235,5 @@ void virtio_scsi_dataplane_stop(VirtIODevice *vdev)
 
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
-    s->dataplane_stopping = false;
-    s->dataplane_started = false;
+    qatomic_set(&s->dataplane_state, DATAPLANE_STOPPED);
 }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 4141dddd51..e6ff667e86 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -110,7 +110,8 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
 
     qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
     virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
-    if (s->dataplane_started && !s->dataplane_fenced) {
+    if (qatomic_read(&s->dataplane_state) == DATAPLANE_STARTED &&
+        !s->dataplane_fenced) {
         virtio_notify_irqfd(vdev, vq);
     } else {
         virtio_notify(vdev, vq);
@@ -288,7 +289,8 @@ static void virtio_scsi_cancel_notify(Notifier *notifier, void *data)
 
 static inline void virtio_scsi_ctx_check(VirtIOSCSI *s, SCSIDevice *d)
 {
-    if (s->dataplane_started && d && blk_is_available(d->conf.blk)) {
+    if (qatomic_read(&s->dataplane_state) == DATAPLANE_STARTED && d &&
+        blk_is_available(d->conf.blk)) {
         assert(blk_get_aio_context(d->conf.blk) == s->ctx);
     }
 }
@@ -516,7 +518,7 @@ static void virtio_scsi_handle_ctrl_vq(VirtIOSCSI *s, VirtQueue *vq)
  */
 static bool virtio_scsi_defer_to_dataplane(VirtIOSCSI *s)
 {
-    if (!s->ctx || s->dataplane_started) {
+    if (!s->ctx || qatomic_read(&s->dataplane_state) <= DATAPLANE_STARTED) {
         return false;
     }
 
@@ -828,7 +830,7 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
 
-    assert(!s->dataplane_started);
+    assert(qatomic_read(&s->dataplane_state) != DATAPLANE_STARTED);
     s->resetting++;
     qbus_reset_all(BUS(&s->bus));
     s->resetting--;
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index a36aad9c86..e9e922dff4 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -85,9 +85,8 @@ struct VirtIOSCSI {
     /* Fields for dataplane below */
     AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
 
-    bool dataplane_started;
-    bool dataplane_starting;
-    bool dataplane_stopping;
+    enum VirtIODataplaneStates dataplane_state;
+
     bool dataplane_fenced;
     uint32_t host_features;
 };
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..25d1515570 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -71,6 +71,13 @@ typedef struct VirtQueueElement
 #define TYPE_VIRTIO_DEVICE "virtio-device"
 OBJECT_DECLARE_TYPE(VirtIODevice, VirtioDeviceClass, VIRTIO_DEVICE)
 
+enum VirtIODataplaneStates {
+    DATAPLANE_STARTING, /* dataplane is being initialized */
+    DATAPLANE_STARTED,  /* dataplane has been intialized */
+    DATAPLANE_STOPPING, /* dataplane is being stopped */
+    DATAPLANE_STOPPED,  /* dataplane is stopped */
+};
+
 enum virtio_device_endian {
     VIRTIO_DEVICE_ENDIAN_UNKNOWN,
     VIRTIO_DEVICE_ENDIAN_LITTLE,
-- 
2.31.1

Re: [PATCH 1/2] virtio-scsi: replace VirtIOBlock dataplane_{start/starting/stopped} with enum

[Stefan Hajnoczi]

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

On Mon, Aug 08, 2022 at 05:41:46AM -0400, Emanuele Giuseppe Esposito wrote:
> Simplify the various dataplane stages in dataplane_start/stop by using
> a single enum instead of having multiple flags.
> 
> Read/write the enum atomically, as it can be read also by iothread
> callbacks.

What guarantees that these relaxed loads/stores always produce
DATAPLANE_STARTING/STARTED in virtio_scsi_defer_to_dataplane() and not
an older value? Are there implicit memory barriers?

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  hw/scsi/virtio-scsi-dataplane.c | 21 +++++++++------------
>  hw/scsi/virtio-scsi.c           | 10 ++++++----
>  include/hw/virtio/virtio-scsi.h |  5 ++---
>  include/hw/virtio/virtio.h      |  7 +++++++
>  4 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index a575c3f0cd..9ad73e3e19 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -106,13 +106,12 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>      VirtIOSCSI *s = VIRTIO_SCSI(vdev);
>  
> -    if (s->dataplane_started ||
> -        s->dataplane_starting ||
> +    if (qatomic_read(&s->dataplane_state) <= DATAPLANE_STARTED ||

It's not obvious that the STOPPING and STOPPED constants have a value
greater than STARTING and STARTED. It could be other way around too. It
would be safer to write the code so there are no assumptions about the
constants:

  VirtIODataplaneStates state = qatomic_read(&s->dataplane_state);

  if (state == DATAPLANE_STARTING || state == DATAPLANE_STARTED || ...)

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

[PATCH 2/2] virtio-blk: replace dataplane_start/stopping/started with enum

[Emanuele Giuseppe Esposito]

Virtio-blk uses VirtIOBlockDataPlane and VirtIOBlock to keep track of
the dataplane flags. This is completely unnecessary, as both structures
are always accessed together and we can simplify the sages with an enum.

Read/write the enum atomically, as it can be read also by iothread
callbacks.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 24 +++++++++---------------
 hw/block/virtio-blk.c           |  9 +++++----
 include/hw/virtio/virtio-blk.h  |  2 +-
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 49276e46f2..427f946859 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -27,9 +27,6 @@
 #include "qom/object_interfaces.h"
 
 struct VirtIOBlockDataPlane {
-    bool starting;
-    bool stopping;
-
     VirtIOBlkConf *conf;
     VirtIODevice *vdev;
     QEMUBH *bh;                     /* bh for guest notification */
@@ -145,7 +142,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     }
 
     vblk = VIRTIO_BLK(s->vdev);
-    assert(!vblk->dataplane_started);
+    assert(qatomic_read(&vblk->dataplane_state) != DATAPLANE_STARTED);
     g_free(s->batch_notify_vqs);
     qemu_bh_delete(s->bh);
     if (s->iothread) {
@@ -167,11 +164,11 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
     Error *local_err = NULL;
     int r;
 
-    if (vblk->dataplane_started || s->starting) {
+    if (qatomic_read(&vblk->dataplane_state) <= DATAPLANE_STARTED) {
         return 0;
     }
 
-    s->starting = true;
+    qatomic_set(&vblk->dataplane_state, DATAPLANE_STARTING);
 
     if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         s->batch_notifications = true;
@@ -219,8 +216,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
 
     memory_region_transaction_commit();
 
-    s->starting = false;
-    vblk->dataplane_started = true;
+    qatomic_set(&vblk->dataplane_state, DATAPLANE_STARTED);
     trace_virtio_blk_data_plane_start(s);
 
     old_context = blk_get_aio_context(s->conf->conf.blk);
@@ -273,8 +269,7 @@ int virtio_blk_data_plane_start(VirtIODevice *vdev)
      */
     virtio_blk_process_queued_requests(vblk, false);
     vblk->dataplane_disabled = true;
-    s->starting = false;
-    vblk->dataplane_started = true;
+    qatomic_set(&vblk->dataplane_state, DATAPLANE_STARTED);
     return -ENOSYS;
 }
 
@@ -304,17 +299,17 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     unsigned i;
     unsigned nvqs = s->conf->num_queues;
 
-    if (!vblk->dataplane_started || s->stopping) {
+    if (qatomic_read(&vblk->dataplane_state) != DATAPLANE_STARTED) {
         return;
     }
 
     /* Better luck next time. */
     if (vblk->dataplane_disabled) {
         vblk->dataplane_disabled = false;
-        vblk->dataplane_started = false;
+        qatomic_set(&vblk->dataplane_state, DATAPLANE_STOPPED);
         return;
     }
-    s->stopping = true;
+    qatomic_set(&vblk->dataplane_state, DATAPLANE_STOPPING);
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
@@ -352,6 +347,5 @@ void virtio_blk_data_plane_stop(VirtIODevice *vdev)
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, nvqs, false);
 
-    vblk->dataplane_started = false;
-    s->stopping = false;
+    qatomic_set(&vblk->dataplane_state, DATAPLANE_STOPPED);
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e9ba752f6b..a53c4a1063 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -84,7 +84,8 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
     iov_discard_undo(&req->inhdr_undo);
     iov_discard_undo(&req->outhdr_undo);
     virtqueue_push(req->vq, &req->elem, req->in_len);
-    if (s->dataplane_started && !s->dataplane_disabled) {
+    if (qatomic_read(&s->dataplane_state) == DATAPLANE_STARTED &&
+        !s->dataplane_disabled) {
         virtio_blk_data_plane_notify(s->dataplane, req->vq);
     } else {
         virtio_notify(vdev, req->vq);
@@ -807,7 +808,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBlock *s = (VirtIOBlock *)vdev;
 
-    if (s->dataplane && !s->dataplane_started) {
+    if (s->dataplane && qatomic_read(&s->dataplane_state) > DATAPLANE_STARTED) {
         /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
          * dataplane here instead of waiting for .set_status().
          */
@@ -907,7 +908,7 @@ static void virtio_blk_reset(VirtIODevice *vdev)
 
     aio_context_release(ctx);
 
-    assert(!s->dataplane_started);
+    assert(qatomic_read(&s->dataplane_state) != DATAPLANE_STARTED);
     blk_set_enable_write_cache(s->blk, s->original_wce);
 }
 
@@ -1033,7 +1034,7 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
     if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
-        assert(!s->dataplane_started);
+        assert(qatomic_read(&s->dataplane_state) != DATAPLANE_STARTED);
     }
 
     if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d311c57cca..3ac66a1f77 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -60,7 +60,7 @@ struct VirtIOBlock {
     bool original_wce;
     VMChangeStateEntry *change;
     bool dataplane_disabled;
-    bool dataplane_started;
+    enum VirtIODataplaneStates dataplane_state;
     struct VirtIOBlockDataPlane *dataplane;
     uint64_t host_features;
     size_t config_size;
-- 
2.31.1

Re: [PATCH 2/2] virtio-blk: replace dataplane_start/stopping/started with enum

[Stefan Hajnoczi]

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

On Mon, Aug 08, 2022 at 05:41:47AM -0400, Emanuele Giuseppe Esposito wrote:
> Virtio-blk uses VirtIOBlockDataPlane and VirtIOBlock to keep track of
> the dataplane flags. This is completely unnecessary, as both structures
> are always accessed together and we can simplify the sages with an enum.

s/sages/stages/

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

Re: [PATCH 0/2] virtio-blk and scsi: replace dataplane_{start/stopping/started}

[Stefan Hajnoczi]

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

On Mon, Aug 08, 2022 at 05:41:45AM -0400, Emanuele Giuseppe Esposito wrote:
> The way the dataplane stages at startup and stop are monitored is unnecessary
> complicated. In virtio-scsi we have dataplane_started, dataplane_starting and
> dataplane_stopping in VirtIOSCSI.
> In virtio-blk we have dataplene_started in VirtIOBlock, and starting and stopping
> in VirtIOBlockDataPlane.
> 
> Just replace all these flags with an atomic enum.
> 
> Based-on: 20220803162824.948023-1-stefanha@redhat.com

As mentioned on IRC, I don't think it's useful to combine these fields
into a state machine because they serve different purposes
(starting/stopping prevents re-entrancy, started/stopped tracks whether
dataplane is enabled, and fenced tracks whether dataplane is broken).

Combining them all makes it harder to refactor those separate concerns.
In the future it would be nice to refactor away all this state and have
stateless IOThread support (I haven't figured out whether that's
possible yet).

I'm not against merging this, but I don't think using an enum is an
improvement - it's just different.

Regarding thread-safety, we need to guarantee that the state stored by a
QEMU thread is visible to the IOThread. This patch series doesn't really
do this: relaxed loads/stores aren't guaranteed to be visible to the
other thread. I comment on this in the patch: there needs to be at least
a comment or maybe something extra, like a memory barrier or stronger
atomic operation, to make the change to atomics worthwhile.

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

Re: [PATCH 0/2] virtio-blk and scsi: replace dataplane_{start/stopping/started}

[Stefan Hajnoczi]

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

On Mon, Aug 08, 2022 at 05:41:45AM -0400, Emanuele Giuseppe Esposito wrote:
> The way the dataplane stages at startup and stop are monitored is unnecessary
> complicated. In virtio-scsi we have dataplane_started, dataplane_starting and
> dataplane_stopping in VirtIOSCSI.
> In virtio-blk we have dataplene_started in VirtIOBlock, and starting and stopping
> in VirtIOBlockDataPlane.
> 
> Just replace all these flags with an atomic enum.
> 
> Based-on: 20220803162824.948023-1-stefanha@redhat.com

Hi Emanuele,
I posted a v2 of my virtio-scsi dataplane startup race condition fix
that's related to this patch. This time it's based on how virtio-blk
does it.

We can merge both our patch series or just one of them in order to fix
the race condition.

Stefan

> 
> Emanuele Giuseppe Esposito (2):
>   virtio-scsi: replace VirtIOBlock dataplane_{start/starting/stopped}
>     with enum
>   virtio-blk: replace dataplane_start/stopping/started with enum
> 
>  hw/block/dataplane/virtio-blk.c | 24 +++++++++---------------
>  hw/block/virtio-blk.c           |  9 +++++----
>  hw/scsi/virtio-scsi-dataplane.c | 21 +++++++++------------
>  hw/scsi/virtio-scsi.c           | 10 ++++++----
>  include/hw/virtio/virtio-blk.h  |  2 +-
>  include/hw/virtio/virtio-scsi.h |  5 ++---
>  include/hw/virtio/virtio.h      |  7 +++++++
>  7 files changed, 39 insertions(+), 39 deletions(-)
> 
> -- 
> 2.31.1
> 

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