Am 10.03.2023 um 16:13 hat Paolo Bonzini geschrieben: > On Fri, Mar 10, 2023 at 3:25 PM Kevin Wolf <kw...@redhat.com> wrote: > > > 1. The TRIM operation should be completed on the IDE level before > > > draining ends. > > > 2. Block layer requests issued after draining has begun are queued. > > > > > > To me, the conclusion seems to be: > > > Issue all block layer requests belonging to the IDE TRIM operation up > > > front. > > > > > > The other alternative I see is to break assumption 2, introduce a way > > > to not queue certain requests while drained, and use it for the > > > recursive requests issued by ide_issue_trim_cb. But not the initial > > > one, if that would defeat the purpose of request queuing. Of course > > > this can't be done if QEMU relies on the assumption in other places > > > already. > > > > I feel like this should be allowed because if anyone has exclusive > > access in this scenario, it's IDE, so it should be able to bypass the > > queuing. Of course, the queuing is still needed if someone else drained > > the backend, so we can't just make TRIM bypass it in general. And if you > > make it conditional on IDE being in blk_drain(), it already starts to > > become ugly again... > > > > So maybe the while loop is unavoidable. > > > > Hmm... But could ide_cancel_dma_sync() just directly use > > AIO_WAIT_WHILE(s->bus->dma->aiocb) instead of using blk_drain()? > > While that should work, it would not fix other uses of > bdrv_drain_all(), for example in softmmu/cpus.c. Stopping the device > model relies on those to run *until the device model has finished > submitting requests*.
If so, do_vm_stop() really expects drain to do something it isn't designed to do. It's only for quiescing backends, not for any other activity a qdev device might still be doing. I think it's really the vm_state_notify() that should take care of stopping device activity. But maybe we can make it work with drain anyway. > So I still think that this bug is a symptom of a problem in the design > of request queuing. > > In fact, shouldn't request queuing was enabled at the _end_ of > bdrv_drained_begin (once the BlockBackend has reached a quiescent > state on its own terms), rather than at the beginning (which leads to > deadlocks like this one)? No, I don't think that is ever right. As I said earlier in this thread (and you said yourself previously), there are two different users of drain: 1. I want to have exclusive access to the node. This one wants request queuing from the start to avoid losing time unnecessarily until the guest stops sending new requests. 2. I want to wait for my requests to complete. This one never wants request queuing. Enabling it at the end of bdrv_drained_begin() wouldn't hurt it (because it has already achieved its goal then), but it's also not necessary for the same reason. IDE reset and do_vm_stop() are case 2, implemented with blk_drain*(). The request queuing was implemented for case 1, something else in the block graph draining the BlockBackend's root node with bdrv_drain*(). So maybe what we could take from this is that request queuing should be temporarily disabled while we're in blk_drain*() because these interfaces are only meant for case 2. In all other cases, it should continue to work as it does now. Kevin
