Re: [PATCH v4 18/23] ext4: wait for ordered I/O in the iomap buffered I/O path

[Zhang Yi <yizhang089@gmail.com>]

On 6/25/2026 5:42 PM, Jan Kara wrote:
> On Mon 22-06-26 11:32:16, Zhang Yi wrote:
>> On 6/18/2026 9:48 PM, Jan Kara wrote:
>>> On Mon 11-05-26 15:23:38, Zhang Yi wrote:
>>>> From: Zhang Yi <yi.zhang@huawei.com>
>>>>
>>>> For append writes, wait for ordered I/O to complete before updating
>>>> i_disksize. This ensures that zeroed data is flushed to disk before the
>>>> metadata update, preventing stale data from being exposed during
>>>> unaligned post-EOF append writes.
>>>>
>>>> Suggested-by: Jan Kara <jack@suse.cz>
>>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>>
>>> Frankly, this all looks too complex to me. Plus your are adding 32-bytes to
>>> struct ext4_inode_info which isn't great either. Why don't you just do
>>> filemap_fdatawait() for the byte at old i_disksize and be done with it?
>>>
>>> I believe we have to simplify this. All this complexity (and thus
>>> maintenance burden) across several patches for the corner case of zeroing
>>> tail block on extention is in my opinion difficult to justify.
>>>
>>> 								Honza
>>
>> Hi, Jan!
>>
>> Thanks for the review. I understand the concern about complexity and the
>> 32-byte increase to ext4_inode_info. I tried using
>> filemap_fdatawait_range() as you suggested, but found two issues where
>> this solution doesn't work.
>>
>> 1. ioend worker deadlock
>>
>> Since worker concurrency resources are limited, we cannot wait for
>> another ioend worker to complete within one ioend worker with the same
>> work_struct. If the worker calls
>> filemap_fdatawait_range(byte_at_old_disksize) to wait for the zeroed
>> block's folio writeback to complete, it sleeps holding the only worker
>> slot. If the folio contains blocks requiring extent conversion, its
>> writeback bit is cleared by iomap_finish_ioends() running inside
>> another worker -- which can only run after the current worker finishes
>> its batch.
>>
>> Concretely:
>>    - Worker W1 processes ioend A, calls filemap_fdatawait_range() on
>>      the old EOF byte, sleeps.
>>    - The zeroed data is in ioend B. bio_endio defers it to
>>      i_iomap_ioend_list and calls queue_work().
>>    - queue_work() on i_iomap_ioend_work is idempotent: it returns false
>>      because the work is currently executing (even though sleeping).
>>    - ioend B sits in the list, never gets processed.
>>    - The folio writeback bit is only cleared by processing ioend B.
>>    - W1 sleeps forever -> deadlock.
> 
> Yes, good point. We cannot wait for folio writeback completion from end_io
> processing for another folio.
> 
>> Therefore, I think we have to put the wakeup logic in
>> ext4_iomap_end_bio() that runs in interrupt context without consuming
>> a worker thread. The ordered range tracking and wait queue are what
>> make that possible.
>>
>> 2. Truncate-up needs an accurate state query
>>
>> In the follow-up patch 19, ext4_set_inode_size() must make a precise
>> decision when updating i_disksize during truncate up.
>>
>> This needs a state query: "is there ordered zero I/O in flight right
>> now?" If yes, the i_disksize update is deferred to
>> ext4_iomap_wb_update_disksize(is_ordered=true), which advances
>> i_disksize to i_size when the ordered I/O completes. If no, we must
>> advance i_disksize immediately, otherwise we will lose the updating
>> forever.
>>
>> Therefore, we need to track the state of the ordered range. Simply
>> using filemap_fdatawait_range() doesn't work. i_ordered_len serves as
>> a maintained state flag that both the ioend completion path and the
>> setattr path can read atomically without sleeping.
>>
>> Suggestions?
> 
> I see. Thanks for explanation. I went back to our discussion back from
> February to remind myself about the constraints on the tail block zeroing
> and the i_disksize update mechanism. And in the light of complexity of the
> current mechanism, I think we've discarded the following possibility too
> easily:
> 
> * On file extend / truncate up just zero tail folio in the page cache, mark
>    it dirty, keep i_disksize at old value, update i_size to the new value,
>    add inode to orphan list.
>    If the i_disksize was block aligned (and so we skip zeroing), we just
>    update i_disksize rightaway.
> 
> * In io end processing if the folio for which we end io has a block which
>    straddles i_disksize, we update i_disksize to current i_size. We defer
>    removing inode from orphan list e.g. to file close time (doing it from
>    end_io processing is problematic locking wise as we need i_rwsem for it).
> 
> This is a very simple scheme with very good performace. It makes sure stale
> data in the tail block cannot be seen on disk after a crash.

Hmm, I think this solution has a problem. Since i_disksize is advanced
to i_size only when the ioend covering the i_disksize block completes,
we must ensure that i_size is updated before the zeroed folio's
writeback completes. Otherwise the ioend worker reads the old i_size,
advances i_disksize to the old value (a no-op), and i_disksize stays
stale forever after i_size is updated.

Currently, neither truncate-up nor file-extend paths guarantee this
ordering -- in both, ext4_block_zero_eof() runs before the i_size
update.

For truncate-up and fallocate, we might be able to split
ext4_update_inode_size() / ext4_set_inode_size() so that i_size is
updated before ext4_block_zero_eof(), and then decide whether to
update i_disksize based on whether i_disksize is block-aligned. I'm
not yet sure what side effects this reordering would have, but I feel
uneasy about this.

The append-write case is hard to solve. We can't know the final i_size
before the write completes, so we can't update i_size ahead of
ext4_block_zero_eof(). When the zeroed folio is later written back, it's
treated as an ordinary overwriting write -- i_disksize is not advanced.
And when the subsequent buffered-write data is written back, i_disksize
still isn't advanced either. The size update is lost.

> 
> Potential problems with this:
> 
> * We need to do i_disksize update from io end processing which means starting a
>    transaction. So this mechanism has to be restricted to buffered IO iomap
>    path due to locking constraints. If the inode uses old ordered mode, it
>    has to keep using it also for handling of the tail block zeroing.
>    I think that's acceptable as we want to transition everything to iomap
>    anyway so this duplicity will eventually go away.
> 
> * After a crash we can see i_disksize already updated but file content will
>    show zeros. This is not breaking any guarantees, it just changes how ext4
>    behaves after a crash. Again, I think this is acceptable tradeoff for the
>    simplicity.

These two points are fine with me. My current implementation also has
these same tradeoffs.

> 
> What do you think? Any problems I have missed? I'm sorry for poking into
> this mechanism again and again but I want to keep the code as simple as
> possible to make our life easier in the future...

That's fine. Although it's a corner case, it's quite subtle, and
handling it properly takes some careful thought. I would very much
like to solve it in a simple and elegant way. Any good suggestions
are welcome.

Thanks,
Yi.

> 
>> Regarding the bloat of ext4_inode_info, perhaps we can drop the
>> wait_queue_head_t (24 bytes) and use wait_var_event()/ wake_up_var()
>> instead. Would this be acceptable?
> 
> Yes, that would be a good way to reduce the bloat if we still need this.
> 
> 								Honza