linux-kernel - Re: [PATCH v3 3/5] f2fs: introduce update_sit_entry_for

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: <Z2XgAMRm6uMAT2XM@google.com>
Date: Fri, 20 Dec 2024 21:22:08 +0000
From: Jaegeuk Kim <jaegeuk@...nel.org>
To: Yi Sun <yi.sun@...soc.com>
Cc: chao@...nel.org, sunyibuaa@...il.com,
	linux-f2fs-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, niuzhiguo84@...il.com,
	Hao_hao.Wang@...soc.com, ke.wang@...soc.com
Subject: Re: [PATCH v3 3/5] f2fs: introduce update_sit_entry_for_release()

This makes the code being inconsistent. Can you refactor first and add the loop
later separately?

For example,
1) add two functions, update_sit_entry_for_alloc() and update_sit_entry_for_release()
2) add a loop in update_sit_entry_for_release()

Thanks,

On 11/04, Yi Sun wrote:
> This function can process some consecutive blocks at a time.
> 
> When using update_sit_entry() to release consecutive blocks,
> ensure that the consecutive blocks belong to the same segment.
> Because after update_sit_entry_for_realese(), @segno is still
> in use in update_sit_entry().
> 
> Signed-off-by: Yi Sun <yi.sun@...soc.com>
> ---
>  fs/f2fs/segment.c | 103 ++++++++++++++++++++++++++++++----------------
>  1 file changed, 68 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 5386ae18d808..843171ce414b 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2424,6 +2424,70 @@ static void update_segment_mtime(struct f2fs_sb_info *sbi, block_t blkaddr,
>  		SIT_I(sbi)->max_mtime = ctime;
>  }
>  
> +/*
> + * NOTE: when updating multiple blocks at the same time, please ensure
> + * that the consecutive input blocks belong to the same segment.
> + */
> +
> +static int update_sit_entry_for_release(struct f2fs_sb_info *sbi, struct seg_entry *se,
> +				block_t blkaddr, unsigned int offset, int del)
> +{
> +	bool exist;
> +#ifdef CONFIG_F2FS_CHECK_FS
> +	bool mir_exist;
> +#endif
> +	int i;
> +	int del_count = -del;
> +
> +	f2fs_bug_on(sbi, GET_SEGNO(sbi, blkaddr) != GET_SEGNO(sbi, blkaddr + del_count - 1));
> +
> +	for (i = 0; i < del_count; i++) {
> +		exist = f2fs_test_and_clear_bit(offset + i, se->cur_valid_map);
> +#ifdef CONFIG_F2FS_CHECK_FS
> +		mir_exist = f2fs_test_and_clear_bit(offset + i,
> +						se->cur_valid_map_mir);
> +		if (unlikely(exist != mir_exist)) {
> +			f2fs_err(sbi, "Inconsistent error when clearing bitmap, blk:%u, old bit:%d",
> +				 blkaddr + i, exist);
> +			f2fs_bug_on(sbi, 1);
> +		}
> +#endif
> +		if (unlikely(!exist)) {
> +			f2fs_err(sbi, "Bitmap was wrongly cleared, blk:%u",
> +				 blkaddr + i);
> +			f2fs_bug_on(sbi, 1);
> +			se->valid_blocks++;
> +			del += 1;
> +		} else if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
> +			/*
> +			 * If checkpoints are off, we must not reuse data that
> +			 * was used in the previous checkpoint. If it was used
> +			 * before, we must track that to know how much space we
> +			 * really have.
> +			 */
> +			if (f2fs_test_bit(offset + i, se->ckpt_valid_map)) {
> +				spin_lock(&sbi->stat_lock);
> +				sbi->unusable_block_count++;
> +				spin_unlock(&sbi->stat_lock);
> +			}
> +		}
> +
> +		if (f2fs_block_unit_discard(sbi) &&
> +			f2fs_test_and_clear_bit(offset + i, se->discard_map))
> +			sbi->discard_blks++;
> +
> +		if (!f2fs_test_bit(offset + i, se->ckpt_valid_map))
> +			se->ckpt_valid_blocks -= 1;
> +	}
> +
> +	return del;
> +}
> +
> +/*
> + * If releasing blocks, this function supports updating multiple consecutive blocks
> + * at one time, but please note that these consecutive blocks need to belong to the
> + * same segment.
> + */
>  static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
>  {
>  	struct seg_entry *se;
> @@ -2479,43 +2543,12 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
>  			if (!f2fs_test_and_set_bit(offset, se->ckpt_valid_map))
>  				se->ckpt_valid_blocks++;
>  		}
> -	} else {
> -		exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
> -#ifdef CONFIG_F2FS_CHECK_FS
> -		mir_exist = f2fs_test_and_clear_bit(offset,
> -						se->cur_valid_map_mir);
> -		if (unlikely(exist != mir_exist)) {
> -			f2fs_err(sbi, "Inconsistent error when clearing bitmap, blk:%u, old bit:%d",
> -				 blkaddr, exist);
> -			f2fs_bug_on(sbi, 1);
> -		}
> -#endif
> -		if (unlikely(!exist)) {
> -			f2fs_err(sbi, "Bitmap was wrongly cleared, blk:%u",
> -				 blkaddr);
> -			f2fs_bug_on(sbi, 1);
> -			se->valid_blocks++;
> -			del = 0;
> -		} else if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
> -			/*
> -			 * If checkpoints are off, we must not reuse data that
> -			 * was used in the previous checkpoint. If it was used
> -			 * before, we must track that to know how much space we
> -			 * really have.
> -			 */
> -			if (f2fs_test_bit(offset, se->ckpt_valid_map)) {
> -				spin_lock(&sbi->stat_lock);
> -				sbi->unusable_block_count++;
> -				spin_unlock(&sbi->stat_lock);
> -			}
> -		}
>  
> -		if (f2fs_block_unit_discard(sbi) &&
> -			f2fs_test_and_clear_bit(offset, se->discard_map))
> -			sbi->discard_blks++;
> +		if (!f2fs_test_bit(offset, se->ckpt_valid_map))
> +			se->ckpt_valid_blocks += del;
> +	} else {
> +		del = update_sit_entry_for_release(sbi, se, blkaddr, offset, del);
>  	}
> -	if (!f2fs_test_bit(offset, se->ckpt_valid_map))
> -		se->ckpt_valid_blocks += del;
>  
>  	__mark_sit_entry_dirty(sbi, segno);
>  
> -- 
> 2.25.1