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

[Zhang Yi]

On 7/8/2026 11:26 PM, Jan Kara wrote:
> On Tue 07-07-26 10:13:34, Zhang Yi wrote:
>> 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, and note that a lookup-only get_block (without
>> EXT4_GET_BLOCKS_CREATE) never sets BH_Mapped for clean unwritten
>> extents, which is why a clean unwritten bh falls through to the
>> "nothing to do" path.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@xxxxxxx>
> 
> 								Honza

Hi, Jan!

Thank you for the review. I have already sent out v3 of this series.
Sorry I forgot to mention it earlier. It includes fixes for some
pre-existing issues in Sashiko. Could you please take a look at that
version?

https://lore.kernel.org/linux-ext4/20260708062049.1982410-1-yi.zhang@xxxxxxxxxxxxxxx/

Thanks,
Yi.

> 
>> ---
>>  fs/ext4/inode.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index c2c2d6ac7f3d..45a7e6cdb2a1 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).
>>   */
>>  static struct buffer_head *ext4_load_tail_bh(struct inode *inode, loff_t from)
>>  {
>> @@ -4065,7 +4069,12 @@ 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.
>> +		 * Note that a lookup-only get_block (without
>> +		 * EXT4_GET_BLOCKS_CREATE) never sets BH_Mapped for clean
>> +		 * unwritten extents.
>> +		 */
>>  		if (!buffer_mapped(bh)) {
>>  			BUFFER_TRACE(bh, "still unmapped");
>>  			goto unlock;
>> -- 
>> 2.52.0
>>