[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Paolo Bonzini
Subject: Re: [PATCH for-8.0] ide: Fix manual in-flight count for TRIM BH
Date: Thu, 9 Mar 2023 14:59:11 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]