On Wed, Dec 18, 2024 at 09:13:46PM +0800, Zhang Yi wrote:
> 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.
Okay sure, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
Regards,
ojaswin
>
> Thanks,
> Yi.
>
