linux-kernel - Re: [PATCH v4 04/10] ext4: refactor ext4_punch

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] [day] [month] [year] [list]

Message-ID: <221b151d-12c7-4e98-afc4-d248aa3637ba@huaweicloud.com>
Date: Wed, 18 Dec 2024 21:13:46 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 jack@...e.cz, yi.zhang@...wei.com, chengzhihao1@...wei.com,
 yukuai3@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH v4 04/10] ext4: refactor ext4_punch_hole()

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

Thanks,
Yi.