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

[Ojaswin Mujoo]

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