Re: [PATCH -next v5 25/32] ext4: defer i_disksize update while DISKSIZE_GROW_PENDING is set

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing State Initialization / Dead Code: EXT4_STATE_DISKSIZE_GROW_PENDING is never set.
- [High] State Leak / Data Loss: Discarding the zeroed EOF folio without writeback permanently leaks the pending bit.
- [High] Durability Loss: O_SYNC extensions via fallocate/zero_range silently persist a stale i_disksize.
--

--- Patch [25]: [PATCH -next v5 25/32] ext4: defer i_disksize update while DISKSIZE_GROW_PENDING is set ---
commit 5fa612fe6ec599be935a3aa55b36845e34d3180e
Author: Zhang Yi <[email protected]>

ext4: defer i_disksize update while DISKSIZE_GROW_PENDING is set

Operations like append allocate, zero range, and truncate update
i_disksize directly. If the new i_disksize exceeds the original value
while the zeroed EOF block is still awaiting writeback, metadata may be
persisted before the zeroed data, exposing stale data on crash.

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3605,30 +3605,67 @@ do {								\

[ ... ]

> +static inline void __ext4_set_i_disksize(struct inode *inode, loff_t newsize)
> +{
> +	WARN_ON_ONCE(!rwsem_is_locked(&EXT4_I(inode)->i_data_sem));
> +
> +	if (newsize < EXT4_I(inode)->i_disksize ||
> +	    !ext4_test_inode_state(inode, EXT4_STATE_DISKSIZE_GROW_PENDING))

[Severity: High]
Could O_SYNC extensions via ext4_do_fallocate() or ext4_zero_range() silently
persist a stale i_disksize here?

If the size is extended with an unaligned old EOF, the i_disksize update is
deferred. Since O_SYNC immediately commits the journal without waiting for
the pending zeroing writeback to complete, it seems it will commit the old,
deferred i_disksize to disk.

Would this cause the file size to revert on crash despite a successful fsync,
discarding the size extension?

> +		WRITE_ONCE(EXT4_I(inode)->i_disksize, newsize);
> +}

[ ... ]

>  static inline int ext4_update_inode_size(struct inode *inode, loff_t newsize)
>  {
>  	int changed = 0;
>  
> +	if (newsize <= inode->i_size && newsize <= EXT4_I(inode)->i_disksize)
> +		return 0;
> +
> +	down_write(&EXT4_I(inode)->i_data_sem);
>  	if (newsize > inode->i_size) {
>  		i_size_write(inode, newsize);
>  		changed = 1;
>  	}
> -	if (newsize > EXT4_I(inode)->i_disksize) {
> -		ext4_update_i_disksize(inode, newsize);
> +	if (newsize > EXT4_I(inode)->i_disksize &&
> +	    !ext4_test_inode_state(inode, EXT4_STATE_DISKSIZE_GROW_PENDING)) {

[Severity: High]
Are we setting EXT4_STATE_DISKSIZE_GROW_PENDING anywhere in the codebase?

Looking at this patch series, it appears the bit is checked here but never
actually initialized or set. Does this mean the deferral condition is always
bypassed and i_disksize is always updated immediately?

[Severity: High]
If the EXT4_STATE_DISKSIZE_GROW_PENDING bit is properly set, could it leak
if the dirty folio is discarded before writeback?

For example, if a file is truncated down before writeback completes, it
seems the bit might never be cleared since the writeback completion function
wouldn't be called for that folio.

Would this leak cause all subsequent appends to skip updating i_disksize,
leading to silent data loss on crash for future writes to the file?

> +		WRITE_ONCE(EXT4_I(inode)->i_disksize, newsize);
>  		changed |= 2;
>  	}
> +	up_write(&EXT4_I(inode)->i_data_sem);
>  	return changed;
>  }

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