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

[Hanna Czenczek]

On 09.03.23 13:08, Paolo Bonzini wrote:
> On Thu, Mar 9, 2023 at 1:05 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > I think having to do this is problematic, because the blk_drain should
> > leave no pending operation.
> >
> > Here it seems okay because you do it in a controlled situation, but the
> > main thread can also issue blk_drain(), or worse bdrv_drained_begin(),
> > and there would be pending I/O operations when it returns.

Not really.  We would stop in the middle of a trim that processes a list 
of discard requests.  So I see it more like stopping in the middle of 
anything that processes guest requests.  Once drain ends, we continue 
processing them, and that’s not exactly pending I/O.

There is a pending object in s->bus->dma->aiocb on the IDE side, so 
there is a pending DMA operation, but naïvely, I don’t see that as a 
problem.

> > Unfortunately I don't have a solution (I'm not considering the idea of
> > using disable_request_queuing and having even more atomics magic in
> > block/block-backend.c), but I'll read the thread.

I wouldn’t disable request queuing, because its introducing commit 
message (cf3129323f900ef5ddbccbe86e4fa801e88c566e) specifically says it 
fixes IDE.  I suppose the reason might actually be this problem here, in 
that before request queuing, it was possible that IDE would continue 
issuing discard requests even while drained, because processing the list 
didn’t stop.  Maybe.

Or the issue is generally that IDE uses dma_* functions, which might 
cause I/O functions to be run from new BHs (I guess through 
reschedule_dma()?).

> Hmm, what about making blk_aio_prwv non-static and calling
> bdrv_co_pdiscard directly from IDE?

You mean transforming ide_issue_trim_cb() into an iterative coroutine 
(instead of being recursive and using AIO) and invoking it via 
blk_aio_prwv()?

It doesn’t feel right to call a bdrv_* function directly from a user 
external to the core block layer, so in this case I’d rather fall back 
to Fiona’s idea of invoking all discards concurrently.

Hanna