| Message ID | 20220808094147.612472-1-eesposit@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtio-blk and scsi: replace dataplane_{start/stopping/started} | expand |
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.
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 >
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(-)