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

[Hanna Czenczek]

On 21.12.23 22:23, Kevin Wolf wrote:

> From: Stefan Hajnoczi <stefanha@redhat.com>
>
> Stop depending on the AioContext lock and instead access
> SCSIDevice->requests from only one thread at a time:
> - When the VM is running only the BlockBackend's AioContext may access
>   the requests list.
> - When the VM is stopped only the main loop may access the requests
>   list.
>
> These constraints protect the requests list without the need for locking
> in the I/O code path.
>
> Note that multiple IOThreads are not supported yet because the code
> assumes all SCSIRequests are executed from a single AioContext. Leave
> that as future work.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-ID: <20231204164259.1515217-2-stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/hw/scsi/scsi.h |   7 +-
>  hw/scsi/scsi-bus.c     | 181 ++++++++++++++++++++++++++++-------------
>  2 files changed, 131 insertions(+), 57 deletions(-)
>     
    
    My reproducer for https://issues.redhat.com/browse/RHEL-3934 now
    breaks more often because of this commit than because of the
    original bug, i.e. when repeatedly hot-plugging and unplugging a
    virtio-scsi and a scsi-hd device, this tends to happen when
    unplugging the scsi-hd:
    
    {"execute":"device_del","arguments":{"id":"stg0"}}
    {"return": {}}
    qemu-system-x86_64: ../block/block-backend.c:2429:
    blk_get_aio_context: Assertion `ctx == blk->ctx' failed.
    
    (gdb) bt
    #0  0x00007f32a668d83c in  () at /usr/lib/libc.so.6
    #1  0x00007f32a663d668 in raise () at /usr/lib/libc.so.6
    #2  0x00007f32a66254b8 in abort () at /usr/lib/libc.so.6
    #3  0x00007f32a66253dc in  () at /usr/lib/libc.so.6
    #4  0x00007f32a6635d26 in  () at /usr/lib/libc.so.6
    #5  0x0000556e6b4880a4 in blk_get_aio_context (blk=0x556e6e89ccf0)
    at ../block/block-backend.c:2429
    #6  blk_get_aio_context (blk=0x556e6e89ccf0) at
    ../block/block-backend.c:2417
    #7  0x0000556e6b112d87 in scsi_device_for_each_req_async_bh
    (opaque=0x556e6e2c6d10) at ../hw/scsi/scsi-bus.c:128
    #8  0x0000556e6b5d1966 in aio_bh_poll (ctx=ctx@entry=0x556e6d8aa290)
    at ../util/async.c:218
    #9  0x0000556e6b5bb16a in aio_poll (ctx=0x556e6d8aa290,
    blocking=blocking@entry=true) at ../util/aio-posix.c:722
    #10 0x0000556e6b4564b6 in iothread_run
    (opaque=opaque@entry=0x556e6d89d920) at ../iothread.c:63
    #11 0x0000556e6b5bde58 in qemu_thread_start (args=0x556e6d8aa9b0) at
    ../util/qemu-thread-posix.c:541
    #12 0x00007f32a668b9eb in  () at /usr/lib/libc.so.6
    #13 0x00007f32a670f7cc in  () at /usr/lib/libc.so.6
    
    I don’t know anything about the problem yet, but as usual, I like
    speculation and discovering how wrong I was later on, so one thing I
    came across that’s funny about virtio-scsi is that requests can
    happen even while a disk is being attached or detached.  That is,
    Linux seems to probe all LUNs when a new virtio-scsi device is being
    attached, and it won’t stop just because a disk is being attached or
    removed.  So maybe that’s part of the problem, that we get a request
    while the BB is being detached, and temporarily in an inconsistent
    state (BDS context differs from BB context).
    
    I’ll look more into it.
    
    Hanna