Re: [PATCH v2 3/3] virtio-scsi: reset SCSI devices from main loop thread

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Tue, Feb 21, 2023 at 11:40:26AM -0500, Stefan Hajnoczi wrote:
> On Fri, Feb 17, 2023 at 11:22:02AM +0100, Kevin Wolf wrote:
> > Am 10.02.2023 um 15:32 hat Stefan Hajnoczi geschrieben:
> > > When an IOThread is configured, the ctrl virtqueue is processed in the
> > > IOThread. TMFs that reset SCSI devices are currently called directly
> > > from the IOThread and trigger an assertion failure in blk_drain():
> > > 
> > >   ../block/block-backend.c:1780: void blk_drain(BlockBackend *): Assertion `qemu_in_main_thread()' failed.
> > > 
> > > The blk_drain() function is not designed to be called from an IOThread
> > > because it needs the Big QEMU Lock (BQL).
> > > 
> > > This patch defers TMFs that reset SCSI devices to a Bottom Half (BH)
> > > that runs in the main loop thread under the BQL. This way it's safe to
> > > call blk_drain() and the assertion failure is avoided.
> > 
> > It's not entirely obvious what is the call path that leads to
> > blk_drain(). Do we somehow call into virtio_scsi_dataplane_stop()?
> 
> I'll make it clearer that resetting the SCSIDevice calls blk_drain():
> 
>   void scsi_device_purge_requests(SCSIDevice *sdev, SCSISense sense)
>   {
>       SCSIRequest *req;
> 
>       aio_context_acquire(blk_get_aio_context(sdev->conf.blk));
>       while (!QTAILQ_EMPTY(&sdev->requests)) {
>           req = QTAILQ_FIRST(&sdev->requests);
>           scsi_req_cancel_async(req, NULL);
>       }
>       blk_drain(sdev->conf.blk);
>       ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> The IOThread code path is virtio_scsi_handle_ctrl_req ->
> virtio_scsi_do_tmf -> device_code_reset -> scsi_disk_reset ->
> scsi_device_purge_requests.
> 
> > > Introduce s->tmf_bh_list for tracking TMF requests that have been
> > > deferred to the BH. When the BH runs it will grab the entire list and
> > > process all requests. Care must be taken to clear the list when the
> > > virtio-scsi device is reset or unrealized. Otherwise deferred TMF
> > > requests could execute later and lead to use-after-free or other
> > > undefined behavior.
> > 
> > Why don't we already need the same for other asynchronously processed
> > requests? Certainly having a read request write to guest memory after
> > the device has been reset or unplugged isn't what we want either.
> 
> I/O is already taken care of because bdrv_drain_all() is called in
> do_vm_stop(). TMFs are not purely a device emulation concept and not a
> block layer concept covered by bdrv_drain_all(), so they need to be
> handled explicitly.
> 
> > 
> > I see that we assert(!s->dataplane_started) in virtio_scsi_reset(),
> > which may be part of the reason. If we're not processing any requests,
> 
> Yes, dataplane is stopped when the virtio-scsi device is reset.
> 
> > then we're safe. virtio_scsi_dataplane_stop() calls blk_drain_all()
> > (which is really a too big hammer) in order to make sure that in-flight
> > requests are completed before dataplane_started becomes false.
> > 
> > I was wondering if we couldn't just blk_inc_in_flight() while a TMF
> > request is in flight and then use the same draining logic to be covered.
> > You could use oneshot BHs then and do away with the list because you
> > don't need to cancel anything any more, you just wait until the BHs have
> > completed.
> > 
> > The practical problem may be that we don't have a blk here (which is
> > probably also why blk_drain_all() is used). We could have our own
> > AIO_WAIT_WHILE() instead. I feel waiting instead of cancelling BHs would
> > simplify the code.
> > 
> > In fact, I think technically, you may not need any of that because
> > blk_drain_all() also executes all BHs in the main loop before it
> > returns, but that might be a bit subtle...
> 
> There is a bit of a layering violation if we use the fake block layer
> requests to track virtio-scsi HBA TMF emulation.
> 
> The starting point for doing this would probably be a
> SCSIDeviceClass->schedule_reset() function that allows scsi-disk.c to
> implement the one-shot BH + blk_inc_in_flight() approach. scsi-disk.c
> has the BlockBackend.
> 
> The fact that we're having to think hard about how to make this work
> makes me wonder whether it's a good idea?
> 
> > > The s->resetting counter that's used by TMFs that reset SCSI devices is
> > > accessed from multiple threads. This patch makes that explicit by using
> > > atomic accessor functions. With this patch applied the counter is only
> > > modified by the main loop thread under the BQL but can be read by any
> > > thread.
> > > 
> > > Reported-by: Qing Wang <qinwang@redhat.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Kevin
> > 



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