On 09.03.23 13:08, Paolo Bonzini wrote:
On Thu, Mar 9, 2023 at 1:05 PM Paolo Bonzini <pbonz...@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