On 1/26/26 16:07, Hanna Czenczek wrote:
> On 29.12.25 17:07, Denis V. Lunev wrote:
>> This helper is mandatory to be called before bdrv_co_flush(). In the
>> other case bdrv_co_flush() will be noop. This helper should be called
>> after actual write is completed for subsequent flush to perform some
>> work.
>>
>> Actually this change is important, without it BDRV_REQ_FUA semantics is
>> broken completely. flush() is not called. This smells like potential
>> data loss if somebody relies on BDRV_REQ_FUA.
>>
>> Signed-off-by: Denis V. Lunev <[email protected]>
>> CC: Andrey Drobyshev <[email protected]>
>> CC: Kevin Wolf <[email protected]>
>> CC: Hanna Reitz <[email protected]>
>> ---
>> block/io.c | 20 ++++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
> Definitely an improvement, but if possible, I would still prefer keeping
> the increment it in bdrv_co_write_req_finish(), because that is where
> all write requests eventually end up in, and that’s also where dirty
> bitmaps are handled. It seems slightly error-prone to manually have to
> call this every time the BDS data is modified.
>
> Can we maybe instead pull the FUA emulation out into
> bdrv_aligned_pwritev(), after bdrv_co_write_req_finish()?
>
> I think we can only pull out the flush, not the FUA emulation detection
> because for write-zeroes, we have to check either supported_zero_flags
> or supported_write_flags, depending on whether we fall back or not. But
> bdrv_driver_pwritev() and bdrv_co_do_pwrite_zeroes() could return an
> need_emulate_fua flag via pointer parameter, and if set,
> bdrv_aligned_pwritev() can then do the flush after
> bdrv_co_write_req_finish(). (This would also allow dropping both “only
> flush once at the end” blocks.)
>
> Maybe it would then also make sense to move the bdrv_co_flush() into
> bdrv_co_write_req_finish(), and add a need_flush parameter to it, so
> that it can see whether the whole request (write + flush) was successful.
>
> Hanna
>
I have tried to think about this problem in a this and that way
and found yet another missed place in the code. This has
frustrated me even more.
For the reference:
raw_co_prw(BlockDriverState *bs, int64_t *offset_ptr, uint64_t bytes,
QEMUIOVector *qiov, int type, int flags)
{
...
assert(qiov->size == bytes);
ret = raw_thread_pool_submit(handle_aiocb_rw, &acb);
if (ret == 0 && (flags & BDRV_REQ_FUA)) {
/* TODO Use pwritev2() instead if it's available */
ret = bdrv_co_flush(bs);
}
goto out; /* Avoid the compiler err of unused label */
This definitely states that the code is cumbersome and you
right that I need to try harder to move FUA emulation out
into generic code.
I'll invest more here.
Thank you for the point,
Den
