Re: [PATCH v3] ext4: do not advance i_size on a buffered write that copied nothing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: broken inline data conversion in ext4_da_truncate_failed_write()
--

commit c64fed07a736168af76e10e5cf487bb520de68ee
Author: Hengyu Liang <hengyul@cs.unc.edu>

ext4: do not advance i_size on a buffered write that copied nothing

This commit prevents ext4_write_end(), ext4_journalled_write_end(), and
ext4_da_do_write_end() from advancing i_size when a buffered write copies
zero bytes. In the delayed allocation path, it also adds
ext4_da_truncate_failed_write() to drop page cache folios and delayed
extents prepared beyond i_size after a failed or short write.

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 6ede3644a738b..9c20876629bc1 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c

[ ... ]

> @@ -3349,6 +3352,24 @@ static int ext4_da_should_update_i_disksize(struct folio *folio,
>  	return 1;
>  }
>  
> +/*
> + * Delalloc counterpart of ext4_truncate_failed_write(): ->write_begin() only
> + * reserved delayed blocks, so drop the page cache and the delayed extents
> + * beyond i_size and leave the blocks that are allocated there alone.
> + */
> +static void ext4_da_truncate_failed_write(struct inode *inode)
> +{
> +	struct address_space *mapping = inode->i_mapping;
> +	ext4_lblk_t lblk = EXT4_B_TO_LBLK(inode, inode->i_size);
> +
> +	filemap_invalidate_lock(mapping);
> +	truncate_inode_pages(mapping, inode->i_size);
> +	down_write(&EXT4_I(inode)->i_data_sem);
> +	ext4_es_remove_extent(inode, lblk, EXT_MAX_BLOCKS - lblk);
> +	up_write(&EXT4_I(inode)->i_data_sem);
> +	filemap_invalidate_unlock(mapping);
> +}
> +
>  static int ext4_da_do_write_end(struct address_space *mapping,
>  			loff_t pos, unsigned len, unsigned copied,
>  			struct folio *folio)
> @@ -3386,12 +3407,12 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	 * checked, we need to update i_disksize here as certain
>  	 * ext4_writepages() paths not allocating blocks and update i_disksize.
>  	 */
> -	if (new_i_size > inode->i_size) {
> +	if (copied && new_i_size > inode->i_size) {
>  		unsigned long end;
>  
>  		i_size_write(inode, new_i_size);
>  		end = offset_in_folio(folio, new_i_size - 1);
> -		if (copied && ext4_da_should_update_i_disksize(folio, end)) {
> +		if (ext4_da_should_update_i_disksize(folio, end)) {
>  			ext4_update_i_disksize(inode, new_i_size);
>  			disksize_changed = true;
>  		}
> @@ -3400,9 +3421,13 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	folio_unlock(folio);
>  	folio_put(folio);
>  
> -	if (pos > old_size)
> +	if (copied && pos > old_size)
>  		pagecache_isize_extended(inode, old_size, pos);
>  
> +	/* Drop what ->write_begin() prepared beyond the resulting i_size. */
> +	if (pos + len > inode->i_size)
> +		ext4_da_truncate_failed_write(inode);

[Severity: High]
Can calling ext4_da_truncate_failed_write() when inode->i_size == 0 leave an
inline-data inode in a half-converted state?

Suppose an inode has inline data initialized (EXT4_INODE_INLINE_DATA is set,
EXT4_INODE_EXTENTS is clear, EXT4_STATE_MAY_INLINE_DATA is set, and
i_inline_size == 60) with inode->i_size == 0 (for example, after truncating
an inline file to 0 or after a small write that failed with -EFAULT in
ext4_write_inline_data_end()).

If a user then issues a buffered write with pos + len exceeding
ext4_get_max_inline_size(inode) that copies 0 bytes, ext4_da_write_begin()
calls ext4_generic_write_inline_data() ->
ext4_da_convert_inline_data_to_extent():

fs/ext4/inline.c:ext4_da_convert_inline_data_to_extent() {
	...
	ret = ext4_block_write_begin(NULL, folio, 0, inline_size,
				     ext4_da_get_block_prep);
	...
	clear_buffer_new(folio_buffers(folio));
	folio_mark_dirty(folio);
	folio_mark_uptodate(folio);
	ext4_clear_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA);
	...
}

This instantiates folio 0, reserves a delayed block at lblk 0, marks folio 0
dirty, and clears EXT4_STATE_MAY_INLINE_DATA while leaving
EXT4_INODE_INLINE_DATA set so that ext4_do_writepages() will later call
ext4_destroy_inline_data() during writeback.

When ext4_da_do_write_end() runs with copied == 0, inode->i_size remains 0
and ext4_da_truncate_failed_write() is called with lblk == 0. That drops
dirty folio 0 and removes the delayed extent at lblk 0, leaving the inode
with EXT4_INODE_INLINE_DATA set, EXT4_INODE_EXTENTS clear,
EXT4_STATE_MAY_INLINE_DATA clear, and no dirty folio in the page cache:

1. If the 0-byte copy happened because the user buffer was not yet faulted in
   on a write at pos >= PAGE_SIZE, generic_perform_write() faults in the user
   page and retries ext4_da_write_begin(). Because EXT4_STATE_MAY_INLINE_DATA
   is now clear, ext4_da_write_begin() skips recreating folio 0, so
   writeback will not convert the inline data unless another write touches
   folio 0.

2. If a subsequent write enters ext4_write_begin() (for example via
   ext4_nonda_switch()), it skips ext4_try_to_write_inline_data() because
   EXT4_STATE_MAY_INLINE_DATA is clear, and _ext4_get_block() fails with
   -ERANGE because ext4_has_inline_data(inode) is still true.

3. If a subsequent delalloc write writes N <= i_inline_size bytes at pos 0
   (creating dirty folio 0 with BH_Delay and a delayed extent at lblk 0
   while EXT4_INODE_INLINE_DATA is still set) and the file is then truncated
   to M (where 0 < M < N) before writeback, ext4_setattr() skips
   ext4_convert_inline_data() because M <= i_inline_size, and calls
   ext4_inline_data_truncate():

fs/ext4/inline.c:ext4_inline_data_truncate() {
	...
	if (i_size < inline_size) {
		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
			ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
	...
}

   This removes the delayed extent at lblk 0 from the extent status tree
   while folio 0 remains dirty with BH_Delay in the page cache. When
   writeback runs, ext4_map_blocks() finds no delayed extent in the extent
   status tree and returns 0 under EXT4_GET_BLOCKS_IO_SUBMIT, triggering
   WARN_ON_ONCE(!err) in mpage_map_one_extent() and discarding the dirty
   folio in mpage_map_and_submit_extent().

Should ext4_da_truncate_failed_write() restore EXT4_STATE_MAY_INLINE_DATA if
ext4_has_inline_data(inode) is still true when truncating back to 0 (or
avoid dropping folio 0 / lblk 0 when inline data conversion is pending)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030040520.4138904-1-hengyul@cs.unc.edu?part=1