On 2018/4/13 11:59, Jaegeuk Kim wrote: > On 04/13, Chao Yu wrote: >> On 2018/4/13 8:37, Jaegeuk Kim wrote: >>> On 04/10, heyunlei wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Jaegeuk Kim [mailto:jaeg...@kernel.org] >>>>> Sent: Tuesday, April 10, 2018 12:01 PM >>>>> To: heyunlei >>>>> Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; >>>>> Zhangdianfang (Euler) >>>>> Subject: Re: [f2fs-dev][PATCH] fsck.f2fs: recover nat bits feature >>>>> default by fsck >>>>> >>>>> On 04/09, Yunlei He wrote: >>>>>> Now, nat bits feature is enabled by default, we will >>>>>> meet with the following scenarios: >>>>>> >>>>>> i. disabled, without CP_NAT_BITS_FLAG, if fsck find some >>>>>> fs errors, fix or write new checkpoint will then enable it. >>>>>> ii. enabled, with CP_NAT_BITS_FLAG, in the case of sudden >>>>>> power off, bitmap will get lost but CP_NAT_BITS_FLAG >>>>>> still exist, fsck will recover bitmap in f2fs_do_mount. >>>>>> iii. enabled, with CP_NAT_BITS_FLAG, both of bitmap and >>>>>> CP_NAT_BITS_FLAG will get lost if not enough space for >>>>>> nat bits or nat bits check failed during mounting. >>>>>> SBI_NEED_FSCK is set, fsck will recover flag and bitmap >>>>>> before next mount. >>>>>> >>>>>> SBI_NEED_FSCK means fs is corrupted, is not suitable for >>>>>> nat bits disabled. This patch try to recover nat bits all >>>>>> by fsck, no need set SBI_NEED_FSCK flag in kernel. >>>>>> >>>>>> Signed-off-by: Yunlei He <heyun...@huawei.com> >>>>>> --- >>>>>> fsck/mount.c | 15 ++++++++++----- >>>>>> 1 file changed, 10 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/fsck/mount.c b/fsck/mount.c >>>>>> index e5574c5..2361ee0 100644 >>>>>> --- a/fsck/mount.c >>>>>> +++ b/fsck/mount.c >>>>>> @@ -2389,7 +2389,7 @@ int f2fs_do_mount(struct f2fs_sb_info *sbi) >>>>>> } >>>>>> >>>>>> /* Check nat_bits */ >>>>>> - if (c.func != DUMP && is_set_ckpt_flags(cp, CP_NAT_BITS_FLAG)) { >>>>>> + if (c.func != DUMP) { >>>>>> u_int32_t nat_bits_bytes, nat_bits_blocks; >>>>>> __le64 *kaddr; >>>>>> u_int32_t blk; >>>>>> @@ -2406,10 +2406,15 @@ int f2fs_do_mount(struct f2fs_sb_info *sbi) >>>>>> kaddr = malloc(PAGE_SIZE); >>>>>> ret = dev_read_block(kaddr, blk); >>>>>> ASSERT(ret >= 0); >>>>>> - if (*kaddr != get_cp_crc(cp)) >>>>>> - write_nat_bits(sbi, sb, cp, sbi->cur_cp); >>>>>> - else >>>>>> - MSG(0, "Info: Found valid nat_bits in >>>>>> checkpoint\n"); >>>>>> + if(is_set_ckpt_flags(cp, CP_NAT_BITS_FLAG)) { >>>>>> + if (*kaddr != get_cp_crc(cp)) >>>>>> + write_nat_bits(sbi, sb, cp, >>>>>> sbi->cur_cp); >>>>>> + else >>>>>> + MSG(0, "Info: Found valid nat_bits in >>>>>> checkpoint\n"); >>>>>> + } else if (c.func == FSCK){ >>>>>> + ASSERT_MSG("Need to recover nat_bits."); >>>>>> + c.fix_on = 1; >>>>> >>>>> What if kernel doesn't support this? >>>> >>>> Fix or write checkpoint now will enable nat bits by default if cp space is >>>> enough, >>>> So maybe it will not affect kernel not supporting nat bits? >>> >>> I don't think we really need this, since it mixes up whole thing. >> >> IIUC, Yunlei just want use a flag to detect *real* data corruption on-line, >> then >> it can be a condition to end up issuing discard from >> background/umount/fstrim to >> prevent further data losing. >> >> For that, as I suggested before, we can split in-memory SBI_NEED_FSCK to >> SBI_LOSE_NAT_BIT and SBI_NEED_FSCK, for backward compatibility, on-disk flag >> can >> still be old one as below: >> >> update_ckpt_flags() >> >> if (SBI_NEED_FSCK || SBI_LOSE_NAT_BIT) >> set_cp_flag(CP_FSCK_FLAG) >> >> How about that? > > I don't think we need to add more complexity here which will give another bug > later. Blocking discards would make sense tho, I don't think nat_bits should > be together with it.
We need a more clear flag to indicate that filesystem is corrupted to decide blocking discard, instead of using current flag which includes a state indicate losing nat_bits. So what's your opinion? keep as it is? Thanks, > > Thanks, > >> >> Thanks, >> >>> >>>> >>>>> >>>>>> + } >>>>>> free(kaddr); >>>>>> } >>>>>> return 0; >>>>>> -- >>>>>> 1.9.1 >>> >>> . >>> > > . > ------------------------------------------------------------------------------ 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
