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

[Zhang Yi]

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.

Thanks,
Yi.

> >   static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
> >   {
> > @@ -4065,7 +4069,7 @@ static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
> >   	if (!buffer_mapped(bh)) {
> >   		BUFFER_TRACE(bh, "unmapped");
> >   		ext4_get_block(inode, iblock, bh, 0);
> > -		/* unmapped? It's a hole - nothing to do */
> > +		/* It's a hole or a clean unwritten block - nothing to do */
> >   		if (!buffer_mapped(bh)) {
> >   			BUFFER_TRACE(bh, "still unmapped");
> >   			goto unlock;
> > --
> > 2.53.0