Re: [PATCH v3 02/22] ext4: factor out ext4_truncate_[up|down]()

[Zhang Yi]

On 4/30/2026 8:55 PM, Jan Kara wrote:
> On Wed 22-04-26 10:10:22, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>>
>> Refactor ext4_setattr() by introducing two helper functions,
>> ext4_truncate_up() and ext4_truncate_down(), to handle size changes. The
>> current ATTR_SIZE processing consolidates checks for both shrinking and
>> non-shrinking cases, leading to cluttered code. Separating the
>> truncation paths improves readability.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Looks good. Just a few nits below.

Thank you for reviewing this series!

> 
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 94283a991e5c..9e4353432325 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -3501,6 +3501,23 @@ static inline int ext4_update_inode_size(struct inode *inode, loff_t newsize)
>>  	return changed;
>>  }
>>  
>> +/*
>> + * Set i_size and i_disksize to 'newsize'.
>> + *
>> + * Both i_rwsem and i_data_sem are required here to avoid races between
>> + * generic append writeback and concurrent truncate that also modify
>> + * i_size and i_disksize.
>> + */
>> +static inline void ext4_set_inode_size(struct inode *inode, loff_t newsize)
>> +{
>> +	WARN_ON_ONCE(S_ISREG(inode->i_mode) && !inode_is_locked(inode));
>> +
>> +	down_write(&EXT4_I(inode)->i_data_sem);
>> +	i_size_write(inode, newsize);
>> +	EXT4_I(inode)->i_disksize = newsize;
>> +	up_write(&EXT4_I(inode)->i_data_sem);
>> +}
>> +
> 
> Do we need this in the header later or can we keep it local to inode.c?

In the current version of the patch 021, this helper is called by
ext4_collapse_range() and ext4_insert_range(). However, after analyzing
sashiko's review comments, I believe this is unnecessary, so I will move
it to inode.c in my next iteration.

> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 0751dc55e94f..5e913aca6499 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -5855,6 +5855,83 @@ static void ext4_wait_for_tail_page_commit(struct inode *inode)
>>  	}
>>  }
>>  
>> +static int ext4_truncate_up(struct inode *inode, loff_t oldsize, loff_t newsize)
>> +{
>> +	ext4_lblk_t old_lblk, new_lblk;
>> +	handle_t *handle;
>> +	int ret;
>> +
>> +	if (!IS_ALIGNED(oldsize | newsize, i_blocksize(inode))) {
>> +		ret = ext4_inode_attach_jinode(inode);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
>> +	if (oldsize & (i_blocksize(inode) - 1)) {
> 
> When you transitioned to IS_ALIGNED above, use it here as well?

Ha, right, I missed it.

> 
>> +		ret = ext4_block_zero_eof(inode, oldsize, LLONG_MAX);
>> +		if (ret)
>> +			return ret;
>> +	}
> 
> ...
> 
>> +		if (attr->ia_size > oldsize)
>> +			error = ext4_truncate_up(inode, oldsize, attr->ia_size);
>> +		else if (shrink)
>> +			error = ext4_truncate_down(inode, oldsize,
>> +						   attr->ia_size, &orphan);
>> +		if (error)
>> +			goto out_mmap_sem;
>>  
>>  		/*
>>  		 * Truncate pagecache after we've waited for commit
> 
> Hum, why not move the truncate_pagecache() call and ext4_truncate() call
> into ext4_truncate_down()? They are not needed in the truncate up case...

Yeah, agree. These two functions also need to be used in cases where
the file size remains unchanged. We can move them into ext4_truncate_down()
and extend it to handle the file size unchanged scenario.

Thanks,
Yi.

> 
> 								Honza
>