Re: [PATCH] ext4: fix extent status cache clearing when truncating inline data

[Jan Kara]

On Thu 15-10-26 10:18:22, Zhang Yi wrote:
> On 10/15/2026 12:27 AM, Jan Kara wrote:
> > On Wed 14-10-26 10:17:14, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> >>
> >> ext4_inline_data_truncate() clears the whole extent status tree of the
> >> inode when it truncates the inline data, with
> >>
> >> 	ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
> >>
> >> It does that when the inode still carries inline data while the
> >> EXT4_STATE_MAY_INLINE_DATA state bit is already cleared, which is the
> >> case once an inline to extent conversion has been initiated, for
> >> example a delalloc buffered write larger than the inline space.
> >>
> >> The clear is meant to drop stale extent status entries of such an inode
> >> to prevent stale extents and stale reserved space after the conversion,
> >> but the range is too wide.  It also drops the entries of the blocks
> >> below the new end of file, that is the blocks that stay in the file.
> >>
> >> When writeback of these blocks starts later, the lookup of such a block
> >> misses the extent status tree, falls back to the extent tree and caches
> >> a hole entry for it.  Those blocks then have dirty, mapped and delayed
> >> buffers together with a hole extent status entry.  This is an
> >> unexpected inconsistency.
> >>
> >> This problem has been hidden so far because we allocate blocks and new
> >> extents for them during writeback.  But once [1] is merged during the
> >> iomap conversion, the writeback behavior will be normalized, and it will
> >> no longer allocate blocks for holes, thus exposing this problem.
> > 
> > Hum, I don't understand. So we have inode with inline data. We truncate it
> > (i.e. further reduce i_size). At this point we hold i_rwsem,
> > invalidate_lock, i_data_sem. So how can race with inline data
> > conversion?
> > 
> 
> Hi Jan,
> 
> It is not a race, the state is left behind by an ordinary delalloc
> buffered write that ran before the truncate started.  Please take a look
> at ext4_da_convert_inline_data_to_extent().
> 
> A buffered write into an inline file that does not fit the inline space
> goes there.  It copies the inline data into the page cache, creates
> delayed allocation mappings for it, marks the folio dirty and clears
> EXT4_STATE_MAY_INLINE_DATA.  The non-delalloc variant
> ext4_convert_inline_data_to_extent() destroys the inline data with
> ext4_destroy_inline_data_nolock() right there, but it can do that
> because it allocates the blocks in the same transaction.  The delalloc
> path deliberately keeps the inline data, it is the only copy of the data
> until the blocks are created.  It is destroyed later by
> ext4_do_writepages() during writeback, which calls
> ext4_destroy_inline_data() when the inode still has inline data and it
> is about to create the blocks, see the "we will soon create the block
> for the 1st page, so we'd better clear the inline data here" comment.
> 
> So until that writeback runs, the inode has inline data, the
> EXT4_STATE_MAY_INLINE_DATA bit cleared, and a dirty page whose blocks
> are mapped as delayed allocations in the extent status tree.  A truncate
> that comes later is fully serialized and still sees
> ext4_has_inline_data() true, which is why it enters
> ext4_inline_data_truncate().
> 
> Those delayed allocation entries below the new end of file are what the
> unconditional ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS) drops,
> even though their dirty buffers are still in the page cache.

Right. Makes sense and thanks for explanation! I forgot we have a separate
delalloc inline data conversion handling. The fix looks good. Feel free to
add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> 
> Cheers,
> Yi.
> 
> > 
> >>
> >> Therefore, restrict the range to the blocks that are actually truncated
> >> away, and pass EXT_MAX_BLOCKS minus lblk as the length.
> >>
> >> [1] https://lore.kernel.org/linux-ext4/20261009103033.2920530-9-yi.zhang@xxxxxxxxxxxxxxx/
> >>
> >> Reported-by: syzbot ci <syzbot+ci46f64f7b6a2a188e@xxxxxxxxxxxxxxxxxxxxxxxxx>
> >> Closes: https://lore.kernel.org/linux-ext4/6ac92a4c.70ed5d86.2ac85.0072.GAE@xxxxxxxxxx/
> >> Fixes: 0add491df4e5 ("ext4: remove extent cache entries when truncating inline data")
> >> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> >> ---
> >>  fs/ext4/inline.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> >> index ceee69a66482b..b630e56815bb5 100644
> >> --- a/fs/ext4/inline.c
> >> +++ b/fs/ext4/inline.c
> >> @@ -1905,8 +1905,12 @@ int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
> >>  		 * the extent status cache must be cleared to avoid leaving
> >>  		 * behind stale delayed allocated extent entries
> >>  		 */
> >> -		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
> >> -			ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
> >> +		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
> >> +			ext4_lblk_t lblk = EXT4_B_TO_LBLK(inode, i_size);
> >> +
> >> +			ext4_es_remove_extent(inode, lblk,
> >> +					      EXT_MAX_BLOCKS - lblk);
> >> +		}
> >>  
> >>  		/* Clear the content in the xattr space. */
> >>  		if (inline_size > EXT4_MIN_INLINE_DATA_SIZE) {
> >> -- 
> >> 2.52.0
> >>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR