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

[Chao Yu <chao@kernel.org>]

On 2018/2/1 22:06, Gao Xiang wrote:
> 
> 
> On 2018/2/1 21:57, Gao Xiang wrote:
>> 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...

Yes, I think so.

>>
>> Thanks,
>>
> 
> BTW,
> "The crc is already random enough, but has 32bits only.
> The cp_ver is not easy to use over 32bits, so we don't need to keep the other
> 32bits untouched in most of life."
> 
> I observe cp_ver(if the high 32 bits are 0) ^ crc == cp_ver | crc, but

We can use this calculation since cp_ver is complete 64-bits random number now.

Thanks,

> if cp_ver is over 32bits, ...
> 
> ...hmmm...
> 
> Thanks,
> 
>>>
>>> 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
>>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel