Re: [PATCH v4 04/10] ext4: refactor ext4_punch_hole()

[Zhang Yi <yi.zhang@huaweicloud.com>]

On 2024/12/18 18:17, Ojaswin Mujoo wrote:
> On Mon, Dec 16, 2024 at 09:39:09AM +0800, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> The current implementation of ext4_punch_hole() contains complex
>> position calculations and stale error tags. To improve the code's
>> clarity and maintainability, it is essential to clean up the code and
>> improve its readability, this can be achieved by: a) simplifying and
>> renaming variables; b) eliminating unnecessary position calculations;
>> c) writing back all data in data=journal mode, and drop page cache from
>> the original offset to the end, rather than using aligned blocks,
>> d) renaming the stale error tags.
>>
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>  fs/ext4/ext4.h  |   2 +
>>  fs/ext4/inode.c | 119 +++++++++++++++++++++---------------------------
>>  2 files changed, 55 insertions(+), 66 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 8843929b46ce..8be06d5f5b43 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -367,6 +367,8 @@ struct ext4_io_submit {
>>  #define EXT4_MAX_BLOCKS(size, offset, blkbits) \
>>  	((EXT4_BLOCK_ALIGN(size + offset, blkbits) >> blkbits) - (offset >> \
>>  								  blkbits))
>> +#define EXT4_B_TO_LBLK(inode, offset) \
>> +	(round_up((offset), i_blocksize(inode)) >> (inode)->i_blkbits)
>>  
>>  /* Translate a block number to a cluster number */
>>  #define EXT4_B2C(sbi, blk)	((blk) >> (sbi)->s_cluster_bits)
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index a5ba2b71d508..7720d3700b27 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
[..]
>> @@ -4069,22 +4060,16 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>>  
>>  	ret = ext4_break_layouts(inode);
>>  	if (ret)
>> -		goto out_dio;
>> +		goto out_invalidate_lock;
>>  
>> -	first_block_offset = round_up(offset, sb->s_blocksize);
>> -	last_block_offset = round_down((offset + length), sb->s_blocksize) - 1;
>> +	ret = ext4_update_disksize_before_punch(inode, offset, length);
> 
> Hey Zhang,
> 
> The changes look good to me, just one question, why are we doing
> disksize update unconditionally now and not only when the range 
> spans a complete block or more.
> 

I want to simplify the code. We only need to update the disksize when
the end of the punching or zeroing range is >= the EOF and i_disksize
is less than i_size. ext4_update_disksize_before_punch() has already
performed this check and has ruled out most cases. Therefore, I
believe that calling it unconditionally will not incur significant
costs.

Thanks,
Yi.