| Message ID | 1487930771-14491-1-git-send-email-yunlong.song@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017/2/24 18:06, Yunlong Song wrote: > No need to check the "if" condition each time, just change it to macro codes. We're going to check flag in CP, not just in code of f2fs. Thanks, > > Signed-off-by: Yunlong Song <yunlong.song@huawei.com> > --- > fs/f2fs/node.h | 20 ++++++++++---------- > fs/f2fs/segment.c | 5 +++-- > 2 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h > index 3fc9c4b..3e5a58b 100644 > --- a/fs/f2fs/node.h > +++ b/fs/f2fs/node.h > @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) > size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); > __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); > > - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { > - __u64 crc = le32_to_cpu(*((__le32 *) > - ((unsigned char *)ckpt + crc_offset))); > - cp_ver |= (crc << 32); > - } > +#ifdef CP_CRC_RECOVERY_FLAG > + __u64 crc = le32_to_cpu(*((__le32 *) > + ((unsigned char *)ckpt + crc_offset))); > + cp_ver |= (crc << 32); > +#endif > rn->footer.cp_ver = cpu_to_le64(cp_ver); > rn->footer.next_blkaddr = cpu_to_le32(blkaddr); > } > @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) > size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); > __u64 cp_ver = cur_cp_version(ckpt); > > - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { > - __u64 crc = le32_to_cpu(*((__le32 *) > - ((unsigned char *)ckpt + crc_offset))); > - cp_ver |= (crc << 32); > - } > +#ifdef CP_CRC_RECOVERY_FLAG > + __u64 crc = le32_to_cpu(*((__le32 *) > + ((unsigned char *)ckpt + crc_offset))); > + cp_ver |= (crc << 32); > +#endif > return cp_ver == cpver_of_node(page); > } > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 9eb6d89..6c2e1ee 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -1573,9 +1573,10 @@ 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) && > - type == CURSEG_WARM_NODE) > +#ifndef CP_CRC_RECOVERY_FLAG > + else if (type == CURSEG_WARM_NODE) > new_curseg(sbi, type, false); > +#endif > else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) > change_curseg(sbi, type, true); > else >
I think we do not need to care about the CP_CRC_RECOVERY_FLAG status of old image, I mean we do not need to check the already-been-written node footer in the image, what we care about is the on-going-to-write node footer, which is used for recovery. If CP_CRC_RECOVERY_FLAG is defined, then __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); is executed in each do_checkpoint actually, and CP will have that flag for each on-going-to-write node footer. I think the recovery process only needs to use the on-going-to-write node rather than the already-been-written node in the old image. The already-been-written node in the old image should not appear in the node chain of recovery process, right? On 2017/2/24 18:29, Chao Yu wrote: > On 2017/2/24 18:06, Yunlong Song wrote: >> No need to check the "if" condition each time, just change it to macro codes. > We're going to check flag in CP, not just in code of f2fs. > > Thanks, > >> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >> --- >> fs/f2fs/node.h | 20 ++++++++++---------- >> fs/f2fs/segment.c | 5 +++-- >> 2 files changed, 13 insertions(+), 12 deletions(-) >> >> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h >> index 3fc9c4b..3e5a58b 100644 >> --- a/fs/f2fs/node.h >> +++ b/fs/f2fs/node.h >> @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) >> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >> __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); >> >> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >> - __u64 crc = le32_to_cpu(*((__le32 *) >> - ((unsigned char *)ckpt + crc_offset))); >> - cp_ver |= (crc << 32); >> - } >> +#ifdef CP_CRC_RECOVERY_FLAG >> + __u64 crc = le32_to_cpu(*((__le32 *) >> + ((unsigned char *)ckpt + crc_offset))); >> + cp_ver |= (crc << 32); >> +#endif >> rn->footer.cp_ver = cpu_to_le64(cp_ver); >> rn->footer.next_blkaddr = cpu_to_le32(blkaddr); >> } >> @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) >> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >> __u64 cp_ver = cur_cp_version(ckpt); >> >> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >> - __u64 crc = le32_to_cpu(*((__le32 *) >> - ((unsigned char *)ckpt + crc_offset))); >> - cp_ver |= (crc << 32); >> - } >> +#ifdef CP_CRC_RECOVERY_FLAG >> + __u64 crc = le32_to_cpu(*((__le32 *) >> + ((unsigned char *)ckpt + crc_offset))); >> + cp_ver |= (crc << 32); >> +#endif >> return cp_ver == cpver_of_node(page); >> } >> >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >> index 9eb6d89..6c2e1ee 100644 >> --- a/fs/f2fs/segment.c >> +++ b/fs/f2fs/segment.c >> @@ -1573,9 +1573,10 @@ 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) && >> - type == CURSEG_WARM_NODE) >> +#ifndef CP_CRC_RECOVERY_FLAG >> + else if (type == CURSEG_WARM_NODE) >> new_curseg(sbi, type, false); >> +#endif >> else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) >> change_curseg(sbi, type, true); >> else >> > > . >
On 2017/2/24 19:11, Yunlong Song wrote: > I think we do not need to care about the CP_CRC_RECOVERY_FLAG status of old image, I mean we > do not need to check the already-been-written node footer in the image, what we care about is the > on-going-to-write node footer, which is used for recovery. > > If CP_CRC_RECOVERY_FLAG is defined, then __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); is > executed in each do_checkpoint actually, and CP will have that flag for each on-going-to-write node footer. > I think the recovery process only needs to use the on-going-to-write node rather than the already-been-written > node in the old image. The already-been-written node in the old image should not appear in the node > chain of recovery process, right? Previously, we changed the disk layout of footer in node block, and then we applied new verifying approach which has better reliability in order to avoid chaining garbage node block. In order to distinguish old disk layout and the new one, we introduce CP_CRC_RECOVERY_FLAG, once a CP is triggered, we will tag current CP with the flag, and use new disk layout and new verifying approach for the following node block updating flow and abnormal power-cut recovery flow. For old image which has no CP_CRC_RECOVERY_FLAG flag been set, f2fs needs to use old disk layout and old verifying approach during recovery for the compatibility. So that's why we need to check the flag in CP here. Thanks, > > On 2017/2/24 18:29, Chao Yu wrote: >> On 2017/2/24 18:06, Yunlong Song wrote: >>> No need to check the "if" condition each time, just change it to macro codes. >> We're going to check flag in CP, not just in code of f2fs. >> >> Thanks, >> >>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >>> --- >>> fs/f2fs/node.h | 20 ++++++++++---------- >>> fs/f2fs/segment.c | 5 +++-- >>> 2 files changed, 13 insertions(+), 12 deletions(-) >>> >>> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h >>> index 3fc9c4b..3e5a58b 100644 >>> --- a/fs/f2fs/node.h >>> +++ b/fs/f2fs/node.h >>> @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) >>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >>> __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); >>> >>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >>> - __u64 crc = le32_to_cpu(*((__le32 *) >>> - ((unsigned char *)ckpt + crc_offset))); >>> - cp_ver |= (crc << 32); >>> - } >>> +#ifdef CP_CRC_RECOVERY_FLAG >>> + __u64 crc = le32_to_cpu(*((__le32 *) >>> + ((unsigned char *)ckpt + crc_offset))); >>> + cp_ver |= (crc << 32); >>> +#endif >>> rn->footer.cp_ver = cpu_to_le64(cp_ver); >>> rn->footer.next_blkaddr = cpu_to_le32(blkaddr); >>> } >>> @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) >>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >>> __u64 cp_ver = cur_cp_version(ckpt); >>> >>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >>> - __u64 crc = le32_to_cpu(*((__le32 *) >>> - ((unsigned char *)ckpt + crc_offset))); >>> - cp_ver |= (crc << 32); >>> - } >>> +#ifdef CP_CRC_RECOVERY_FLAG >>> + __u64 crc = le32_to_cpu(*((__le32 *) >>> + ((unsigned char *)ckpt + crc_offset))); >>> + cp_ver |= (crc << 32); >>> +#endif >>> return cp_ver == cpver_of_node(page); >>> } >>> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index 9eb6d89..6c2e1ee 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -1573,9 +1573,10 @@ 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) && >>> - type == CURSEG_WARM_NODE) >>> +#ifndef CP_CRC_RECOVERY_FLAG >>> + else if (type == CURSEG_WARM_NODE) >>> new_curseg(sbi, type, false); >>> +#endif >>> else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) >>> change_curseg(sbi, type, true); >>> else >>> >> >> . >> > >
On 2017/2/24 19:37, Chao Yu wrote: > On 2017/2/24 19:11, Yunlong Song wrote: >> I think we do not need to care about the CP_CRC_RECOVERY_FLAG status of old image, I mean we >> do not need to check the already-been-written node footer in the image, what we care about is the >> on-going-to-write node footer, which is used for recovery. >> >> If CP_CRC_RECOVERY_FLAG is defined, then __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); is >> executed in each do_checkpoint actually, and CP will have that flag for each on-going-to-write node footer. >> I think the recovery process only needs to use the on-going-to-write node rather than the already-been-written >> node in the old image. The already-been-written node in the old image should not appear in the node >> chain of recovery process, right? > Previously, we changed the disk layout of footer in node block, and then we > applied new verifying approach which has better reliability in order to avoid > chaining garbage node block. > > In order to distinguish old disk layout and the new one, we introduce > CP_CRC_RECOVERY_FLAG, once a CP is triggered, we will tag current CP with the > flag, and use new disk layout and new verifying approach for the following node > block updating flow and abnormal power-cut recovery flow. > > For old image which has no CP_CRC_RECOVERY_FLAG flag been set, f2fs needs to use > old disk layout and old verifying approach during recovery for the > compatibility. So that's why we need to check the flag in CP here. For the old disk layout, because we still use new approach to set CP_CRC_RECOVERY_FLAG in the node footer in each do_checkpoint, then I think f2fs should also use new verifying approach during recovery rather than old verifying approach. What is the problem if we do like this? > Thanks, > >> On 2017/2/24 18:29, Chao Yu wrote: >>> On 2017/2/24 18:06, Yunlong Song wrote: >>>> No need to check the "if" condition each time, just change it to macro codes. >>> We're going to check flag in CP, not just in code of f2fs. >>> >>> Thanks, >>> >>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >>>> --- >>>> fs/f2fs/node.h | 20 ++++++++++---------- >>>> fs/f2fs/segment.c | 5 +++-- >>>> 2 files changed, 13 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h >>>> index 3fc9c4b..3e5a58b 100644 >>>> --- a/fs/f2fs/node.h >>>> +++ b/fs/f2fs/node.h >>>> @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) >>>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >>>> __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); >>>> >>>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >>>> - __u64 crc = le32_to_cpu(*((__le32 *) >>>> - ((unsigned char *)ckpt + crc_offset))); >>>> - cp_ver |= (crc << 32); >>>> - } >>>> +#ifdef CP_CRC_RECOVERY_FLAG >>>> + __u64 crc = le32_to_cpu(*((__le32 *) >>>> + ((unsigned char *)ckpt + crc_offset))); >>>> + cp_ver |= (crc << 32); >>>> +#endif >>>> rn->footer.cp_ver = cpu_to_le64(cp_ver); >>>> rn->footer.next_blkaddr = cpu_to_le32(blkaddr); >>>> } >>>> @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) >>>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >>>> __u64 cp_ver = cur_cp_version(ckpt); >>>> >>>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >>>> - __u64 crc = le32_to_cpu(*((__le32 *) >>>> - ((unsigned char *)ckpt + crc_offset))); >>>> - cp_ver |= (crc << 32); >>>> - } >>>> +#ifdef CP_CRC_RECOVERY_FLAG >>>> + __u64 crc = le32_to_cpu(*((__le32 *) >>>> + ((unsigned char *)ckpt + crc_offset))); >>>> + cp_ver |= (crc << 32); >>>> +#endif >>>> return cp_ver == cpver_of_node(page); >>>> } >>>> >>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>> index 9eb6d89..6c2e1ee 100644 >>>> --- a/fs/f2fs/segment.c >>>> +++ b/fs/f2fs/segment.c >>>> @@ -1573,9 +1573,10 @@ 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) && >>>> - type == CURSEG_WARM_NODE) >>>> +#ifndef CP_CRC_RECOVERY_FLAG >>>> + else if (type == CURSEG_WARM_NODE) >>>> new_curseg(sbi, type, false); >>>> +#endif >>>> else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) >>>> change_curseg(sbi, type, true); >>>> else >>>> >>> . >>> >> > > . >
On 02/24, Yunlong Song wrote: > On 2017/2/24 19:37, Chao Yu wrote: > > On 2017/2/24 19:11, Yunlong Song wrote: > >> I think we do not need to care about the CP_CRC_RECOVERY_FLAG status of old image, I mean we > >> do not need to check the already-been-written node footer in the image, what we care about is the > >> on-going-to-write node footer, which is used for recovery. > >> > >> If CP_CRC_RECOVERY_FLAG is defined, then __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); is > >> executed in each do_checkpoint actually, and CP will have that flag for each on-going-to-write node footer. > >> I think the recovery process only needs to use the on-going-to-write node rather than the already-been-written > >> node in the old image. The already-been-written node in the old image should not appear in the node > >> chain of recovery process, right? > > Previously, we changed the disk layout of footer in node block, and then we > > applied new verifying approach which has better reliability in order to avoid > > chaining garbage node block. > > > > In order to distinguish old disk layout and the new one, we introduce > > CP_CRC_RECOVERY_FLAG, once a CP is triggered, we will tag current CP with the > > flag, and use new disk layout and new verifying approach for the following node > > block updating flow and abnormal power-cut recovery flow. > > > > For old image which has no CP_CRC_RECOVERY_FLAG flag been set, f2fs needs to use > > old disk layout and old verifying approach during recovery for the > > compatibility. So that's why we need to check the flag in CP here. > > For the old disk layout, because we still use new approach to set CP_CRC_RECOVERY_FLAG in the node footer in each do_checkpoint, > then I think f2fs should also use new verifying approach during recovery rather than old verifying approach. What is the problem if > we do like this? This is to handle only one case in which: 1. uses old kernel without this flag, 2. calls fsync and gets sudden power-cut, 3. updates new kernel having this flag before mount. Then, if we do not check this flag at mount time, we will lose the last fsync'ed node blocks. Thanks, > > > Thanks, > > > >> On 2017/2/24 18:29, Chao Yu wrote: > >>> On 2017/2/24 18:06, Yunlong Song wrote: > >>>> No need to check the "if" condition each time, just change it to macro codes. > >>> We're going to check flag in CP, not just in code of f2fs. > >>> > >>> Thanks, > >>> > >>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> > >>>> --- > >>>> fs/f2fs/node.h | 20 ++++++++++---------- > >>>> fs/f2fs/segment.c | 5 +++-- > >>>> 2 files changed, 13 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h > >>>> index 3fc9c4b..3e5a58b 100644 > >>>> --- a/fs/f2fs/node.h > >>>> +++ b/fs/f2fs/node.h > >>>> @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) > >>>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); > >>>> __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); > >>>> > >>>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { > >>>> - __u64 crc = le32_to_cpu(*((__le32 *) > >>>> - ((unsigned char *)ckpt + crc_offset))); > >>>> - cp_ver |= (crc << 32); > >>>> - } > >>>> +#ifdef CP_CRC_RECOVERY_FLAG > >>>> + __u64 crc = le32_to_cpu(*((__le32 *) > >>>> + ((unsigned char *)ckpt + crc_offset))); > >>>> + cp_ver |= (crc << 32); > >>>> +#endif > >>>> rn->footer.cp_ver = cpu_to_le64(cp_ver); > >>>> rn->footer.next_blkaddr = cpu_to_le32(blkaddr); > >>>> } > >>>> @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) > >>>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); > >>>> __u64 cp_ver = cur_cp_version(ckpt); > >>>> > >>>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { > >>>> - __u64 crc = le32_to_cpu(*((__le32 *) > >>>> - ((unsigned char *)ckpt + crc_offset))); > >>>> - cp_ver |= (crc << 32); > >>>> - } > >>>> +#ifdef CP_CRC_RECOVERY_FLAG > >>>> + __u64 crc = le32_to_cpu(*((__le32 *) > >>>> + ((unsigned char *)ckpt + crc_offset))); > >>>> + cp_ver |= (crc << 32); > >>>> +#endif > >>>> return cp_ver == cpver_of_node(page); > >>>> } > >>>> > >>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >>>> index 9eb6d89..6c2e1ee 100644 > >>>> --- a/fs/f2fs/segment.c > >>>> +++ b/fs/f2fs/segment.c > >>>> @@ -1573,9 +1573,10 @@ 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) && > >>>> - type == CURSEG_WARM_NODE) > >>>> +#ifndef CP_CRC_RECOVERY_FLAG > >>>> + else if (type == CURSEG_WARM_NODE) > >>>> new_curseg(sbi, type, false); > >>>> +#endif > >>>> else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) > >>>> change_curseg(sbi, type, true); > >>>> else > >>>> > >>> . > >>> > >> > > > > . > > > > > -- > Thanks, > Yunlong Song >
On 2017/2/25 2:12, Jaegeuk Kim wrote: > On 02/24, Yunlong Song wrote: >> On 2017/2/24 19:37, Chao Yu wrote: >>> On 2017/2/24 19:11, Yunlong Song wrote: >>>> I think we do not need to care about the CP_CRC_RECOVERY_FLAG status of old image, I mean we >>>> do not need to check the already-been-written node footer in the image, what we care about is the >>>> on-going-to-write node footer, which is used for recovery. >>>> >>>> If CP_CRC_RECOVERY_FLAG is defined, then __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); is >>>> executed in each do_checkpoint actually, and CP will have that flag for each on-going-to-write node footer. >>>> I think the recovery process only needs to use the on-going-to-write node rather than the already-been-written >>>> node in the old image. The already-been-written node in the old image should not appear in the node >>>> chain of recovery process, right? >>> Previously, we changed the disk layout of footer in node block, and then we >>> applied new verifying approach which has better reliability in order to avoid >>> chaining garbage node block. >>> >>> In order to distinguish old disk layout and the new one, we introduce >>> CP_CRC_RECOVERY_FLAG, once a CP is triggered, we will tag current CP with the >>> flag, and use new disk layout and new verifying approach for the following node >>> block updating flow and abnormal power-cut recovery flow. >>> >>> For old image which has no CP_CRC_RECOVERY_FLAG flag been set, f2fs needs to use >>> old disk layout and old verifying approach during recovery for the >>> compatibility. So that's why we need to check the flag in CP here. >> >> For the old disk layout, because we still use new approach to set CP_CRC_RECOVERY_FLAG in the node footer in each do_checkpoint, >> then I think f2fs should also use new verifying approach during recovery rather than old verifying approach. What is the problem if >> we do like this? > > This is to handle only one case in which: > > 1. uses old kernel without this flag, > 2. calls fsync and gets sudden power-cut, > 3. updates new kernel having this flag before mount. > > Then, if we do not check this flag at mount time, we will lose the last fsync'ed > node blocks. Yup, thanks for the explanation. :) Thanks, > > Thanks, > >> >>> Thanks, >>> >>>> On 2017/2/24 18:29, Chao Yu wrote: >>>>> On 2017/2/24 18:06, Yunlong Song wrote: >>>>>> No need to check the "if" condition each time, just change it to macro codes. >>>>> We're going to check flag in CP, not just in code of f2fs. >>>>> >>>>> Thanks, >>>>> >>>>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com> >>>>>> --- >>>>>> fs/f2fs/node.h | 20 ++++++++++---------- >>>>>> fs/f2fs/segment.c | 5 +++-- >>>>>> 2 files changed, 13 insertions(+), 12 deletions(-) >>>>>> >>>>>> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h >>>>>> index 3fc9c4b..3e5a58b 100644 >>>>>> --- a/fs/f2fs/node.h >>>>>> +++ b/fs/f2fs/node.h >>>>>> @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) >>>>>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >>>>>> __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); >>>>>> >>>>>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >>>>>> - __u64 crc = le32_to_cpu(*((__le32 *) >>>>>> - ((unsigned char *)ckpt + crc_offset))); >>>>>> - cp_ver |= (crc << 32); >>>>>> - } >>>>>> +#ifdef CP_CRC_RECOVERY_FLAG >>>>>> + __u64 crc = le32_to_cpu(*((__le32 *) >>>>>> + ((unsigned char *)ckpt + crc_offset))); >>>>>> + cp_ver |= (crc << 32); >>>>>> +#endif >>>>>> rn->footer.cp_ver = cpu_to_le64(cp_ver); >>>>>> rn->footer.next_blkaddr = cpu_to_le32(blkaddr); >>>>>> } >>>>>> @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) >>>>>> size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); >>>>>> __u64 cp_ver = cur_cp_version(ckpt); >>>>>> >>>>>> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { >>>>>> - __u64 crc = le32_to_cpu(*((__le32 *) >>>>>> - ((unsigned char *)ckpt + crc_offset))); >>>>>> - cp_ver |= (crc << 32); >>>>>> - } >>>>>> +#ifdef CP_CRC_RECOVERY_FLAG >>>>>> + __u64 crc = le32_to_cpu(*((__le32 *) >>>>>> + ((unsigned char *)ckpt + crc_offset))); >>>>>> + cp_ver |= (crc << 32); >>>>>> +#endif >>>>>> return cp_ver == cpver_of_node(page); >>>>>> } >>>>>> >>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>>>>> index 9eb6d89..6c2e1ee 100644 >>>>>> --- a/fs/f2fs/segment.c >>>>>> +++ b/fs/f2fs/segment.c >>>>>> @@ -1573,9 +1573,10 @@ 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) && >>>>>> - type == CURSEG_WARM_NODE) >>>>>> +#ifndef CP_CRC_RECOVERY_FLAG >>>>>> + else if (type == CURSEG_WARM_NODE) >>>>>> new_curseg(sbi, type, false); >>>>>> +#endif >>>>>> else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) >>>>>> change_curseg(sbi, type, true); >>>>>> else >>>>>> >>>>> . >>>>> >>>> >>> >>> . >>> >> >> >> -- >> Thanks, >> Yunlong Song >> > > . >
On 2017/2/25 2:12, Jaegeuk Kim wrote: > > This is to handle only one case in which: > > 1. uses old kernel without this flag, > 2. calls fsync and gets sudden power-cut, > 3. updates new kernel having this flag before mount. > > Then, if we do not check this flag at mount time, we will lose the last fsync'ed > node blocks. > So I just send a v2 patch to only change to macro in fill_node_footer_blkaddr and allocate_segment_by_default, these two functions are called frequently, so it is better to use macro code. And I remain the original checking code in is_recoverable_dnode, this function is called only once after mount, it's OK.
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h index 3fc9c4b..3e5a58b 100644 --- a/fs/f2fs/node.h +++ b/fs/f2fs/node.h @@ -303,11 +303,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); __u64 cp_ver = le64_to_cpu(ckpt->checkpoint_ver); - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { - __u64 crc = le32_to_cpu(*((__le32 *) - ((unsigned char *)ckpt + crc_offset))); - cp_ver |= (crc << 32); - } +#ifdef CP_CRC_RECOVERY_FLAG + __u64 crc = le32_to_cpu(*((__le32 *) + ((unsigned char *)ckpt + crc_offset))); + cp_ver |= (crc << 32); +#endif rn->footer.cp_ver = cpu_to_le64(cp_ver); rn->footer.next_blkaddr = cpu_to_le32(blkaddr); } @@ -318,11 +318,11 @@ static inline bool is_recoverable_dnode(struct page *page) size_t crc_offset = le32_to_cpu(ckpt->checksum_offset); __u64 cp_ver = cur_cp_version(ckpt); - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) { - __u64 crc = le32_to_cpu(*((__le32 *) - ((unsigned char *)ckpt + crc_offset))); - cp_ver |= (crc << 32); - } +#ifdef CP_CRC_RECOVERY_FLAG + __u64 crc = le32_to_cpu(*((__le32 *) + ((unsigned char *)ckpt + crc_offset))); + cp_ver |= (crc << 32); +#endif return cp_ver == cpver_of_node(page); } diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 9eb6d89..6c2e1ee 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -1573,9 +1573,10 @@ 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) && - type == CURSEG_WARM_NODE) +#ifndef CP_CRC_RECOVERY_FLAG + else if (type == CURSEG_WARM_NODE) new_curseg(sbi, type, false); +#endif else if (need_SSR(sbi) && get_ssr_segment(sbi, type)) change_curseg(sbi, type, true); else
No need to check the "if" condition each time, just change it to macro codes. Signed-off-by: Yunlong Song <yunlong.song@huawei.com> --- fs/f2fs/node.h | 20 ++++++++++---------- fs/f2fs/segment.c | 5 +++-- 2 files changed, 13 insertions(+), 12 deletions(-)