Re: [PATCH v2] ide: Increment BB in-flight counter for TRIM BH

[John Snow]

On Tue, Feb 15, 2022 at 12:14 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> Ping
>
> (I can take it too, if you’d like, John, but you’re listed as the only
> maintainer for hw/ide, so...  Just say the word, though!)
>

Sorry, I sent you a mail off-list at the time where I said you were
free to take it whenever you like. Why'd I send it off-list? I don't
know....

Please feel free to send this with your next block PR.

--js

> On 20.01.22 15:22, Hanna Reitz wrote:
> > When we still have an AIOCB registered for DMA operations, we try to
> > settle the respective operation by draining the BlockBackend associated
> > with the IDE device.
> >
> > However, this assumes that every DMA operation is associated with an
> > increment of the BlockBackend’s in-flight counter (e.g. through some
> > ongoing I/O operation), so that draining the BB until its in-flight
> > counter reaches 0 will settle all DMA operations.  That is not the case:
> > For TRIM, the guest can issue a zero-length operation that will not
> > result in any I/O operation forwarded to the BlockBackend, and also not
> > increment the in-flight counter in any other way.  In such a case,
> > blk_drain() will be a no-op if no other operations are in flight.
> >
> > It is clear that if blk_drain() is a no-op, the value of
> > s->bus->dma->aiocb will not change between checking it in the `if`
> > condition and asserting that it is NULL after blk_drain().
> >
> > The particular problem is that ide_issue_trim() creates a BH
> > (ide_trim_bh_cb()) to settle the TRIM request: iocb->common.cb() is
> > ide_dma_cb(), which will either create a new request, or find the
> > transfer to be done and call ide_set_inactive(), which clears
> > s->bus->dma->aiocb.  Therefore, the blk_drain() must wait for
> > ide_trim_bh_cb() to run, which currently it will not always do.
> >
> > To fix this issue, we increment the BlockBackend's in-flight counter
> > when the TRIM operation begins (in ide_issue_trim(), when the
> > ide_trim_bh_cb() BH is created) and decrement it when ide_trim_bh_cb()
> > is done.
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2029980
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> > ---
> > v1:
> > https://lists.nongnu.org/archive/html/qemu-block/2022-01/msg00024.html
> >
> > v2:
> > - Increment BB’s in-flight counter while the BH is active so that
> >    blk_drain() will poll until the BH is done, as suggested by Paolo
> >
> > (No git-backport-diff, because this patch was basically completely
> > rewritten, so it wouldn’t be worth it.)
> > ---
> >   hw/ide/core.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
>