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

[Jan Kara]

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. 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...

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