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

[Paolo Bonzini <pbonzini@redhat.com>]

On 3/9/23 13:31, Hanna Czenczek wrote:
> 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.

What about the bdrv_drain_all() when a VM stops, would the guest 
continue to access memory and disks after bdrv_drain() return?

Migration could also be a problem, because the partial TRIM would not be 
recorded in the s->bus->error_status field of IDEState (no surprise 
there, it's not an error).  Also, errors happening after bdrv_drain() 
might not be migrated correctly.

> 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()?).

Ah, you mean that you can have pending I/O operations while 
blk->in_flight is zero?  That would be a problem indeed.  We already 
have BlockDevOps for ide-cd and ide-hd, should we add a .drained_poll 
callback there?

>> 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.

Yeah, honestly it doesn't feel very much right to me either.

Paolo