[PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Jaegeuk Kim]

If we met this once, let fsck.f2fs clear this only.
Note that, this addresses all the subtle fault injection test.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/checkpoint.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 03fea4efd64b..10a3ada28715 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
-	else
-		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
 
 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
-- 
2.19.0.605.g01d371f741-goog

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/12 10:33, Jaegeuk Kim wrote:
> If we met this once, let fsck.f2fs clear this only.
> Note that, this addresses all the subtle fault injection test.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 03fea4efd64b..10a3ada28715 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> -	else
> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);

I didn't get it, previously, if we didn't persist all quota file's data in
checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
checkpoint, we have persisted all quota file's data, quota files are consistent
with all other files in filesystem, why we can't remove this NEED_FSCK flag..?

Thanks,

>  
>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Jaegeuk Kim]

On 02/13, Chao Yu wrote:
> On 2019/2/12 10:33, Jaegeuk Kim wrote:
> > If we met this once, let fsck.f2fs clear this only.
> > Note that, this addresses all the subtle fault injection test.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 03fea4efd64b..10a3ada28715 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  
> >  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> > -	else
> > -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 
> I didn't get it, previously, if we didn't persist all quota file's data in
> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
> checkpoint, we have persisted all quota file's data, quota files are consistent
> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?

I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
checkpoint?

> 
> Thanks,
> 
> >  
> >  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> > 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/16 12:55, Jaegeuk Kim wrote:
> On 02/13, Chao Yu wrote:
>> On 2019/2/12 10:33, Jaegeuk Kim wrote:
>>> If we met this once, let fsck.f2fs clear this only.
>>> Note that, this addresses all the subtle fault injection test.
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/checkpoint.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 03fea4efd64b..10a3ada28715 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  
>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>> -	else
>>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>
>> I didn't get it, previously, if we didn't persist all quota file's data in
>> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
>> checkpoint, we have persisted all quota file's data, quota files are consistent
>> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
> 
> I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear

I know it's subtle... and I agreed we can fix it like this in upstream
first, but in our product, it's not rare that we hit the
QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
caused by quota repairing of fsck.

> SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
> checkpoint?

But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
again, right?

if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);


So in order to figure out whether this is caused by out-of-order execution
of below assignments:

	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
	else
		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later

	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first


Could you have a try:

	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
	else
		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);

Thanks,

> 
>>
>> Thanks,
>>
>>>  
>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Jaegeuk Kim]

On 02/18, Chao Yu wrote:
> On 2019/2/16 12:55, Jaegeuk Kim wrote:
> > On 02/13, Chao Yu wrote:
> >> On 2019/2/12 10:33, Jaegeuk Kim wrote:
> >>> If we met this once, let fsck.f2fs clear this only.
> >>> Note that, this addresses all the subtle fault injection test.
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>> ---
> >>>  fs/f2fs/checkpoint.c | 2 --
> >>>  1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index 03fea4efd64b..10a3ada28715 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>  
> >>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>> -	else
> >>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>
> >> I didn't get it, previously, if we didn't persist all quota file's data in
> >> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
> >> checkpoint, we have persisted all quota file's data, quota files are consistent
> >> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
> > 
> > I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
> 
> I know it's subtle... and I agreed we can fix it like this in upstream
> first, but in our product, it's not rare that we hit the
> QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
> caused by quota repairing of fsck.
> 
> > SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
> > checkpoint?
> 
> But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
> again, right?
> 
> if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 
> 
> So in order to figure out whether this is caused by out-of-order execution
> of below assignments:
> 
> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 	else
> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
> 
> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
> 
> 
> Could you have a try:
> 
> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
> 			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 	else
> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);

What does this mean? I'm in doubt we have a missing case where we clear this
flag, CP_QUOTA_NEED_FSCK_FLAG.

> 
> Thanks,
> 
> > 
> >>
> >> Thanks,
> >>
> >>>  
> >>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/20 15:08, Jaegeuk Kim wrote:
> On 02/18, Chao Yu wrote:
>> On 2019/2/16 12:55, Jaegeuk Kim wrote:
>>> On 02/13, Chao Yu wrote:
>>>> On 2019/2/12 10:33, Jaegeuk Kim wrote:
>>>>> If we met this once, let fsck.f2fs clear this only.
>>>>> Note that, this addresses all the subtle fault injection test.
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>>  fs/f2fs/checkpoint.c | 2 --
>>>>>  1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>> index 03fea4efd64b..10a3ada28715 100644
>>>>> --- a/fs/f2fs/checkpoint.c
>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>  
>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>> -	else
>>>>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>
>>>> I didn't get it, previously, if we didn't persist all quota file's data in
>>>> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
>>>> checkpoint, we have persisted all quota file's data, quota files are consistent
>>>> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
>>>
>>> I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
>>
>> I know it's subtle... and I agreed we can fix it like this in upstream
>> first, but in our product, it's not rare that we hit the
>> QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
>> caused by quota repairing of fsck.
>>
>>> SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
>>> checkpoint?
>>
>> But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
>> again, right?
>>
>> if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>
>>
>> So in order to figure out whether this is caused by out-of-order execution
>> of below assignments:
>>
>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>> 	else
>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
>>
>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
>>
>>
>> Could you have a try:
>>
>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
>> 			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>> 	else
>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 
> What does this mean? I'm in doubt we have a missing case where we clear this

Cpu pipeline / compiler can make code out-of-order execution, which means:

a = 1;
b = 2;

may actually be executed as the order of:

b = 2;
a = 1;

So I doubt that below two line codes can be executed out-of-order:

else
	__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later

if ()
	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first

> flag, CP_QUOTA_NEED_FSCK_FLAG.

Agreed, I've checked each operation in f2fs_quota_operations yesterday, and
didn't find any missing places yet...

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>  
>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>
>>>
>>> .
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/20 15:25, Chao Yu wrote:
> On 2019/2/20 15:08, Jaegeuk Kim wrote:
>> On 02/18, Chao Yu wrote:
>>> On 2019/2/16 12:55, Jaegeuk Kim wrote:
>>>> On 02/13, Chao Yu wrote:
>>>>> On 2019/2/12 10:33, Jaegeuk Kim wrote:
>>>>>> If we met this once, let fsck.f2fs clear this only.
>>>>>> Note that, this addresses all the subtle fault injection test.
>>>>>>
>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>> ---
>>>>>>  fs/f2fs/checkpoint.c | 2 --
>>>>>>  1 file changed, 2 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>> index 03fea4efd64b..10a3ada28715 100644
>>>>>> --- a/fs/f2fs/checkpoint.c
>>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>  
>>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>> -	else
>>>>>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>
>>>>> I didn't get it, previously, if we didn't persist all quota file's data in
>>>>> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
>>>>> checkpoint, we have persisted all quota file's data, quota files are consistent
>>>>> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
>>>>
>>>> I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
>>>
>>> I know it's subtle... and I agreed we can fix it like this in upstream
>>> first, but in our product, it's not rare that we hit the
>>> QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
>>> caused by quota repairing of fsck.
>>>
>>>> SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
>>>> checkpoint?
>>>
>>> But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
>>> again, right?
>>>
>>> if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>
>>>
>>> So in order to figure out whether this is caused by out-of-order execution
>>> of below assignments:
>>>
>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>> 	else
>>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
>>>
>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
>>>
>>>
>>> Could you have a try:
>>>
>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
>>> 			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>> 	else
>>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>
>> What does this mean? I'm in doubt we have a missing case where we clear this
> 
> Cpu pipeline / compiler can make code out-of-order execution, which means:
> 
> a = 1;
> b = 2;
> 
> may actually be executed as the order of:
> 
> b = 2;
> a = 1;
> 
> So I doubt that below two line codes can be executed out-of-order:
> 
> else
> 	__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
> 
> if ()
> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
> 
>> flag, CP_QUOTA_NEED_FSCK_FLAG.
> 
> Agreed, I've checked each operation in f2fs_quota_operations yesterday, and
> didn't find any missing places yet...

Oh, I mean the place where set SBI_QUOTA_NEED_REPAIR, I also doubt we
missed to set the flag.

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>  
>>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>>
>>>>
>>>> .
>>>>
>>
>> .
>>
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Jaegeuk Kim]

On 02/20, Chao Yu wrote:
> On 2019/2/20 15:25, Chao Yu wrote:
> > On 2019/2/20 15:08, Jaegeuk Kim wrote:
> >> On 02/18, Chao Yu wrote:
> >>> On 2019/2/16 12:55, Jaegeuk Kim wrote:
> >>>> On 02/13, Chao Yu wrote:
> >>>>> On 2019/2/12 10:33, Jaegeuk Kim wrote:
> >>>>>> If we met this once, let fsck.f2fs clear this only.
> >>>>>> Note that, this addresses all the subtle fault injection test.
> >>>>>>
> >>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>>> ---
> >>>>>>  fs/f2fs/checkpoint.c | 2 --
> >>>>>>  1 file changed, 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>>>>> index 03fea4efd64b..10a3ada28715 100644
> >>>>>> --- a/fs/f2fs/checkpoint.c
> >>>>>> +++ b/fs/f2fs/checkpoint.c
> >>>>>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>  
> >>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>> -	else
> >>>>>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>
> >>>>> I didn't get it, previously, if we didn't persist all quota file's data in
> >>>>> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
> >>>>> checkpoint, we have persisted all quota file's data, quota files are consistent
> >>>>> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
> >>>>
> >>>> I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
> >>>
> >>> I know it's subtle... and I agreed we can fix it like this in upstream
> >>> first, but in our product, it's not rare that we hit the
> >>> QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
> >>> caused by quota repairing of fsck.
> >>>
> >>>> SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
> >>>> checkpoint?
> >>>
> >>> But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
> >>> again, right?
> >>>
> >>> if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>
> >>>
> >>> So in order to figure out whether this is caused by out-of-order execution
> >>> of below assignments:
> >>>
> >>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>> 	else
> >>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
> >>>
> >>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
> >>>
> >>>
> >>> Could you have a try:
> >>>
> >>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
> >>> 			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>> 	else
> >>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>
> >> What does this mean? I'm in doubt we have a missing case where we clear this
> > 
> > Cpu pipeline / compiler can make code out-of-order execution, which means:
> > 
> > a = 1;
> > b = 2;
> > 
> > may actually be executed as the order of:
> > 
> > b = 2;
> > a = 1;
> > 
> > So I doubt that below two line codes can be executed out-of-order:
> > 
> > else
> > 	__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
> > 
> > if ()
> > 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first

In spin_lock_irqsave()?

> > 
> >> flag, CP_QUOTA_NEED_FSCK_FLAG.
> > 
> > Agreed, I've checked each operation in f2fs_quota_operations yesterday, and
> > didn't find any missing places yet...
> 
> Oh, I mean the place where set SBI_QUOTA_NEED_REPAIR, I also doubt we
> missed to set the flag.

So, I may need to keep this patch untill we find the missing case. I'll keep an
eye on this.

> 
> Thanks,
> 
> > 
> > Thanks,
> > 
> >>
> >>>
> >>> Thanks,
> >>>
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>>  
> >>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>>
> >>>>
> >>>> .
> >>>>
> >>
> >> .
> >>
> > 
> > 
> > 
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/22 10:40, Jaegeuk Kim wrote:
> On 02/20, Chao Yu wrote:
>> On 2019/2/20 15:25, Chao Yu wrote:
>>> On 2019/2/20 15:08, Jaegeuk Kim wrote:
>>>> On 02/18, Chao Yu wrote:
>>>>> On 2019/2/16 12:55, Jaegeuk Kim wrote:
>>>>>> On 02/13, Chao Yu wrote:
>>>>>>> On 2019/2/12 10:33, Jaegeuk Kim wrote:
>>>>>>>> If we met this once, let fsck.f2fs clear this only.
>>>>>>>> Note that, this addresses all the subtle fault injection test.
>>>>>>>>
>>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>>>>> ---
>>>>>>>>  fs/f2fs/checkpoint.c | 2 --
>>>>>>>>  1 file changed, 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>>>>> index 03fea4efd64b..10a3ada28715 100644
>>>>>>>> --- a/fs/f2fs/checkpoint.c
>>>>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>>>>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>>>>  
>>>>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>>>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>>>> -	else
>>>>>>>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>>>
>>>>>>> I didn't get it, previously, if we didn't persist all quota file's data in
>>>>>>> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
>>>>>>> checkpoint, we have persisted all quota file's data, quota files are consistent
>>>>>>> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
>>>>>>
>>>>>> I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
>>>>>
>>>>> I know it's subtle... and I agreed we can fix it like this in upstream
>>>>> first, but in our product, it's not rare that we hit the
>>>>> QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
>>>>> caused by quota repairing of fsck.
>>>>>
>>>>>> SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
>>>>>> checkpoint?
>>>>>
>>>>> But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
>>>>> again, right?
>>>>>
>>>>> if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>>>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>
>>>>>
>>>>> So in order to figure out whether this is caused by out-of-order execution
>>>>> of below assignments:
>>>>>
>>>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>> 	else
>>>>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
>>>>>
>>>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
>>>>>
>>>>>
>>>>> Could you have a try:
>>>>>
>>>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
>>>>> 			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>>>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>> 	else
>>>>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>
>>>> What does this mean? I'm in doubt we have a missing case where we clear this
>>>
>>> Cpu pipeline / compiler can make code out-of-order execution, which means:
>>>
>>> a = 1;
>>> b = 2;
>>>
>>> may actually be executed as the order of:
>>>
>>> b = 2;
>>> a = 1;
>>>
>>> So I doubt that below two line codes can be executed out-of-order:
>>>
>>> else
>>> 	__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
>>>
>>> if ()
>>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
> 
> In spin_lock_irqsave()?

Not sure in coverage of spin_lock, system can guarantee codes being
executed orderly.

> 
>>>
>>>> flag, CP_QUOTA_NEED_FSCK_FLAG.
>>>
>>> Agreed, I've checked each operation in f2fs_quota_operations yesterday, and
>>> didn't find any missing places yet...
>>
>> Oh, I mean the place where set SBI_QUOTA_NEED_REPAIR, I also doubt we
>> missed to set the flag.
> 
> So, I may need to keep this patch untill we find the missing case. I'll keep an
> eye on this.

Agreed, do you mind adding one line comment there to notice that?

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>>  
>>>>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>>>>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
>>>>>>>>
>>>>>>
>>>>>> .
>>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> Linux-f2fs-devel mailing list
>>> Linux-f2fs-devel@lists.sourceforge.net
>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>>
>>> .
>>>
> 
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Jaegeuk Kim]

On 02/22, Chao Yu wrote:
> On 2019/2/22 10:40, Jaegeuk Kim wrote:
> > On 02/20, Chao Yu wrote:
> >> On 2019/2/20 15:25, Chao Yu wrote:
> >>> On 2019/2/20 15:08, Jaegeuk Kim wrote:
> >>>> On 02/18, Chao Yu wrote:
> >>>>> On 2019/2/16 12:55, Jaegeuk Kim wrote:
> >>>>>> On 02/13, Chao Yu wrote:
> >>>>>>> On 2019/2/12 10:33, Jaegeuk Kim wrote:
> >>>>>>>> If we met this once, let fsck.f2fs clear this only.
> >>>>>>>> Note that, this addresses all the subtle fault injection test.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>>>>>>> ---
> >>>>>>>>  fs/f2fs/checkpoint.c | 2 --
> >>>>>>>>  1 file changed, 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>>>>>>> index 03fea4efd64b..10a3ada28715 100644
> >>>>>>>> --- a/fs/f2fs/checkpoint.c
> >>>>>>>> +++ b/fs/f2fs/checkpoint.c
> >>>>>>>> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >>>>>>>>  
> >>>>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>>>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>>>> -	else
> >>>>>>>> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>>>
> >>>>>>> I didn't get it, previously, if we didn't persist all quota file's data in
> >>>>>>> checkpoint, then we will tag CP_QUOTA_NEED_FSCK_FLAG in CP area, but in current
> >>>>>>> checkpoint, we have persisted all quota file's data, quota files are consistent
> >>>>>>> with all other files in filesystem, why we can't remove this NEED_FSCK flag..?
> >>>>>>
> >>>>>> I said it's subtle. So, I guessed 1) set CP_QUOTA_NEED_FSCK_FLAG, 2) clear
> >>>>>
> >>>>> I know it's subtle... and I agreed we can fix it like this in upstream
> >>>>> first, but in our product, it's not rare that we hit the
> >>>>> QUOTA_SKIP_FLUSH(its value is 4) case, later we may encounter long latency
> >>>>> caused by quota repairing of fsck.
> >>>>>
> >>>>>> SBI_QUOTA_SKIP_FLUSH by checkpoint, 3) clear CP_QUOTA_NEED_FSCK_FLAG by another
> >>>>>> checkpoint?
> >>>>>
> >>>>> But later if QUOTA_NEED_REPAIR is set, we will set QUOTA_NEED_FSCK_FLAG
> >>>>> again, right?
> >>>>>
> >>>>> if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>>>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>
> >>>>>
> >>>>> So in order to figure out whether this is caused by out-of-order execution
> >>>>> of below assignments:
> >>>>>
> >>>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>> 	else
> >>>>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
> >>>>>
> >>>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
> >>>>>
> >>>>>
> >>>>> Could you have a try:
> >>>>>
> >>>>> 	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR) ||
> >>>>> 			is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >>>>> 		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>> 	else
> >>>>> 		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>
> >>>> What does this mean? I'm in doubt we have a missing case where we clear this
> >>>
> >>> Cpu pipeline / compiler can make code out-of-order execution, which means:
> >>>
> >>> a = 1;
> >>> b = 2;
> >>>
> >>> may actually be executed as the order of:
> >>>
> >>> b = 2;
> >>> a = 1;
> >>>
> >>> So I doubt that below two line codes can be executed out-of-order:
> >>>
> >>> else
> >>> 	__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- clear flag later
> >>>
> >>> if ()
> >>> 	__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG); --- set flag first
> > 
> > In spin_lock_irqsave()?
> 
> Not sure in coverage of spin_lock, system can guarantee codes being
> executed orderly.
> 
> > 
> >>>
> >>>> flag, CP_QUOTA_NEED_FSCK_FLAG.
> >>>
> >>> Agreed, I've checked each operation in f2fs_quota_operations yesterday, and
> >>> didn't find any missing places yet...
> >>
> >> Oh, I mean the place where set SBI_QUOTA_NEED_REPAIR, I also doubt we
> >> missed to set the flag.
> > 
> > So, I may need to keep this patch untill we find the missing case. I'll keep an
> > eye on this.
> 
> Agreed, do you mind adding one line comment there to notice that?

       /*
        * TODO: we count on fsck.f2fs to clear this flag until we figure out
        * missing cases which clear it incorrectly.
        */

I added like this.

Thanks,


> 
> Thanks,
> 
> > 
> >>
> >> Thanks,
> >>
> >>>
> >>> Thanks,
> >>>
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>>>  
> >>>>>>>>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
> >>>>>>>>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> >>>>>>>>
> >>>>>>
> >>>>>> .
> >>>>>>
> >>>>
> >>>> .
> >>>>
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> Linux-f2fs-devel mailing list
> >>> Linux-f2fs-devel@lists.sourceforge.net
> >>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>>
> >>> .
> >>>
> > 
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/27 1:57, Jaegeuk Kim wrote:
>        /*
>         * TODO: we count on fsck.f2fs to clear this flag until we figure out
>         * missing cases which clear it incorrectly.
>         */
> 
> I added like this.

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

Thanks,

> 
> Thanks,

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Chao Yu]

On 2019/2/12 10:33, Jaegeuk Kim wrote:
> If we met this once, let fsck.f2fs clear this only.
> Note that, this addresses all the subtle fault injection test.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/checkpoint.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 03fea4efd64b..10a3ada28715 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  
>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> -	else
> -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);

Jaegeuk,

Will below commit fix this issue? Not sure, but just want to check that.. :)

f2fs-tools: fix to check total valid block count before block allocation

Thanks,

>  
>  	if (is_sbi_flag_set(sbi, SBI_QUOTA_NEED_REPAIR))
>  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 

Re: [f2fs-dev] [PATCH] f2fs: don't clear CP_QUOTA_NEED_FSCK_FLAG

[Jaegeuk Kim]

On 05/10, Chao Yu wrote:
> On 2019/2/12 10:33, Jaegeuk Kim wrote:
> > If we met this once, let fsck.f2fs clear this only.
> > Note that, this addresses all the subtle fault injection test.
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/checkpoint.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 03fea4efd64b..10a3ada28715 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -1267,8 +1267,6 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  
> >  	if (is_sbi_flag_set(sbi, SBI_QUOTA_SKIP_FLUSH))
> >  		__set_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> > -	else
> > -		__clear_ckpt_flags(ckpt, CP_QUOTA_NEED_FSCK_FLAG);
> 
> Jaegeuk,
> 
> Will below commit fix this issue? Not sure, but just want to check that.. :)
> 
> f2fs-tools: fix to check total valid block count before block allocation

I started test again whether we can revert this or not. :)
Let's see.