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.

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

Reply via email to