Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery

[Chao Yu <chao@kernel.org>]

Hi Xiang,

On 2018/2/1 0:16, Gaoxiang (OS) wrote:
> This patch add a flag CP_CRC_RECOVERY_XOR_FLAG to use XORed crc ^ cp_ver
> since crc | cp_ver is more likely to get a collision or become 11..1111 | cp_ver.

FYI, we have discussed about this before:

https://patchwork.kernel.org/patch/9342639/

At that time, cp_ver will always be initialized with 1, so there is almost a
little chance to use high 32 bits, result in less collision, so I think it
will be OK.

But now, cp_ver will be initialized with a random 64 bits value, then the
collision will be increased, I agree that xor method will be better, but I'm
not sure we should use this, since layout change makes complicated handling
in codes for back compatibility.

And do you encounter any incorrect recovery, or is there any following feature
rely on this?

Thanks,

> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>  fs/f2fs/checkpoint.c    |  4 ++--
>  fs/f2fs/node.h          | 16 +++++++++++-----
>  fs/f2fs/segment.c       |  3 ++-
>  include/linux/f2fs_fs.h |  1 +
>  4 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 8b0945b..9e7e63b 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1157,8 +1157,8 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	if (is_sbi_flag_set(sbi, SBI_NEED_FSCK))
>  		__set_ckpt_flags(ckpt, CP_FSCK_FLAG);
>  
> -	/* set this flag to activate crc|cp_ver for recovery */
> -	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
> +	/* set this flag to activate crc^cp_ver for recovery */
> +	__set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG);
>  	__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
>  
>  	spin_unlock_irqrestore(&sbi->cp_lock, flags);
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index 081ef0d..7b9489f 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -293,8 +293,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr)
>  	struct f2fs_node *rn = F2FS_NODE(page);
>  	__u64 cp_ver = cur_cp_version(ckpt);
>  
> -	if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
> -		cp_ver |= (cur_cp_crc(ckpt) << 32);
> +	if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
> +		cp_ver ^= cur_cp_crc(ckpt) << 32;
> +	/* for backward compatibility */
> +	else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
> +		cp_ver |= cur_cp_crc(ckpt) << 32;
>  
>  	rn->footer.cp_ver = cpu_to_le64(cp_ver);
>  	rn->footer.next_blkaddr = cpu_to_le32(blkaddr);
> @@ -307,10 +310,13 @@ static inline bool is_recoverable_dnode(struct page *page)
>  
>  	/* Don't care crc part, if fsck.f2fs sets it. */
>  	if (__is_set_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG))
> -		return (cp_ver << 32) == (cpver_of_node(page) << 32);
> +		return (__u32)cp_ver == (__u32)cpver_of_node(page);
>  
> -	if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
> -		cp_ver |= (cur_cp_crc(ckpt) << 32);
> +	if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
> +		cp_ver ^= cur_cp_crc(ckpt) << 32;
> +	/* for backward compatibility */
> +	else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
> +		cp_ver |= cur_cp_crc(ckpt) << 32;
>  
>  	return cp_ver == cpver_of_node(page);
>  }
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 205b0d9..64d0c1f 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2316,7 +2316,8 @@ static void allocate_segment_by_default(struct f2fs_sb_info *sbi,
>  
>  	if (force)
>  		new_curseg(sbi, type, true);
> -	else if (!is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) &&
> +	else if (!(is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) ||
> +		is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_XOR_FLAG)) &&
>  					type == CURSEG_WARM_NODE)
>  		new_curseg(sbi, type, false);
>  	else if (curseg->alloc_type == LFS && is_next_segment_free(sbi, type))
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index f7f0990..07ddf4b 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -117,6 +117,7 @@ struct f2fs_super_block {
>  /*
>   * For checkpoint
>   */
> +#define CP_CRC_RECOVERY_XOR_FLAG	0x00000800
>  #define CP_LARGE_NAT_BITMAP_FLAG	0x00000400
>  #define CP_NOCRC_RECOVERY_FLAG	0x00000200
>  #define CP_TRIMMED_FLAG		0x00000100
>