Re: [PATCH 2/2] ext4: protect WRITE_ZEROES written extents with orphan list

On 7/31/2026 8:06 PM, Ojaswin Mujoo wrote:
> On Wed, Jul 29, 2026 at 04:59:18PM +0800, Zhang Yi wrote:
>> From: Zhang Yi <[email protected]>
>>
>> In ext4_alloc_file_blocks(), the WRITE_ZEROES path converts unwritten
>> extents to written in one transaction, while i_disksize is updated to
>> cover them only in a later transaction. A crash in between leaves
>> written extents beyond i_disksize on disk, which fsck will complain
>> about.
>>
>> To fix this, add the inode to the orphan list in the same handle that
>> does the conversion, and remove it once i_disksize has caught up.
>> Also add a sanity check to ensure conversion does not extend beyond EOF.
>>
>> Since ext4_alloc_file_blocks() is called from the fallocate() path,
>> partial allocation is safe. On partial conversion failure, advance
>> i_disksize only up to the boundary of successfully converted blocks, so
>> that orphan cleanup sees a consistent state. Document this behavior in
>> the function comment.
> 
> Hi Yi,
> 
> Look good,
> 
> Reviewed-by: Ojaswin Mujoo <[email protected]>

Hi, Ojaswin,

Thank you for the review!

> 
> Just a small question below:
>>
>> Reported-by: Jan Kara <[email protected]>
>> Closes: https://lore.kernel.org/linux-ext4/3f6ao5amv7glbgigndtegcucgo3n34ij3lau6l3da3hgdxgn3v@ev66wv3r5umt/
>> Fixes: f4265b8d32c4 ("ext4: add FALLOC_FL_WRITE_ZEROES support")
>> Cc: [email protected]
>> Signed-off-by: Zhang Yi <[email protected]>
>> ---
>>  fs/ext4/extents.c | 78 ++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 70 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 1ab1a6e2ed83..a3dde7ba0d23 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -4571,6 +4571,22 @@ int ext4_ext_truncate(handle_t *handle, struct inode *inode)
>>  	return err;
>>  }
>>  
>> +/*
>> + * Pre-allocate blocks for the range [@offset, @offset + @len). Allocated
>> + * blocks are marked as unwritten by default. If EXT4_GET_BLOCKS_ZERO is
>> + * set, the allocated blocks are zeroed on disk and their extents are
>> + * converted to written state.
>> + *
>> + * When @new_size is nonzero, the caller intends to extend the file, and
>> + * the file size should be updated to the end of the allocated blocks.
>> + *
>> + * Allocation may partially succeed due to some non-fatal issues. In that
>> + * case, i_disksize (and i_size) is advanced up to the successfully
>> + * processed portion of the range.
>> + *
>> + * Return 0 on success, or a negative error code on failure or partial
>> + * failure.
>> + */
>>  static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>>  				  loff_t new_size, int flags)
>>  {
>> @@ -4585,6 +4601,7 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>>  	loff_t epos = 0, old_size = i_size_read(inode);
>>  	unsigned int blkbits = inode->i_blkbits;
>>  	bool alloc_zero = false;
>> +	bool orphan = false;
>>  
>>  	BUG_ON(!ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS));
>>  	map.m_lblk = offset >> blkbits;
>> @@ -4659,19 +4676,49 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>>  
>>  		if (alloc_zero &&
>>  		    (map.m_flags & (EXT4_MAP_MAPPED | EXT4_MAP_UNWRITTEN))) {
>> +			ext4_lblk_t converted;
>> +
>> +			WARN_ON_ONCE(map.m_lblk + map.m_len >
>> +				EXT4_B_TO_LBLK(inode, new_size ?: old_size));
>> +
>>  			ret = ext4_issue_zeroout(inode, map.m_lblk, map.m_pblk,
>>  						 map.m_len);
>> -			if (likely(!ret))
>> -				ret = ext4_convert_unwritten_extents(NULL,
>> -					inode, (loff_t)map.m_lblk << blkbits,
>> -					(loff_t)map.m_len << blkbits, NULL);
>> -			if (ret)
>> +			if (unlikely(ret))
>>  				break;
>> +
>> +			handle = ext4_journal_start(inode, EXT4_HT_MAP_BLOCKS,
>> +						    credits);
>> +			if (IS_ERR(handle)) {
>> +				ret = PTR_ERR(handle);
>> +				break;
>> +			}
>> +
>> +			ret = ext4_convert_unwritten_extents(handle,
>> +					inode, (loff_t)map.m_lblk << blkbits,
>> +					(loff_t)map.m_len << blkbits,
>> +					&converted);
>> +			if (ret)
>> +				map.m_len = converted;
>> +
>> +			/*
>> +			 * If blocks beyond i_disksize are converted, add
>> +			 * the inode to the orphan list and advance the epos.
>> +			 */
>> +			if (new_size && converted) {
>> +				ret2 = ext4_orphan_add(handle, inode);
>> +				ret = ret ? ret : ret2;
>> +				orphan = true;
>> +			}
>> +
>> +			ret3 = ext4_journal_stop(handle);
>> +			ret = ret ? ret : ret3;
>>  		}
>>  
>>  		map.m_lblk += map.m_len;
>>  		map.m_len = len_lblk = len_lblk - map.m_len;
>>  		epos = EXT4_LBLK_TO_B(inode, map.m_lblk);
>> +		if (ret)
>> +			break;
>>  	}
>>  
>>  	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>> @@ -4687,11 +4734,23 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>>  	if (epos > new_size)
>>  		epos = new_size;
>>  
>> -	handle = ext4_journal_start(inode, EXT4_HT_MISC, 1);
>> -	if (IS_ERR(handle))
>> -		return ret ? ret : PTR_ERR(handle);
>> +	handle = ext4_journal_start(inode, EXT4_HT_MISC, 2);
>> +	if (IS_ERR(handle)) {
>> +		/*
>> +		 * The conversion has successfully completed. Not much to
>> +		 * do with the error here so just cleanup the orphan list
>> +		 * and hope for the best.
>> +		 */
>> +		if (orphan && inode->i_nlink)
>> +			ext4_orphan_del(NULL, inode);
>> +		ret2 = PTR_ERR(handle);
>> +		goto out;
>> +	}
>>  
>>  	ext4_update_inode_size(inode, epos);
>> +	if (orphan && inode->i_nlink)
>> +		ext4_orphan_del(handle, inode);
>> +
> 
> So I believe the i_nlink check is incase the inode is already at i_nlink
> = 0 before the fallocate() was called and hence mostly already on the
> orphan list. We need to skip deletion from the list as inode eviction
> will handle cleaning it up, right?

Yes, after ext4_evict_inode() has reclaimed all the file blocks, the
inode will be cleared from the orphan list.

Thanks,
Yi.