linux-ext4 - Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept

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]

Date:   Thu, 13 Apr 2023 10:00:56 +0800
From:   Baokun Li <libaokun1@...wei.com>
To:     Jan Kara <jack@...e.cz>
CC:     <linux-ext4@...r.kernel.org>, <tytso@....edu>,
        <adilger.kernel@...ger.ca>, <ritesh.list@...il.com>,
        <linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>,
        <yangerkun@...wei.com>, <yukuai3@...wei.com>,
        Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept

On 2023/4/13 2:53, Jan Kara wrote:
> On Wed 12-04-23 20:41:20, Baokun Li wrote:
>> A helper function is added to help determine if the current extent can
>> be dropped, although only ext4_es_is_delayed() extents cannot be dropped
>> currently.
>>
>> Suggested-by: Jan Kara <jack@...e.cz>
>> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> Looks good. Just some small suggestions below...
Thank you very much for your review!
>
>> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
>> index 7bc221038c6c..f9dab2510bdc 100644
>> --- a/fs/ext4/extents_status.c
>> +++ b/fs/ext4/extents_status.c
>> @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode)
>>   	spin_unlock(&sbi->s_es_lock);
>>   }
>>   
>> +/*
>> + * Returns 1 indicates that we cannot fail to allocate memory for this
>> + * extent_status and cannot reclaim, clear, or free the extent until
>> + * its status changes.
>> + */
>> +static inline int ext4_es_must_keep(struct extent_status *es)
> Maybe we can return bool? Also I'd rephrase the comment as:

Totally agree! I tried to move it to fs/ext4/extents_status.h before, so I
changed the function type to int, but later I realized that it was not 
necessary
to move it to the header file, but the function type was not changed back.

>
> /*
>   * Returns true if we cannot fail to allocate memory for this extent_status
>   * entry and cannot reclaim it until its status changes.
>   */
OK, looks good, thanks!
>> +{
>> +	/* filemap, bigalloc, and seek_data/hole need to use it. */
>> +	if (ext4_es_is_delayed(es))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>>   static struct extent_status *
>>   ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>>   		     ext4_fsblk_t pblk)
>> @@ -460,10 +474,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>>   	es->es_len = len;
>>   	es->es_pblk = pblk;
>>   
>> -	/*
>> -	 * We don't count delayed extent because we never try to reclaim them
>> -	 */
>> -	if (!ext4_es_is_delayed(es)) {
>> +	/* We never try to reclaim a must kept extent, so we don't count it. */
>> +	if (!ext4_es_must_keep(es)) {
>>   		if (!EXT4_I(inode)->i_es_shk_nr++)
>>   			ext4_es_list_add(inode);
>>   		percpu_counter_inc(&EXT4_SB(inode->i_sb)->
>> @@ -481,8 +493,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
>>   	EXT4_I(inode)->i_es_all_nr--;
>>   	percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt);
>>   
>> -	/* Decrease the shrink counter when this es is not delayed */
>> -	if (!ext4_es_is_delayed(es)) {
>> +	/* Decrease the shrink counter when this es is not a must be kept */
> Let's rephrase the comment as:
> 	/* Decrease the shrink counter when we can reclaim the extent */

Okay, this is very nice!

>
>> +	if (!ext4_es_must_keep(es)) {
>>   		BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0);
>>   		if (!--EXT4_I(inode)->i_es_shk_nr)
>>   			ext4_es_list_del(inode);
>> @@ -853,7 +865,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>>   	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
>>   					  128, EXT4_I(inode)))
>>   		goto retry;
>> -	if (err == -ENOMEM && !ext4_es_is_delayed(&newes))
>> +	if (err == -ENOMEM && !ext4_es_must_keep(&newes))
>>   		err = 0;
>>   
>>   	if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
>> @@ -1706,11 +1718,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end,
>>   
>>   		(*nr_to_scan)--;
>>   		node = rb_next(&es->rb_node);
>> -		/*
>> -		 * We can't reclaim delayed extent from status tree because
>> -		 * fiemap, bigallic, and seek_data/hole need to use it.
>> -		 */
>> -		if (ext4_es_is_delayed(es))
>> +		/* We can't reclaim a must be kept extent from status tree. */
> I guess we can just drop this comment. The function name explains enough...

Totally agree!

>
>> +		if (ext4_es_must_keep(es))
>>   			goto next;
>>   		if (ext4_es_is_referenced(es)) {
>>   			ext4_es_clear_referenced(es);
> 								Honza

Can you please help review the remaining patches for any problems?
If you have any suggestions, I'll fix them together and post another 
version of v4.

Thanks again for your review!
-- 
With Best Regards,
Baokun Li
.