Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Mon, Dec 04, 2023 at 01:46:13PM +0100, Kevin Wolf wrote: > Am 29.11.2023 um 20:55 hat Stefan Hajnoczi geschrieben: > > Protect the Task Management Function BH state with a lock. The TMF BH > > runs in the main loop thread. An IOThread might process a TMF at the > > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > > must be protected by a lock. > > > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > > This avoids more locking to protect the virtqueue and SCSI layer state. > > > > Signed-off-by: Stefan Hajnoczi > > The second part reminds me that the implicit protection of the virtqueue > and SCSI data structures by having all accesses in a single thread is > hard to review and I think we wanted to put some assertions there to > check that we're really running in the right thread. I don't think we > have done that so far, so I suppose after this patch would be the place > in the series to add them, before we remove the protection by the > AioContext lock? Thanks for reminding me. I will add assertions in the next revision of this series. Stefan signature.asc Description: PGP signature
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Thu, Nov 30, 2023 at 09:25:52AM -0600, Eric Blake wrote: > On Wed, Nov 29, 2023 at 02:55:42PM -0500, Stefan Hajnoczi wrote: > > Protect the Task Management Function BH state with a lock. The TMF BH > > runs in the main loop thread. An IOThread might process a TMF at the > > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > > must be protected by a lock. > > > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > > This avoids more locking to protect the virtqueue and SCSI layer state. > > Are we trying to get this into 8.2? No. 8.2 still has the AioContext lock is therefore safe without this patch. Stefan > > > > > Signed-off-by: Stefan Hajnoczi > > --- > > include/hw/virtio/virtio-scsi.h | 3 +- > > hw/scsi/virtio-scsi.c | 62 ++--- > > 2 files changed, 43 insertions(+), 22 deletions(-) > > > > Reviewed-by: Eric Blake > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. > Virtualization: qemu.org | libguestfs.org > signature.asc Description: PGP signature
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
Am 29.11.2023 um 20:55 hat Stefan Hajnoczi geschrieben: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. > > Signed-off-by: Stefan Hajnoczi The second part reminds me that the implicit protection of the virtqueue and SCSI data structures by having all accesses in a single thread is hard to review and I think we wanted to put some assertions there to check that we're really running in the right thread. I don't think we have done that so far, so I suppose after this patch would be the place in the series to add them, before we remove the protection by the AioContext lock? Kevin
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
Am 29.11.2023 um 20:55 hat Stefan Hajnoczi geschrieben: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. > > Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf
Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock
On Wed, Nov 29, 2023 at 02:55:42PM -0500, Stefan Hajnoczi wrote: > Protect the Task Management Function BH state with a lock. The TMF BH > runs in the main loop thread. An IOThread might process a TMF at the > same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh > must be protected by a lock. > > Run TMF request completion in the IOThread using aio_wait_bh_oneshot(). > This avoids more locking to protect the virtqueue and SCSI layer state. Are we trying to get this into 8.2? > > Signed-off-by: Stefan Hajnoczi > --- > include/hw/virtio/virtio-scsi.h | 3 +- > hw/scsi/virtio-scsi.c | 62 ++--- > 2 files changed, 43 insertions(+), 22 deletions(-) > Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
