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

[Zhang Yi <yizhang089@gmail.com>]

On 7/2/2026 10:20 PM, Jan Kara wrote:
> On Tue 30-06-26 20:06:55, Zhang Yi wrote:
>> 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.
> 
> Good point, that needs to be handled.
> 
>> 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.
> 
> One thing we need to do is to perform i_size updates in append write,
> truncate, etc. under i_data_sem (similarly to i_disksize updates) so that
> we can properly synchronize i_size + i_disksize updates from IO completion
> and these paths.
> 
> How we could then handle the races between writeback of folio straddling
> i_disksize and the i_size updates from the i_size extending paths is that
> we'd set EXT4_STATE_DISKSIZE_GROW_PENDING bit when zeroing the folio
> straddling i_disksize and clear this (under i_data_sem) in IO completion
> when updating i_disksize to i_size. And when setting new i_size after
> append write, truncate up, etc. we will also update i_disksize when this
> state bit is not set (again all happening under i_data_sem). This should
> also automatically include the case where i_disksize was block aligned and
> thus no zeroing was needed and we need to update i_disksize together with
> i_size. Sounds good?
> 
> 								Honza

Ha, yeah. This overall looks good to me now. Your solution doesn't look
significantly different from mine currently implementation. My approach
uses i_ordered_len to precisely track the pending state of whether a
zeroed folio is awaiting submission, while you choose to use
EXT4_STATE_DISKSIZE_GROW_PENDING to represent this, which completely
eliminates the 32-byte bloat in ext4_inode_info.

However, I'd prefer we should still be able to keep the IO-completion
waiting logic instead of adding the orphan list. We can precisely
determine whether an IO is a "cross-i_disksize IO" by checking whether
its position straddles i_disksize, and when it completes, wake up the
worker waiting for it in interrupt context -- this won't deadlock (bio
end_io doesn't consume a worker thread). Besides, Since we plan to use
the EXT4_STATE_DISKSIZE_GROW_PENDING bit, the waiting doesn't require a
dedicated wait queue; wait_on_bit / wake_up_bit(&ei->i_state_flags,
EXT4_STATE_DISKSIZE_GROW_PENDING) should suffice.

Moreover, if we use the orphan list approach, it seems difficult to
decide when to release the orphan list entries. I'm concerned that
there may be concurrency issues that would need careful consideration
(For example, if the orphan list is deleted when the file is closed,
then after the last opener closes the file, is it necessary to wait for
synchronous writeback? If there is concurrent file expansion during the
writeback process, are there concurrency issues?) The original waiting
scheme is more straightforward to me.

In short, For v5, I think we should:
1) Track state via EXT4_STATE_DISKSIZE_GROW_PENDING instead of checking
    whether i_ordered_len is 0;
2) Determine the I/O type during writeback by checking i_disksize
    instead of the i_ordered_lblk parameter;
3) No longer use a dedicated wait queue; instead wait on
    EXT4_STATE_DISKSIZE_GROW_PENDING.

Thereforer, I think this solution won't simplify the logic much compared
to my current one; it will likely still require changes in quite a few
patches. The benefit is saving memory. I can't think of any other
potential issues for now, So I'm glad to have a try.

In addition, I'll need to address a few more details in v5:

1. Following earlier discussions with Ojaswin, I plan to drop the
    patch 07 "ext4: submit zeroed post-EOF data immediately in the iomap
    buffered I/O path" in the next version -- no longer submitting I/O
    immediately after zeroing the EOF block, to improve concurrency.
    Then, in ext4_iomap_writepages() we check the writeback range:
    if it behind the zeroed folio, we explicitly submit the zeroed folio
    to prevent the appending I/O from being wait too long.

2. We also need to handle the case where the zeroed folio is
    truncated before being written back. In that case, i_disksize must
    be updated directly since the folio no longer has a chance to be
    written back.

3. In the SYNC mode of fallocate and zero range, the zeroed folio
    needs to be submitted synchronously as well; otherwise i_disksize
    won't be updated immediately (which would violate SYNC semantics).


What do you think?


Besides, I've been revisiting why the iomap buffered I/O series disables
data=ordered mode these days. If we can directly solve the deadlock
problem in data=ordered mode, things would be much simpler. However,
IIRC, I recall that Ted previously mentioned planning to deprecate the
data=ordered mode in the future, so I'm not sure whether exploring this
direction is worthwhile. In any case, let me share my thoughts:

The root cause of this deadlock is that, during writeback, we cannot
start a new handle/transaction without stalling on the currently
committing transaction (T_N) to finish. Please see the Patch 07 with the
deadlock call graph:

  
https://github.com/zhangyi089/linux/commit/c1a0e32ebf134411edb600b9b602b4af9420d25d

When writeback tries to start T_{N+1}, it has two stalls involved:

   [ T_0 | T_1 | ... | T_N-1 |  T_N  ]

1. Waiting for the T_N to enter T_FLUSH if it has start committing, so a
    new running transaction can be created. This is one-directional
    waiting, not part of the deadlock cycle.
2. Waiting for log space, which is where the deadlock cycle below
    closes.

    T_{N+1} start  ->  waits for  ->  T_0/1/... checkpoint completion
    T_0 checkpoint ->  waits for  ->  T_N commit completion if it
                                      contians the same bh
    T_N commit     ->  waits for  ->  ordered data writeback
    ordered writeback = T_{N+1} start path  ->  (cycle)

Only case 2 is the actual deadlock. So what I'm thinking is could we try
to break the cycle at the T_0 checkpoint -> T_N commit edge? That is
when checkpoint hits a bh that T_N has adopted, write the pre-T_N
contents to the original on-disk location instead of waiting for T_N to
commit. To avoid excessive copying, this requires:

- Identifying T_N as the last transaction that can start without
   blocking on log space reclamation — once T_N starts, the remaining
   log space is enough to fit T_N's own handle credits, but not enough to
   fit another full j_max_transaction_buffers after that.
- When T_N adopts a bh still on an older checkpoint list, snapshot the
   bh contents at adoption time, so the older checkpoint can write that
   snapshot to disk independently of T_N's commit progress.

   [ T_0 | T_1 | ... | T_N-1 |    T_N    ]
                             |<--     -->|  -> T_N
                              (j_max_transaction_buffers)

1. Mark T_N as the last TX that started without obstruction. When a new
    handle joins a running transaction, evaluate whether the remaining
    log space, after subtracting this handle's credits, can still fit a
    full j_max_transaction_buffers. If not, the current transaction is
    committed immediately and the handle joins the next one, which
    becomes T_N — the last transaction that can start without log-space
    reclamation, T_N is then marked.
2. Snapshotting on adoption. In do_get_write_access(), when the current
    handle's transaction is T_N and the bh is still on an older
    checkpoint list, copy the bh contents into a new
    jh->b_checkpoint_data pointer(like b_frozen_data does);
3. Consuming the snapshot at checkpoint. In jbd2_log_do_checkpoint(),
    the current branch that handles an adopted bh by starting and waiting
    for T_N's commit is extended: if b_checkpoint_data is present, write
    it to the original block location and remove the bh from the
    checkpoint list — without waiting for T_N to commit.
4. Releasing b_checkpoint_data. Freed when the bh is unlinked from its
    checkpoint list or T_N is finished to commit.

This keeps the snapshot granularity at a minimum, only T_N's adopted
buffers get the extra copy. When metadata changes are light, T_N rarely
adopts such buffers, and the cost is unnoticeable; when metadata changes
are heavy , the dominant cost is the metadata writeback itself, and the
extra in-memory copy is unlikely to be the bottleneck. However, the
downside is that there may be some memory overhead, especially in the
metadata-intensive cases.

Another possible optimization: at T_N start, opportunistically try to
push the checkpoint tail. If enough space is reclaimed that another full
j_max_transaction_buffers fits after T_N's handle credits, T_N is no
longer "last" and the snapshotting can be skipped entirely.

If this scheme works, I think we should be able to completely remove the
limitation that writeback cannot start a handle in data=ordered mode.
This would also benefit the buffer_head path — perhaps we could
deprecate the reserved handle mechanism.

This is just a preliminary idea. What do you think of this approach?

Thanks,
Yl.