Re: Question about scsi device hotplug (e.g scsi-hd)

[Stefan Hajnoczi]

On Thu, Apr 02, 2020 at 11:46:57AM +0300, Maxim Levitsky wrote:
> On Wed, 2020-04-01 at 18:36 +0200, Paolo Bonzini wrote:
> > On 01/04/20 17:09, Stefan Hajnoczi wrote:
> > > > What do you think about it?
> > > 
> > > Maybe aio_disable_external() is needed to postpone device emulation
> > > until after realize has finished?
> 
> Isn't that virtio specific? In theory this issue can touch any driver that
> behaves similar to scsi.

No, aio_disable_external() is a general solution for suspending file
descriptor handlers.  In practice virtio is the main user of external
fds (ioeventfd).  Devices that do not use ioeventfd will not perform
device emulation from the AioContext event loop and are therefore safe.

> Also due to racing, the request might already be in virtqueue and the 
> virtio-scsi iothread might have already
> fetched it when we place the device on the bus.
> 
> In fact I don't see any locking in bus_add_child / scsi_device_find so in 
> fact if the timing is right
> and iothread calls the scsi_device_find while main thread adds the device on 
> the bus that will cause
> the whole thing go south.

Thread-safety with IOThreads is a separate issue and also worth fixing.
It requires a different and complementary solution from the
single-threaded ioeventfd bug we've been discussing so far.

It looks like virtio_scsi_acquire/release() were intended to provide
thread-safety for virtio-scsi:

  static bool virtio_scsi_data_plane_handle_cmd(VirtIODevice *vdev,
                                                VirtQueue *vq)
  {
      bool progress;
      VirtIOSCSI *s = VIRTIO_SCSI(vdev);

      virtio_scsi_acquire(s);
      assert(s->ctx && s->dataplane_started);
      progress = virtio_scsi_handle_cmd_vq(s, vq);
      virtio_scsi_release(s);
      return progress;
  }

If the monitor code calls virtio_scsi_acquire() then further request
processing will have to wait until the monitor code is finished.

> IMHO the right solution for this is first to realize the device and then 
> place it
> on the bus, and even that I am not sure that will fix the race completely.
>
> IMHO the correct solution here is to stop querying the bus children on the 
> guest controlled IO path 
> (scsi_device_find) but rather use the hotplug handlers to
> store the active luns of the scsi device in parallel (and with proper 
> locking).

It would be nice to have a general solution in qdev core that makes
thread-safe hotplug easy for all devices.

Stefan

signature.asc
Description: PGP signature