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

[Jan Kara]

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@xxxxxxxxxx>
> > > > > 
> > > > > 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@xxxxxxx>
> > > > > Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> > > > 
> > > > 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
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR