Re: [PATCH for-8.0] ide: Fix manual in-flight count for TRIM BH

[Kevin Wolf]

Am 10.03.2023 um 16:13 hat Paolo Bonzini geschrieben:
> On Fri, Mar 10, 2023 at 3:25 PM Kevin Wolf <kwolf@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