[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext
From: |
Stefan Hajnoczi |
Subject: |
Re: [PATCH 1/8] virtio_queue_aio_attach_host_notifier: remove AioContext lock |
Date: |
Tue, 12 Jul 2022 13:47:07 +0100 |
On Fri, Jul 08, 2022 at 11:01:37AM +0200, Emanuele Giuseppe Esposito wrote:
>
>
> Am 05/07/2022 um 16:11 schrieb Stefan Hajnoczi:
> > On Thu, Jun 09, 2022 at 10:37:20AM -0400, Emanuele Giuseppe Esposito wrote:
> >> @@ -146,7 +147,6 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev)
> >>
> >> s->dataplane_starting = false;
> >> s->dataplane_started = true;
> >> - aio_context_release(s->ctx);
> >> return 0;
> >
> > This looks risky because s->dataplane_started is accessed by IO code and
> > there is a race condition here. Maybe you can refactor the code along
> > the lines of virtio-blk to avoid the race.
> >
>
> Uhmm could you explain why is virtio-blk also safe here?
> And what is currently protecting dataplane_started (in both blk and
> scsi, as I don't see any other AioContext lock taken)?
dataplane_started is assigned before the host notifier is set up, which
I'm assuming is an implicit write barrier.
> Because I see that for example virtio_blk_req_complete is IO_CODE, so it
> could theoretically read dataplane_started while it is being changed in
> dataplane_stop? Even though I guess it doesn't because we disable and
> clean the host notifier before modifying it?
virtio_blk_data_plane_stop() has:
aio_context_acquire(s->ctx);
aio_wait_bh_oneshot(s->ctx, virtio_blk_data_plane_stop_bh, s);
/* Drain and try to switch bs back to the QEMU main loop. If other users
* keep the BlockBackend in the iothread, that's ok */
blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context(), NULL);
aio_context_release(s->ctx);
and disables host notifiers. At that point the IOThread no longer
receives virtqueue kicks and all in-flight requests have completed.
dataplane_started is only written afterwards so there is no race with
virtio_blk_req_complete().
>
> But if so, I don't get what is the difference with scsi code, and why we
> need to protect only that instance with the aiocontext lock?
The race condition I pointed out is not with virtio_blk_req_complete()
and data_plane_stop(). It's data_plane_start() racing with
virtio_blk_req_complete().
The virtio-scsi dataplane code is different for historical reasons and
happens to have the race. I don't think the virtio-blk code is affected.
Stefan
signature.asc
Description: PGP signature