Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6c0e4596-1698-25d5-7b59-c0ee6ef7eefd@kernel.org>
Date:   Wed, 4 May 2022 08:39:12 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH] f2fs: fix deadloop in foreground GC
Content-Language: en-US
To:     Jaegeuk Kim <jaegeuk@kernel.org>
Cc:     linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Ming Yan <yanming@tju.edu.cn>, Chao Yu <chao.yu@oppo.com>
References: <20220429204631.7241-1-chao@kernel.org>
 <YnGrvEjxgaXDnxxi@google.com>
From:   Chao Yu <chao@kernel.org>
In-Reply-To: <YnGrvEjxgaXDnxxi@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/5/4 6:25, Jaegeuk Kim wrote:
> On 04/30, Chao Yu wrote:
>> As Yanming reported in bugzilla:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=215914
>>
>> The root cause is: in a very small sized image, it's very easy to
>> exceed threshold of foreground GC, if we calculate free space and
>> dirty data based on section granularity, in corner case,
>> has_not_enough_free_secs() will always return true, result in
>> deadloop in f2fs_gc().
> 
> Performance regression was reported. Can we check this for very small sized
> image only?

I noticed that, I've fixed the issue in v2, could you please take a look?

Thanks,

> 
>>
>> So this patch refactors has_not_enough_free_secs() as below to fix
>> this issue:
>> 1. calculate needed space based on block granularity, and separate
>> all blocks to two parts, section part, and block part, comparing
>> section part to free section, and comparing block part to free space
>> in openned log.
>> 2. account F2FS_DIRTY_NODES, F2FS_DIRTY_IMETA and F2FS_DIRTY_DENTS
>> as node block consumer;
>> 3. account F2FS_DIRTY_DENTS as data block consumer;
>>
>> Cc: stable@vger.kernel.org
>> Reported-by: Ming Yan <yanming@tju.edu.cn>
>> Signed-off-by: Chao Yu <chao.yu@oppo.com>
>> ---
>>   fs/f2fs/segment.h | 30 +++++++++++++++++-------------
>>   1 file changed, 17 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 8a591455d796..28f7aa9b40bf 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -575,11 +575,10 @@ static inline int reserved_sections(struct f2fs_sb_info *sbi)
>>   	return GET_SEC_FROM_SEG(sbi, reserved_segments(sbi));
>>   }
>>   
>> -static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi)
>> +static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
>> +			unsigned int node_blocks, unsigned int dent_blocks)
>>   {
>> -	unsigned int node_blocks = get_pages(sbi, F2FS_DIRTY_NODES) +
>> -					get_pages(sbi, F2FS_DIRTY_DENTS);
>> -	unsigned int dent_blocks = get_pages(sbi, F2FS_DIRTY_DENTS);
>> +
>>   	unsigned int segno, left_blocks;
>>   	int i;
>>   
>> @@ -605,19 +604,24 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi)
>>   static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi,
>>   					int freed, int needed)
>>   {
>> -	int node_secs = get_blocktype_secs(sbi, F2FS_DIRTY_NODES);
>> -	int dent_secs = get_blocktype_secs(sbi, F2FS_DIRTY_DENTS);
>> -	int imeta_secs = get_blocktype_secs(sbi, F2FS_DIRTY_IMETA);
>> +	unsigned int total_node_blocks = get_pages(sbi, F2FS_DIRTY_NODES) +
>> +					get_pages(sbi, F2FS_DIRTY_DENTS) +
>> +					get_pages(sbi, F2FS_DIRTY_IMETA);
>> +	unsigned int total_dent_blocks = get_pages(sbi, F2FS_DIRTY_DENTS);
>> +	unsigned int node_secs = total_node_blocks / BLKS_PER_SEC(sbi);
>> +	unsigned int dent_secs = total_dent_blocks / BLKS_PER_SEC(sbi);
>> +	unsigned int node_blocks = total_node_blocks % BLKS_PER_SEC(sbi);
>> +	unsigned int dent_blocks = total_dent_blocks % BLKS_PER_SEC(sbi);
>>   
>>   	if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>>   		return false;
>>   
>> -	if (free_sections(sbi) + freed == reserved_sections(sbi) + needed &&
>> -			has_curseg_enough_space(sbi))
>> -		return false;
>> -	return (free_sections(sbi) + freed) <=
>> -		(node_secs + 2 * dent_secs + imeta_secs +
>> -		reserved_sections(sbi) + needed);
>> +	if (free_sections(sbi) + freed <=
>> +			node_secs + dent_secs + reserved_sections(sbi) + needed)
>> +		return true;
>> +	if (!has_curseg_enough_space(sbi, node_blocks, dent_blocks))
>> +		return true;
>> +	return false;
>>   }
>>   
>>   static inline bool f2fs_is_checkpoint_ready(struct f2fs_sb_info *sbi)
>> -- 
>> 2.32.0