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

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

[Yunlei He]

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;
+		}
 		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

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

[Chao Yu]

On 2018/4/9 11:34, 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>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

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

[Jaegeuk Kim]

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?

> +		}
>  		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

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

[heyunlei]

>-----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?

>
>> +		}
>>  		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

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

[Jaegeuk Kim]

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.

> 
> >
> >> +		}
> >>  		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

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

[Chao Yu]

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?

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

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

[Jaegeuk Kim]

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.

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

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

[Chao Yu]

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

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

[heyunlei]

>-----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?

How about patch as below:

@@ -1055,6 +1055,7 @@ enum {
        SBI_POR_DOING,                          /* recovery is doing or not */
        SBI_NEED_SB_WRITE,                      /* need to recover superblock */
        SBI_NEED_CP,                            /* need to checkpoint */
+       SBI_DISABLE_NAT_BITS,                   /* disable nat bits temporay */
 };

 enum {
@@ -1517,11 +1518,10 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 {
        unsigned long flags;

-       set_sbi_flag(sbi, SBI_NEED_FSCK);
+       set_sbi_flag(sbi, SBI_DISABLE_NAT_BITS);

        if (lock)
                spin_lock_irqsave(&sbi->cp_lock, flags);
-       __clear_ckpt_flags(F2FS_CKPT(sbi), CP_NAT_BITS_FLAG);
        kfree(NM_I(sbi)->nat_bits);
        NM_I(sbi)->nat_bits = NULL;
        if (lock)
@@ -1531,7 +1531,8 @@ static inline void disable_nat_bits(struct f2fs_sb_info *sbi, bool lock)
 static inline bool enabled_nat_bits(struct f2fs_sb_info *sbi,
                                        struct cp_control *cpc)
 {
-       bool set = is_set_ckpt_flags(sbi, CP_NAT_BITS_FLAG);
+       bool set = is_set_ckpt_flags(sbi, CP_NAT_BITS_FLAG) &&
+                       !is_sbi_flag_set(sbi, SBI_DISABLE_NAT_BITS);

        return (cpc) ? (cpc->reason & CP_UMOUNT) && set : set;
 }

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