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

On Mon, Aug 03, 2026 at 09:31:53AM +0800, Zhang Yi wrote:
> 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.

Cool, thanks for confirming :)

Regards,
ojaswin

> 
> Thanks,
> Yi.
> 
>