* [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro @ 2017-02-24 10:06 Yunlong Song 2017-02-24 10:29 ` Chao Yu 2017-02-25 8:01 ` [PATCH v2] " Yunlong Song 0 siblings, 2 replies; 10+ messages in thread From: Yunlong Song @ 2017-02-24 10:06 UTC (permalink / raw) To: jaegeuk, cm224.lee, yuchao0, chao, sylinux, yunlong.song, miaoxie Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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(-) 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 -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 10:06 [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro Yunlong Song @ 2017-02-24 10:29 ` Chao Yu 2017-02-24 11:11 ` Yunlong Song 2017-02-25 8:01 ` [PATCH v2] " Yunlong Song 1 sibling, 1 reply; 10+ messages in thread From: Chao Yu @ 2017-02-24 10:29 UTC (permalink / raw) To: Yunlong Song, jaegeuk, cm224.lee, chao, sylinux, miaoxie Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 10:29 ` Chao Yu @ 2017-02-24 11:11 ` Yunlong Song 2017-02-24 11:37 ` Chao Yu 0 siblings, 1 reply; 10+ messages in thread From: Yunlong Song @ 2017-02-24 11:11 UTC (permalink / raw) To: Chao Yu, jaegeuk, cm224.lee, chao, sylinux, miaoxie Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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 >> > > . > -- Thanks, Yunlong Song ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 11:11 ` Yunlong Song @ 2017-02-24 11:37 ` Chao Yu 2017-02-24 11:57 ` Yunlong Song 0 siblings, 1 reply; 10+ messages in thread From: Chao Yu @ 2017-02-24 11:37 UTC (permalink / raw) To: Yunlong Song, jaegeuk, cm224.lee, chao, sylinux, miaoxie Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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 >>> >> >> . >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 11:37 ` Chao Yu @ 2017-02-24 11:57 ` Yunlong Song 2017-02-24 18:12 ` Jaegeuk Kim 0 siblings, 1 reply; 10+ messages in thread From: Yunlong Song @ 2017-02-24 11:57 UTC (permalink / raw) To: Chao Yu, jaegeuk, cm224.lee, chao, sylinux, miaoxie Cc: bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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 >>>> >>> . >>> >> > > . > -- Thanks, Yunlong Song ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 11:57 ` Yunlong Song @ 2017-02-24 18:12 ` Jaegeuk Kim 2017-02-25 0:54 ` Chao Yu 2017-02-25 8:10 ` Yunlong Song 0 siblings, 2 replies; 10+ messages in thread From: Jaegeuk Kim @ 2017-02-24 18:12 UTC (permalink / raw) To: Yunlong Song Cc: Chao Yu, cm224.lee, chao, sylinux, miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 18:12 ` Jaegeuk Kim @ 2017-02-25 0:54 ` Chao Yu 2017-02-25 8:10 ` Yunlong Song 1 sibling, 0 replies; 10+ messages in thread From: Chao Yu @ 2017-02-25 0:54 UTC (permalink / raw) To: Jaegeuk Kim, Yunlong Song Cc: cm224.lee, chao, sylinux, miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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 >> > > . > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 18:12 ` Jaegeuk Kim 2017-02-25 0:54 ` Chao Yu @ 2017-02-25 8:10 ` Yunlong Song 1 sibling, 0 replies; 10+ messages in thread From: Yunlong Song @ 2017-02-25 8:10 UTC (permalink / raw) To: Jaegeuk Kim Cc: Chao Yu, cm224.lee, chao, sylinux, miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel 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. -- Thanks, Yunlong Song ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-24 10:06 [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro Yunlong Song 2017-02-24 10:29 ` Chao Yu @ 2017-02-25 8:01 ` Yunlong Song 2017-02-25 18:46 ` Jaegeuk Kim 1 sibling, 1 reply; 10+ messages in thread From: Yunlong Song @ 2017-02-25 8:01 UTC (permalink / raw) To: jaegeuk, cm224.lee, yuchao0, chao, sylinux, yunlong.song, miaoxie Cc: bintian.wang, linux-f2fs-devel, linux-fsdevel, linux-kernel 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 | 10 +++++----- fs/f2fs/segment.c | 5 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h index 3fc9c4b..caebb23 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); } 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 -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro 2017-02-25 8:01 ` [PATCH v2] " Yunlong Song @ 2017-02-25 18:46 ` Jaegeuk Kim 0 siblings, 0 replies; 10+ messages in thread From: Jaegeuk Kim @ 2017-02-25 18:46 UTC (permalink / raw) To: Yunlong Song Cc: cm224.lee, yuchao0, chao, sylinux, miaoxie, bintian.wang, linux-f2fs-devel, linux-fsdevel, linux-kernel On 02/25, Yunlong Song wrote: > 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 | 10 +++++----- > fs/f2fs/segment.c | 5 +++-- > 2 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h > index 3fc9c4b..caebb23 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 NAK. This is not a kernel config, nor a recommendable coding style. Moreover, it is simply able to be defined through header file in old kernel. Thanks, > + __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); > } > 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 > -- > 1.8.5.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-25 18:47 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-02-24 10:06 [PATCH] f2fs: change the codes of checking CP_CRC_RECOVERY_FLAG to macro Yunlong Song 2017-02-24 10:29 ` Chao Yu 2017-02-24 11:11 ` Yunlong Song 2017-02-24 11:37 ` Chao Yu 2017-02-24 11:57 ` Yunlong Song 2017-02-24 18:12 ` Jaegeuk Kim 2017-02-25 0:54 ` Chao Yu 2017-02-25 8:10 ` Yunlong Song 2017-02-25 8:01 ` [PATCH v2] " Yunlong Song 2017-02-25 18:46 ` Jaegeuk Kim
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).