| Message ID | 9047C53C18267742AB12E43B65C7F9F70BCE5C21@dggemi505-mbx.china.huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >>
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 >>>
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 >>>>
> We can use this calculation since cp_ver is complete 64-bits random number now.
Sorry, I meant we can't.
Thanks,
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, >
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
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(-)