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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]