Re: [PATCH 2/6] ext4: clarify return semantics of ext4_load_tail_bh()

[Jan Kara]

On Fri 03-07-26 11:39:03, Zhang Yi wrote:
> On 7/3/2026 12:43 AM, Jan Kara wrote:
> > On Thu 02-07-26 22:40:17, Zhang Yi wrote:
> >> On 7/2/2026 5:53 PM, Jan Kara wrote:
> >>> On Wed 01-07-26 22:20:05, yizhang089@xxxxxxxxx wrote:
> >>>> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> >>>>
> >>>> ext4_load_tail_bh() returns NULL for both holes and clean unwritten
> >>>> buffers, but the conditions that lead to this are not obvious from the
> >>>> code alone. Document this behavior to clarify the return value, so that
> >>>> readers do not mistakenly assume that only holes result in a NULL
> >>>> return.
> >>>>
> >>>> Also update the inline comment following the ext4_get_block() call to
> >>>> reflect this: both holes and clean unwritten buffers fall through to the
> >>>> "nothing to do" path.
> >>>>
> >>>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> >>>> ---
> >>>>   fs/ext4/inode.c | 6 +++++-
> >>>>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> >>>> index c2c2d6ac7f3d..0b31fa873743 100644
> >>>> --- a/fs/ext4/inode.c
> >>>> +++ b/fs/ext4/inode.c
> >>>> @@ -4026,6 +4026,10 @@ void ext4_set_aops(struct inode *inode)
> >>>>    * because it might have data in pagecache (eg, if called from ext4_zero_range,
> >>>>    * ext4_punch_hole, etc) which needs to be properly zeroed out. Otherwise a
> >>>>    * racing writeback can come later and flush the stale pagecache to disk.
> >>>> + *
> >>>> + * Return the loaded bh if it actually needs zeroing - in written, dirty
> >>>> + * unwritten, or delalloc state. Return NULL if it's clean (i.e., a hole or
> >>>> + * a clean unwritten block).
> >>>>    */
> >>>
> >>> Great that you're adding this comment because when I was reading previous
> >>> patch, I've spent like 10 minutes trying to figure that out :). But as far
> >>> as I'm reading the code, ext4_load_tail_bh() will return the unwritten bh
> >>> even if it is clean - map_bh() in _ext4_get_block() will set
> >>> buffer_mapped() even for unwritten extent. What am I missing? BTW, it also
> >>> seems to be ext4_load_tail_bh() could attempt to ext4_read_bh_lock() on
> >>> unwritten bh if things align wrongly...
> >>>
> >>> 								Honza
> >>
> >> Thank you for the review!
> >>
> >> Please note the ext4_update_bh_state(bh, map.m_flags) call in
> >> _ext4_get_block() — it restores the mapped flag back to unwritten. As a
> >> result, the !buffer_mapped(bh) check will evaluate to true for a clean
> >> unwritten block, the function will return NULL.
> > 
> > Argh, right. But ext4_ext_map_blocks() sets both BH_Unwritten and BH_Mapped
> > in the returned map->m_flags for unwritten extents. So I think my comment
> > still applies. 
> 
> Because ext4_get_block() is called with create = 0, this is purely a
> lookup operation. Therefore, in ext4_ext_map_blocks() ->
> ext4_ext_handle_unwritten_extents(), only EXT4_MAP_UNWRITTEN is set,
> and EXT4_MAP_MAPPED is not (please see the branch where
> if (flags & EXT4_GET_BLOCKS_CREATE) == 0)). So at this point, whether
> the lookup is served from the extent status cache or from the on-disk
> query, it works correctly now.

Aah, now I remember. Yes, we have a catch in the mapping code like that.
Thanks for reminding me again. Now back to my original question: So 
ext4_load_tail_bh() returns unwritten bh when it is dirty because someone
must have called ext4_get_block() on it with EXT4_GET_BLOCKS_CREATE and at
that point we set the BH_Mapped flag on it? Subtle... It would be worth to
write this explanation to that comment as well.

> > Plus there's a very strange inconsistency between this and
> > the lookup in extent status tree in ext4_map_blocks() where (as you
> > indicate) we only set BH_Unwritten but *not* BH_Mapped. AFAICT there's
> > something buggy in here :) Note that when we don't set BH_Mapped flag e.g.
> > from ext4_get_block_unwritten() (which gets used when delalloc is disabled),
> > then writes to unwritten extent will get lost because
> > ext4_bio_write_folio() will just treat them as holes...
> 
> In fact, there is no real problem here. For the write path that gets an
> unwritten block, EXT4_GET_BLOCKS_UNWRIT_EXT and
> EXT4_GET_BLOCKS_CREATE are always passed together. This ensures that
> even if we find the unwritten extent in the extent status cache, we
> still call ext4_map_create_blocks() (due to EXT4_GET_BLOCKS_CREATE)
> and go through ext4_ext_handle_unwritten_extents(). Inside that
> function, because the flags contain EXT4_GET_BLOCKS_UNWRIT_EXT,
> EXT4_MAP_MAPPED is set. So for the write path, ext4_bio_write_folio()
> still writes back correctly.
> 
> That said, the semantic inconsistency between the lookup-only and
> allocation paths has been bothering me for a while. I had to be very
> careful about this when developing the buffered I/O iomap path, because
> this semantics also differs from the iomap infrastructure. In iomap, for
> an unwritten extent, IOMAP_UNWRITTEN and IOMAP_MAPPED are not set
> simultaneously; IOMAP_MAPPED always represents a written extent. The
> good news is that this conversion is not complicated.
> 
> I asked an agent to look into the historical reasons, and here is what
> he told me:
> 
>   When ext4 introduced fallocate + delalloc in 2008-2009, it followed
>   the XFS convention that "unwritten is not mapped." However, ext4's
>   writeback path (mpage_da_submit_io) depended on BH_MAPPED to issue
>   I/O, which meant that unwritten buffers would never be written out.
>   Commit bf068ee266f9 was the first fix - it converted unwritten to
>   mapped during the writepages stage. Commit 29fa89d08894 was the
>   second fix - it made unwritten carry mapped at the write-begin stage.
>   Commit 2a8964d63d50 was a revert patch - it cleared the unwritten
>   flag in the create = 1 path to avoid duplicate get_block() calls.
> 
>   Then the extent status cache was introduced in 2013. Commits
>   a25a4e1a5d5d (Zheng Liu, 2013-02) and d100eef2440f (2013) added the
>   ES-cache lookup path. In that path, ext4_es_is_unwritten returns
>   EXT4_MAP_UNWRITTEN without EXT4_MAP_MAPPED - this was not a new
>   design decision, but rather a continuation of the old 2009 convention
>   that "unwritten is not mapped." Zheng Liu's commit message did not
>   discuss whether MAPPED should be set at the same time; it only said
>   "ext4_map_blocks needs to use this flag to determine the extent
>   status." He was simply implementing the ES-cache lookup, and the
>   status semantics followed the old implementation.
> 
>   The comment in ext4_map_blocks() - which says "if blocks have been
>   preallocated ext4_ext_map_blocks() returns with buffer head unmapped"
>   - was inherited from Aneesh's 2009 code. It was not a new decision
>   made in 2013.
> 
> So my understanding is that this inconsistency in ext4_map_blocks() is
> a historical legacy problem. There are many places where handling these
> two flags is also semantically ambiguous. Some may treat those with the
> BH_mapped flag set as written, such as ext4_load_tail_bh(). Therefore,
> setting the mapped flag may cause many issues, and it can only be done
> by setting the unwritten flag when performing a query. If I am wrong
> about any of this, please correct me.
> 
> Going forward, I think we should revisit and clearly redefine the
> semantics of these two flags, and unify the behavior of ext4_get_block().
> Otherwise, carrying this historical baggage will only hinder future
> development. :-)

Yeah, frankly I think it would be good to cleanup. But as you say it is
currently working correctly.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR