On 2024/9/21 0:13, Jan Kara wrote:
> On Wed 04-09-24 14:29:18, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
>>
>> Since we always write back dirty data before zeroing range and punching
>> hole, the delalloc extended file's disksize of should be updated
>> properly when writing back pages, hence we don't need to update file's
>> disksize before discarding page cache in ext4_zero_range() and
>> ext4_punch_hole(), just drop ext4_update_disksize_before_punch().
>>
>> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
>
> So when we don't write out before hole punching & company this needs to stay
> in some shape or form.
>
Thanks for taking time to review this series!
I don't fully understand this comment, please let me confirm. Do you
suggested that we still don't write out all the data before punching /
zeroing / collapseing(i.e. drop patch 01), so we need to keep
ext4_update_disksize_before_punch()(i.e. also drop this patch), is
that right?
Thanks,
Yi.
>
>> ---
>> fs/ext4/ext4.h | 3 ---
>> fs/ext4/extents.c | 4 ----
>> fs/ext4/inode.c | 37 +------------------------------------
>> 3 files changed, 1 insertion(+), 43 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 08acd152261e..e8d7965f62c4 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -3414,9 +3414,6 @@ static inline int ext4_update_inode_size(struct inode *inode, loff_t newsize)
>> return changed;
>> }
>>
>> -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
>> - loff_t len);
>> -
>> struct ext4_group_info {
>> unsigned long bb_state;
>> #ifdef AGGRESSIVE_CHECK
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 19a9b14935b7..d9fccf2970e9 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -4637,10 +4637,6 @@ static long ext4_zero_range(struct file *file, loff_t offset,
>> flags |= (EXT4_GET_BLOCKS_CONVERT_UNWRITTEN |
>> EXT4_EX_NOCACHE);
>>
>> - ret = ext4_update_disksize_before_punch(inode, offset, len);
>> - if (ret)
>> - goto out_invalidate_lock;
>> -
>> /* Now release the pages and zero block aligned part of pages */
>> truncate_pagecache_range(inode, start, end - 1);
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 8af25442d44d..9343ce9f2b01 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3872,37 +3872,6 @@ int ext4_can_truncate(struct inode *inode)
>> return 0;
>> }
>>
>> -/*
>> - * We have to make sure i_disksize gets properly updated before we truncate
>> - * page cache due to hole punching or zero range. Otherwise i_disksize update
>> - * can get lost as it may have been postponed to submission of writeback but
>> - * that will never happen after we truncate page cache.
>> - */
>> -int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
>> - loff_t len)
>> -{
>> - handle_t *handle;
>> - int ret;
>> -
>> - loff_t size = i_size_read(inode);
>> -
>> - WARN_ON(!inode_is_locked(inode));
>> - if (offset > size || offset + len < size)
>> - return 0;
>> -
>> - if (EXT4_I(inode)->i_disksize >= size)
>> - return 0;
>> -
>> - handle = ext4_journal_start(inode, EXT4_HT_MISC, 1);
>> - if (IS_ERR(handle))
>> - return PTR_ERR(handle);
>> - ext4_update_i_disksize(inode, size);
>> - ret = ext4_mark_inode_dirty(handle, inode);
>> - ext4_journal_stop(handle);
>> -
>> - return ret;
>> -}
>> -
>> static void ext4_wait_dax_page(struct inode *inode)
>> {
>> filemap_invalidate_unlock(inode->i_mapping);
>> @@ -4022,13 +3991,9 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>> 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) {
>> - ret = ext4_update_disksize_before_punch(inode, offset, length);
>> - if (ret)
>> - goto out_dio;
>> + if (last_block_offset > first_block_offset)
>> truncate_pagecache_range(inode, first_block_offset,
>> last_block_offset);
>> - }
>>
>> if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
>> credits = ext4_writepage_trans_blocks(inode);
>> --
>> 2.39.2
>>
