linux-kernel - Re: [PATCH v2 03/10] ext4: drop ext4_update_disksize_before

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5de46c69-74f4-4955-a825-8c8970c0aa09@huaweicloud.com>
Date: Tue, 24 Sep 2024 15:43:22 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 ritesh.list@...il.com, yi.zhang@...wei.com, chengzhihao1@...wei.com,
 yukuai3@...wei.com
Subject: Re: [PATCH v2 03/10] ext4: drop ext4_update_disksize_before_punch()

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@...wei.com>
>>
>> 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@...wei.com>
> 
> 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
>>