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

[Gao Xiang <gaoxiang25@xxxxxxxxxx>]

Hi Chao,

On 2018/2/1 21:27, Chao Yu wrote:
> 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?

No... I just looked at node_footer because of another work and saw this 
part of code by chance.
I think XOR-ed calculation is much better in mathematics, and typical 
method for mixing two values (eg. XOR encryption) is also XOR-ed 
calculation rather than OR-ed...

Thanks,

> Thanks,
>
> > Signed-off-by: Gao Xiang <gaoxiang25@xxxxxxxxxx>
> > ---
> >   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