Re: [PATCH -next v5 27/32] ext4: clear DISKSIZE_GROW_PENDING on truncate or error

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The EXT4_STATE_DISKSIZE_GROW_PENDING inode state bit is never set anywhere in the codebase. As a result, the entire deferred disksize update mechanism introduced in this patch series is non-functional.
- [Medium] The DISKSIZE_GROW_PENDING state is not cleared for linked inodes (i_nlink > 0) during eviction, leading to a WARN_ON_ONCE in ext4_destroy_inode() on forced shutdown.
--

--- Patch [27]: [PATCH -next v5 27/32] ext4: clear DISKSIZE_GROW_PENDING on truncate or error ---
commit a9d36fc1a5b406105120b9e8fd8d91b4a9fe12f0
Author: Zhang Yi <[email protected]>

ext4: clear DISKSIZE_GROW_PENDING on truncate or error

The disksize-grow-pending state is set when a zeroed EOF block is queued
for writeback and cleared by the ioend completion path once writeback
finishes. This patch clears the disksize-grow-pending state on discard,
inode eviction, filesystem emergency state, or truncate to avoid blocking
writebacks and preventing warnings in ext4_destroy_inode().

> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 056937e278594..a1dfb70127ca8 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -273,6 +273,8 @@ void ext4_evict_inode(struct inode *inode)
>  
>  	if (ext4_should_order_data(inode))
>  		ext4_begin_ordered_truncate(inode, 0);
> +	if (ext4_inode_buffered_iomap(inode))
> +		ext4_iomap_clear_disksize_pending(inode);

[Severity: Medium]
If a filesystem is forcibly shut down (e.g., via EXT4_IOC_SHUTDOWN without
logging an error that sets EXT4_ERROR_FS) and then unmounted, dirty linked
inodes will be evicted.

Looking earlier in ext4_evict_inode(), if inode->i_nlink is > 0, the code 
jumps to the no_delete label, completely bypassing this cleanup:

	if (inode->i_nlink) {
		...
		goto no_delete;
	}

Will this cause the newly added WARN_ON_ONCE in ext4_destroy_inode() to
trigger for linked inodes since they bypass this state clearing?

>  	truncate_inode_pages_final(&inode->i_data);
>  
>  	/*

[ ... ]

> @@ -6741,7 +6759,18 @@ static int ext4_truncate_down(struct inode *inode, loff_t oldsize,
>  	start_lblk = newsize > 0 ? (newsize - 1) >> inode->i_blkbits : 0;
>  	ext4_fc_track_range(handle, inode, start_lblk, EXT_MAX_BLOCKS - 1);
>  
> -	ext4_set_inode_size(inode, newsize);
> +	down_write(&EXT4_I(inode)->i_data_sem);
> +	/*
> +	 * Truncate the zeroed EOF block invalidates the pending disksize
> +	 * update, so clear the disksize-grow-pending state.
> +	 */
> +	if (ext4_test_inode_state(inode, EXT4_STATE_DISKSIZE_GROW_PENDING) &&
> +	    (newsize <= EXT4_I(inode)->i_disksize))
> +		ext4_iomap_clear_disksize_pending(inode);

[Severity: High]
Is the EXT4_STATE_DISKSIZE_GROW_PENDING bit ever set anywhere in the
codebase? 

A search of the tree indicates the literal EXT4_STATE_DISKSIZE_GROW_PENDING
is only ever read or cleared, never set via ext4_set_inode_state() or 
similar. 

There is a TODO comment in ext4_block_zero_eof() indicating that this state
should be set:

	/*
	 * TODO: In the iomap path, handle this by tracking the ordered
	 * range and updating i_disksize to i_size after the zeroed data
	 * has been written back.
	 */

If the bit is never actually set, won't this entire deferred disksize update
mechanism be non-functional, leaving the filesystem vulnerable to the stale
data exposure this patchset was intended to fix?

> +
> +	i_size_write(inode, newsize);
> +	__ext4_set_i_disksize(inode, newsize);
> +	up_write(&EXT4_I(inode)->i_data_sem);
>  
>  	ret = ext4_mark_inode_dirty(handle, inode);
>  	ext4_journal_stop(handle);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=27