Re: [PATCH] fsck.f2fs: recover nat bits feature default by fsck

[Chao Yu <yuchao0@huawei.com>]

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:jaegeuk@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 <heyunlei@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