* [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
@ 2018-01-31 16:16 Gaoxiang (OS)
2018-02-01 13:27 ` Chao Yu
0 siblings, 1 reply; 7+ messages in thread
From: Gaoxiang (OS) @ 2018-01-31 16:16 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu, Yuchao (T)
Cc: linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, heyunlei, Gao Xiang
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.
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
--
2.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
2018-01-31 16:16 [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery Gaoxiang (OS)
@ 2018-02-01 13:27 ` Chao Yu
2018-02-01 13:57 ` Gao Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Chao Yu @ 2018-02-01 13:27 UTC (permalink / raw)
To: Gaoxiang (OS), Jaegeuk Kim, Yuchao (T)
Cc: linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, heyunlei, Gao Xiang
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
2018-02-01 13:27 ` Chao Yu
@ 2018-02-01 13:57 ` Gao Xiang
2018-02-01 14:06 ` [f2fs-dev] " Gao Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2018-02-01 13:57 UTC (permalink / raw)
To: Chao Yu, Jaegeuk Kim, Yuchao (T)
Cc: linux-fsdevel@vger.kernel.org, Gao Xiang, heyunlei,
linux-f2fs-devel@lists.sourceforge.net
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@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
2018-02-01 13:57 ` Gao Xiang
@ 2018-02-01 14:06 ` Gao Xiang
2018-02-01 14:29 ` Chao Yu
0 siblings, 1 reply; 7+ messages in thread
From: Gao Xiang @ 2018-02-01 14:06 UTC (permalink / raw)
To: Chao Yu, Jaegeuk Kim, Yuchao (T)
Cc: linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, heyunlei, Gao Xiang
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...
>
> 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
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
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
2018-02-01 14:06 ` [f2fs-dev] " Gao Xiang
@ 2018-02-01 14:29 ` Chao Yu
2018-02-01 14:30 ` Chao Yu
0 siblings, 1 reply; 7+ messages in thread
From: Chao Yu @ 2018-02-01 14:29 UTC (permalink / raw)
To: Gao Xiang, Jaegeuk Kim, Yuchao (T)
Cc: linux-fsdevel@vger.kernel.org, Gao Xiang, heyunlei,
linux-f2fs-devel@lists.sourceforge.net
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
2018-02-01 14:29 ` Chao Yu
@ 2018-02-01 14:30 ` Chao Yu
2018-02-01 14:38 ` Gao Xiang
0 siblings, 1 reply; 7+ messages in thread
From: Chao Yu @ 2018-02-01 14:30 UTC (permalink / raw)
To: Gao Xiang, Jaegeuk Kim, Yuchao (T)
Cc: linux-fsdevel@vger.kernel.org, Gao Xiang, heyunlei,
linux-f2fs-devel@lists.sourceforge.net
> We can use this calculation since cp_ver is complete 64-bits random number now.
Sorry, I meant we can't.
Thanks,
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
2018-02-01 14:30 ` Chao Yu
@ 2018-02-01 14:38 ` Gao Xiang
0 siblings, 0 replies; 7+ messages in thread
From: Gao Xiang @ 2018-02-01 14:38 UTC (permalink / raw)
To: Chao Yu, Jaegeuk Kim, Yuchao (T)
Cc: linux-fsdevel@vger.kernel.org, Gao Xiang, heyunlei,
linux-f2fs-devel@lists.sourceforge.net
Hi Chao,
On 2018/2/1 22:30, Chao Yu wrote:
>> We can use this calculation since cp_ver is complete 64-bits random number now.
>
> Sorry, I meant we can't.
>
Alright, I think it is an unimportant proposal for now.
We could fix now, sometime later or never :)
Just saw by chance and a little suggestion to the community...
Thanks,
> Thanks,
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-02-01 14:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-31 16:16 [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery Gaoxiang (OS)
2018-02-01 13:27 ` Chao Yu
2018-02-01 13:57 ` Gao Xiang
2018-02-01 14:06 ` [f2fs-dev] " Gao Xiang
2018-02-01 14:29 ` Chao Yu
2018-02-01 14:30 ` Chao Yu
2018-02-01 14:38 ` Gao Xiang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).