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

[Zhang Yi <yizhang089@gmail.com>]

On 7/2/2026 5:53 PM, Jan Kara wrote:
> On Wed 01-07-26 22:20:05, yizhang089@gmail.com wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> 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@huawei.com>
>> ---
>>   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
>>