Re: [PULL 11/33] scsi: only access SCSIDevice->requests from one thread

[Stefan Hajnoczi]

On Fri, Feb 02, 2024 at 01:32:39PM +0100, Hanna Czenczek wrote:
> On 01.02.24 16:25, Hanna Czenczek wrote:
> > On 01.02.24 15:28, Stefan Hajnoczi wrote:
> 
> [...]
> 
> > > Did you find a scenario where the virtio-scsi AioContext is different
> > > from the scsi-hd BB's Aiocontext?
> > 
> > Technically, that’s the reason for this thread, specifically that
> > virtio_scsi_hotunplug() switches the BB back to the main context while
> > scsi_device_for_each_req_async_bh() is running.  Yes, we can fix that
> > specific case via the in-flight counter, but I’m wondering whether
> > there’s really any merit in requiring the BB to always be in
> > virtio-scsi’s context, or whether it would make more sense to schedule
> > everything in virtio-scsi’s context.  Now that BBs/BDSs can receive
> > requests from any context, that is.
> 
> Now that I know that wouldn’t be easy, let me turn this around: As far as I
> understand, scsi_device_for_each_req_async_bh() should still run in
> virtio-scsi’s context, but that’s hard, so we take the BB’s context, which
> we therefore require to be the same one. Further, (again AFAIU,)
> virtio-scsi’s context cannot change (only set in
> virtio_scsi_dataplane_setup(), which is run in
> virtio_scsi_device_realize()).  Therefore, why does the
> scsi_device_for_each_req_async() code accommodate for BB context changes?

1. scsi_disk_reset() -> scsi_device_purge_requests() is called without
   in-flight requests.
2. The BH is scheduled by scsi_device_purge_requests() ->
   scsi_device_for_each_req_async().
3. blk_drain() is a nop when there no in-flight requests and does not
   flush BHs.
3. The AioContext changes when the virtio-scsi device resets.
4. The BH executes.

Kevin and I touched on the idea of flushing BHs in bdrv_drain() even
when there are no requests in flight. This hasn't been implemented as of
today, but would also reduce the chance of scenarios like the one I
mentioned.

I think it's safer to handle the case where the BH runs after an
AioContext change until either everything is thread-safe or the
AioContext never changes.

Stefan

signature.asc
Description: PGP signature