On Wed 04-09-24 14:29:20, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>
> Current ext4_punch_hole() is full of complex position calculation and
> stale error out tags. In order to clean up the code and make things
> clear, refactor it by a) simplify and rename variables, make the style
> the same as ext4_zero_range(), b) remove some unnecessary position
> calculations, always write back dirty data and drop cache from offset to
> end, instead of only write back aligned blocks, c) rename the three
> stale error tags.
>
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/ext4/inode.c | 114 ++++++++++++++++++++++--------------------------
> 1 file changed, 51 insertions(+), 63 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 9343ce9f2b01..dfaf9e9d6ad8 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3916,13 +3916,14 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
> {
> struct inode *inode = file_inode(file);
> struct super_block *sb = inode->i_sb;
> - ext4_lblk_t first_block, stop_block;
> + ext4_lblk_t start_lblk, end_lblk;
> struct address_space *mapping = inode->i_mapping;
> - loff_t first_block_offset, last_block_offset, max_length;
> - struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> + loff_t max_end = EXT4_SB(sb)->s_bitmap_maxbytes - sb->s_blocksize;
> + loff_t end = offset + length;
> + unsigned long blocksize = i_blocksize(inode);
> handle_t *handle;
> unsigned int credits;
> - int ret = 0, ret2 = 0;
> + int ret = 0;
>
> trace_ext4_punch_hole(inode, offset, length, 0);
>
> @@ -3930,36 +3931,27 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>
> /* No need to punch hole beyond i_size */
> if (offset >= inode->i_size)
> - goto out_mutex;
> + goto out;
>
> /*
> - * If the hole extends beyond i_size, set the hole
> - * to end after the page that contains i_size
> + * If the hole extends beyond i_size, set the hole to end after
> + * the page that contains i_size, and also make sure that the hole
> + * within one block before last range.
> */
> - if (offset + length > inode->i_size) {
> - length = inode->i_size +
> - PAGE_SIZE - (inode->i_size & (PAGE_SIZE - 1)) -
> - offset;
> - }
> + if (end > inode->i_size)
> + end = round_up(inode->i_size, PAGE_SIZE);
> + if (end > max_end)
> + end = max_end;
> + length = end - offset;
>
> /*
> - * For punch hole the length + offset needs to be within one block
> - * before last range. Adjust the length if it goes beyond that limit.
> + * Attach jinode to inode for jbd2 if we do any zeroing of partial
> + * block.
> */
> - max_length = sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize;
> - if (offset + length > max_length)
> - length = max_length - offset;
> -
> - if (offset & (sb->s_blocksize - 1) ||
> - (offset + length) & (sb->s_blocksize - 1)) {
> - /*
> - * Attach jinode to inode for jbd2 if we do any zeroing of
> - * partial block
> - */
> + if (offset & (blocksize - 1) || end & (blocksize - 1)) {
> ret = ext4_inode_attach_jinode(inode);
> if (ret < 0)
> - goto out_mutex;
> -
> + goto out;
> }
>
> /* Wait all existing dio workers, newcomers will block on i_rwsem */
> @@ -3967,7 +3959,7 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>
> ret = file_modified(file);
> if (ret)
> - goto out_mutex;
> + goto out;
>
> /*
> * Prevent page faults from reinstantiating pages we have released from
> @@ -3977,23 +3969,17 @@ 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;
>
> /* Write out all dirty pages to avoid race conditions */
> if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
> - ret = filemap_write_and_wait_range(mapping, offset,
> - offset + length - 1);
> + ret = filemap_write_and_wait_range(mapping, offset, end - 1);
> 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;
> -
> /* Now release the pages and zero block aligned part of pages*/
> - if (last_block_offset > first_block_offset)
> - truncate_pagecache_range(inode, first_block_offset,
> - last_block_offset);
> + truncate_pagecache_range(inode, offset, end - 1);
>
> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> credits = ext4_writepage_trans_blocks(inode);
> @@ -4003,52 +3989,54 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
> if (IS_ERR(handle)) {
> ret = PTR_ERR(handle);
> ext4_std_error(sb, ret);
> - goto out_dio;
> + goto out_invalidate_lock;
> }
>
> - ret = ext4_zero_partial_blocks(handle, inode, offset,
> - length);
> + ret = ext4_zero_partial_blocks(handle, inode, offset, length);
> if (ret)
> - goto out_stop;
> -
> - first_block = (offset + sb->s_blocksize - 1) >>
> - EXT4_BLOCK_SIZE_BITS(sb);
> - stop_block = (offset + length) >> EXT4_BLOCK_SIZE_BITS(sb);
> + goto out_handle;
>
> /* If there are blocks to remove, do it */
> - if (stop_block > first_block) {
> - ext4_lblk_t hole_len = stop_block - first_block;
> + start_lblk = round_up(offset, blocksize) >> inode->i_blkbits;
> + end_lblk = end >> inode->i_blkbits;
> +
> + if (end_lblk > start_lblk) {
> + ext4_lblk_t hole_len = end_lblk - start_lblk;
>
> down_write(&EXT4_I(inode)->i_data_sem);
> ext4_discard_preallocations(inode);
>
> - ext4_es_remove_extent(inode, first_block, hole_len);
> + ext4_es_remove_extent(inode, start_lblk, hole_len);
>
> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> - ret = ext4_ext_remove_space(inode, first_block,
> - stop_block - 1);
> + ret = ext4_ext_remove_space(inode, start_lblk,
> + end_lblk - 1);
> else
> - ret = ext4_ind_remove_space(handle, inode, first_block,
> - stop_block);
> + ret = ext4_ind_remove_space(handle, inode, start_lblk,
> + end_lblk);
> + if (ret) {
> + up_write(&EXT4_I(inode)->i_data_sem);
> + goto out_handle;
> + }
>
> - ext4_es_insert_extent(inode, first_block, hole_len, ~0,
> + ext4_es_insert_extent(inode, start_lblk, hole_len, ~0,
> EXTENT_STATUS_HOLE);
> up_write(&EXT4_I(inode)->i_data_sem);
> }
> - ext4_fc_track_range(handle, inode, first_block, stop_block);
> + ext4_fc_track_range(handle, inode, start_lblk, end_lblk);
> +
> + ret = ext4_mark_inode_dirty(handle, inode);
> + if (unlikely(ret))
> + goto out_handle;
> +
> + ext4_update_inode_fsync_trans(handle, inode, 1);
> if (IS_SYNC(inode))
> ext4_handle_sync(handle);
> -
> - ret2 = ext4_mark_inode_dirty(handle, inode);
> - if (unlikely(ret2))
> - ret = ret2;
> - if (ret >= 0)
> - ext4_update_inode_fsync_trans(handle, inode, 1);
> -out_stop:
> +out_handle:
> ext4_journal_stop(handle);
> -out_dio:
> +out_invalidate_lock:
> filemap_invalidate_unlock(mapping);
> -out_mutex:
> +out:
> inode_unlock(inode);
> return ret;
> }
> --
> 2.39.2
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
