linux-kernel - Re: [PATCH] f2fs: fix deadloop in foreground GC

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: <YnGrvEjxgaXDnxxi@google.com>
Date:   Tue, 3 May 2022 15:25:00 -0700
From:   Jaegeuk Kim <jaegeuk@...nel.org>
To:     Chao Yu <chao@...nel.org>
Cc:     linux-f2fs-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Ming Yan <yanming@....edu.cn>, Chao Yu <chao.yu@...o.com>
Subject: Re: [PATCH] f2fs: fix deadloop in foreground GC

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?

> 
> 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@...r.kernel.org
> Reported-by: Ming Yan <yanming@....edu.cn>
> Signed-off-by: Chao Yu <chao.yu@...o.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