Re: deadlock when using iothread during backup_clean()

[Kevin Wolf]

Am 17.10.2023 um 12:18 hat Fiona Ebner geschrieben:
> Am 06.10.23 um 14:18 schrieb Fiona Ebner:
> > Am 04.10.23 um 19:08 schrieb Vladimir Sementsov-Ogievskiy:
> >> On 28.09.23 11:06, Fiona Ebner wrote:
> >>> For fixing the backup cancel deadlock, I tried the following:
> >>>
> >>>> diff --git a/blockjob.c b/blockjob.c
> >>>> index 58c5d64539..fd6132ebfe 100644
> >>>> --- a/blockjob.c
> >>>> +++ b/blockjob.c
> >>>> @@ -198,7 +198,9 @@ void block_job_remove_all_bdrv(BlockJob *job)
> >>>>        * one to make sure that such a concurrent access does not attempt
> >>>>        * to process an already freed BdrvChild.
> >>>>        */
> >>>> +    aio_context_release(job->job.aio_context);
> >>>>       bdrv_graph_wrlock(NULL);
> >>>> +    aio_context_acquire(job->job.aio_context);
> >>>>       while (job->nodes) {
> >>>>           GSList *l = job->nodes;
> >>>>           BdrvChild *c = l->data;
> >>>
> >>> but it's not enough unfortunately. And I don't just mean with the later
> >>> deadlock during bdrv_close() (via bdrv_cbw_drop()) as mentioned in the
> >>> other mail.
> >>>
> >>> Even when I got lucky and that deadlock didn't trigger by chance or with
> >>> an additional change to try and avoid that one
> >>>
> >>>> diff --git a/block.c b/block.c
> >>>> index e7f349b25c..02d2c4e777 100644
> >>>> --- a/block.c
> >>>> +++ b/block.c
> >>>> @@ -5165,7 +5165,7 @@ static void bdrv_close(BlockDriverState *bs)
> >>>>           bs->drv = NULL;
> >>>>       }
> >>>>   -    bdrv_graph_wrlock(NULL);
> >>>> +    bdrv_graph_wrlock(bs);
> >>>>       QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
> >>>>           bdrv_unref_child(bs, child);
> >>>>       }
> >>>
> >>> often guest IO would get completely stuck after canceling the backup.
> >>> There's nothing obvious to me in the backtraces at that point, but it
> >>> seems the vCPU and main threads running like usual, while the IO thread
> >>> is stuck in aio_poll(), i.e. never returns from the __ppoll() call. This
> >>> would happen with both, a VirtIO SCSI and a VirtIO block disk and with
> >>> both aio=io_uring and aio=threads.
> >>
> >> When IO is stuck, it may be helpful to look at bs->tracked_requests list
> >> in gdb. If there are requests, each one has field .co, which points to
> >> the coroutine of request.
> > 
> > After the IO was stuck in the guest, I used bdrv_next_all_states() to
> > iterate over the states and there's only the bdrv_raw and the
> > bdrv_host_device. For both, tracked_requests was empty.
> > 
> > What is also very interesting is that the IO isn't always dead
> > immediately. It can be that the fio command still runs with lower speed
> > for a while (sometimes even up to about a minute, but most often about
> > 10-15 seconds or so). During that time, I still can see calls to
> > virtio_scsi_handle_cmd() and blk_aio_write_entry(). Then they suddenly stop.
> > 
> >>>
> >>> I should also mention I'm using
> >>>
> >>>> fio --name=file --size=4k --direct=1 --rw=randwrite --bs=4k
> >>>> --ioengine=psync --numjobs=5 --runtime=6000 --time_based
> >>>
> >>> inside the guest during canceling of the backup.
> > 
> > One single time, it got stuck polling while canceling [0]. I usually
> > need to do the backup+cancel a few times, because the IO doesn't get
> > stuck each time, so this was a subsequent invocation. The reentrancy to
> > virtio_scsi_handle_cmd()/etc. seems strange at a first glance. Can that
> > be normal?
> > 
> 
> I ran into similar issues now with mirror, (both deadlocks and stuck
> guest IO at other times), and interestingly, also during job start.
> 
> Also had a backtrace similar to [0] once, so I took a closer look.
> Probably was obvious to others already, but for the record:
> 
> 1. the graph is locked by the main thread
> 2. the iothread holds the AioContext lock
> 3. the main thread waits on the AioContext lock
> 4. the iothread waits for coroutine spawned by blk_is_available()

Where does this blk_is_available() in the iothread come from? Having it
wait without dropping the AioContext lock sounds like something that
we'd want to avoid. Ideally, devices using iothreads shouldn't use
synchronous requests at all, but I think scsi-disk might have some of
them.

> As for why it doesn't progress, blk_co_is_available_entry() uses
> bdrv_graph_co_rdlock() and can't get it, because the main thread has the
> write lock. Should be fixed once the AioContext locks are gone, but not
> sure what should be done to avoid it until then.

Then the nested event loop in blk_is_available() would probably be
enough to make progress, yes.

Maybe we could actually drop the lock (and immediately reacquire it) in
AIO_WAIT_WHILE() even if we're in the home thread? That should give the
main thread a chance to make progress.

But I think we're actually not very far from removing the AioContext
lock, so if we can't find an easy fix in the meantime, waiting for that
could be a realistic option.

> I'm still trying to figure out what happens in the cases where the guest
> IO gets stuck. I should mention that I'm not experiencing the issues
> when disabling graph locking. It's rather obvious for the deadlocks, but
> I also can't reproduce the issues with stuck guest IO. Did it on top of
> QEMU 8.1.1, because stuff like bdrv_schedule_unref() came in later and I
> didn't want to mess that up :)

Kevin