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@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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.
