[PATCH] f2fs: compress: remove unneed check condition

[PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
has checked SBI_POR_DOING condition, and also cluster_may_compress()
has checked CP_ERROR_FLAG condition, so remove redundant check condition
in __cluster_may_compress() for cleanup.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/compress.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 3c9d797dbdd6..532c311e3a89 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
 
 		f2fs_bug_on(sbi, !page);
 
-		if (unlikely(f2fs_cp_error(sbi)))
-			return false;
-		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
-			return false;
-
 		/* beyond EOF */
 		if (page->index >= nr_pages)
 			return false;
-- 
2.29.2

[f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
has checked SBI_POR_DOING condition, and also cluster_may_compress()
has checked CP_ERROR_FLAG condition, so remove redundant check condition
in __cluster_may_compress() for cleanup.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/compress.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 3c9d797dbdd6..532c311e3a89 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
 
 		f2fs_bug_on(sbi, !page);
 
-		if (unlikely(f2fs_cp_error(sbi)))
-			return false;
-		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
-			return false;
-
 		/* beyond EOF */
 		if (page->index >= nr_pages)
 			return false;
-- 
2.29.2



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: compress: remove unneed check condition

[Jaegeuk Kim]

On 04/21, Chao Yu wrote:
> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
> has checked SBI_POR_DOING condition, and also cluster_may_compress()
> has checked CP_ERROR_FLAG condition, so remove redundant check condition
> in __cluster_may_compress() for cleanup.

I think cp_error can get any time without synchronization. Is it safe to say
it's redundant?

> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/compress.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 3c9d797dbdd6..532c311e3a89 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>  
>  		f2fs_bug_on(sbi, !page);
>  
> -		if (unlikely(f2fs_cp_error(sbi)))
> -			return false;
> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> -			return false;
> -
>  		/* beyond EOF */
>  		if (page->index >= nr_pages)
>  			return false;
> -- 
> 2.29.2

Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Jaegeuk Kim]

On 04/21, Chao Yu wrote:
> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
> has checked SBI_POR_DOING condition, and also cluster_may_compress()
> has checked CP_ERROR_FLAG condition, so remove redundant check condition
> in __cluster_may_compress() for cleanup.

I think cp_error can get any time without synchronization. Is it safe to say
it's redundant?

> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/compress.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 3c9d797dbdd6..532c311e3a89 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>  
>  		f2fs_bug_on(sbi, !page);
>  
> -		if (unlikely(f2fs_cp_error(sbi)))
> -			return false;
> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> -			return false;
> -
>  		/* beyond EOF */
>  		if (page->index >= nr_pages)
>  			return false;
> -- 
> 2.29.2


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

On 2021/4/22 12:04, Jaegeuk Kim wrote:
> On 04/21, Chao Yu wrote:
>> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
>> has checked SBI_POR_DOING condition, and also cluster_may_compress()
>> has checked CP_ERROR_FLAG condition, so remove redundant check condition
>> in __cluster_may_compress() for cleanup.
> 
> I think cp_error can get any time without synchronization. Is it safe to say
> it's redundant?

Yes,

But no matter how late we check cp_error, cp_error can happen after our
check points, it won't cause regression if we remove cp_error check there,
because for compress write, it uses OPU, it won't overwrite any existed data
in device.

Seems it will be more appropriate to check cp_error in
f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
rather than in __cluster_may_compress().

BTW, shouldn't we rename __cluster_may_compress() to
cluster_beyond_filesize() for better readability?

Thanks,

> 
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>   fs/f2fs/compress.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>> index 3c9d797dbdd6..532c311e3a89 100644
>> --- a/fs/f2fs/compress.c
>> +++ b/fs/f2fs/compress.c
>> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>>   
>>   		f2fs_bug_on(sbi, !page);
>>   
>> -		if (unlikely(f2fs_cp_error(sbi)))
>> -			return false;
>> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>> -			return false;
>> -
>>   		/* beyond EOF */
>>   		if (page->index >= nr_pages)
>>   			return false;
>> -- 
>> 2.29.2
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

On 2021/4/22 12:04, Jaegeuk Kim wrote:
> On 04/21, Chao Yu wrote:
>> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
>> has checked SBI_POR_DOING condition, and also cluster_may_compress()
>> has checked CP_ERROR_FLAG condition, so remove redundant check condition
>> in __cluster_may_compress() for cleanup.
> 
> I think cp_error can get any time without synchronization. Is it safe to say
> it's redundant?

Yes,

But no matter how late we check cp_error, cp_error can happen after our
check points, it won't cause regression if we remove cp_error check there,
because for compress write, it uses OPU, it won't overwrite any existed data
in device.

Seems it will be more appropriate to check cp_error in
f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
rather than in __cluster_may_compress().

BTW, shouldn't we rename __cluster_may_compress() to
cluster_beyond_filesize() for better readability?

Thanks,

> 
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>   fs/f2fs/compress.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>> index 3c9d797dbdd6..532c311e3a89 100644
>> --- a/fs/f2fs/compress.c
>> +++ b/fs/f2fs/compress.c
>> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>>   
>>   		f2fs_bug_on(sbi, !page);
>>   
>> -		if (unlikely(f2fs_cp_error(sbi)))
>> -			return false;
>> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>> -			return false;
>> -
>>   		/* beyond EOF */
>>   		if (page->index >= nr_pages)
>>   			return false;
>> -- 
>> 2.29.2
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: compress: remove unneed check condition

[Jaegeuk Kim]

On 04/22, Chao Yu wrote:
> On 2021/4/22 12:04, Jaegeuk Kim wrote:
> > On 04/21, Chao Yu wrote:
> > > In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
> > > has checked SBI_POR_DOING condition, and also cluster_may_compress()
> > > has checked CP_ERROR_FLAG condition, so remove redundant check condition
> > > in __cluster_may_compress() for cleanup.
> > 
> > I think cp_error can get any time without synchronization. Is it safe to say
> > it's redundant?
> 
> Yes,
> 
> But no matter how late we check cp_error, cp_error can happen after our
> check points, it won't cause regression if we remove cp_error check there,
> because for compress write, it uses OPU, it won't overwrite any existed data
> in device.
> 
> Seems it will be more appropriate to check cp_error in
> f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
> rather than in __cluster_may_compress().
> 
> BTW, shouldn't we rename __cluster_may_compress() to
> cluster_beyond_filesize() for better readability?

f2fs_cluster_has_data()?

> 
> Thanks,
> 
> > 
> > > 
> > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > ---
> > >   fs/f2fs/compress.c | 5 -----
> > >   1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > > index 3c9d797dbdd6..532c311e3a89 100644
> > > --- a/fs/f2fs/compress.c
> > > +++ b/fs/f2fs/compress.c
> > > @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
> > >   		f2fs_bug_on(sbi, !page);
> > > -		if (unlikely(f2fs_cp_error(sbi)))
> > > -			return false;
> > > -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> > > -			return false;
> > > -
> > >   		/* beyond EOF */
> > >   		if (page->index >= nr_pages)
> > >   			return false;
> > > -- 
> > > 2.29.2
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Jaegeuk Kim]

On 04/22, Chao Yu wrote:
> On 2021/4/22 12:04, Jaegeuk Kim wrote:
> > On 04/21, Chao Yu wrote:
> > > In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
> > > has checked SBI_POR_DOING condition, and also cluster_may_compress()
> > > has checked CP_ERROR_FLAG condition, so remove redundant check condition
> > > in __cluster_may_compress() for cleanup.
> > 
> > I think cp_error can get any time without synchronization. Is it safe to say
> > it's redundant?
> 
> Yes,
> 
> But no matter how late we check cp_error, cp_error can happen after our
> check points, it won't cause regression if we remove cp_error check there,
> because for compress write, it uses OPU, it won't overwrite any existed data
> in device.
> 
> Seems it will be more appropriate to check cp_error in
> f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
> rather than in __cluster_may_compress().
> 
> BTW, shouldn't we rename __cluster_may_compress() to
> cluster_beyond_filesize() for better readability?

f2fs_cluster_has_data()?

> 
> Thanks,
> 
> > 
> > > 
> > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > ---
> > >   fs/f2fs/compress.c | 5 -----
> > >   1 file changed, 5 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > > index 3c9d797dbdd6..532c311e3a89 100644
> > > --- a/fs/f2fs/compress.c
> > > +++ b/fs/f2fs/compress.c
> > > @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
> > >   		f2fs_bug_on(sbi, !page);
> > > -		if (unlikely(f2fs_cp_error(sbi)))
> > > -			return false;
> > > -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> > > -			return false;
> > > -
> > >   		/* beyond EOF */
> > >   		if (page->index >= nr_pages)
> > >   			return false;
> > > -- 
> > > 2.29.2
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

On 2021/4/25 8:47, Jaegeuk Kim wrote:
> On 04/22, Chao Yu wrote:
>> On 2021/4/22 12:04, Jaegeuk Kim wrote:
>>> On 04/21, Chao Yu wrote:
>>>> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
>>>> has checked SBI_POR_DOING condition, and also cluster_may_compress()
>>>> has checked CP_ERROR_FLAG condition, so remove redundant check condition
>>>> in __cluster_may_compress() for cleanup.
>>>
>>> I think cp_error can get any time without synchronization. Is it safe to say
>>> it's redundant?
>>
>> Yes,
>>
>> But no matter how late we check cp_error, cp_error can happen after our
>> check points, it won't cause regression if we remove cp_error check there,
>> because for compress write, it uses OPU, it won't overwrite any existed data
>> in device.
>>
>> Seems it will be more appropriate to check cp_error in
>> f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
>> rather than in __cluster_may_compress().
>>
>> BTW, shouldn't we rename __cluster_may_compress() to
>> cluster_beyond_filesize() for better readability?
> 
> f2fs_cluster_has_data()?

Maybe cluster_has_invalid_data()? which indicates there is invalid data
beyond filesize.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>    fs/f2fs/compress.c | 5 -----
>>>>    1 file changed, 5 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>>>> index 3c9d797dbdd6..532c311e3a89 100644
>>>> --- a/fs/f2fs/compress.c
>>>> +++ b/fs/f2fs/compress.c
>>>> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>>>>    		f2fs_bug_on(sbi, !page);
>>>> -		if (unlikely(f2fs_cp_error(sbi)))
>>>> -			return false;
>>>> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>>>> -			return false;
>>>> -
>>>>    		/* beyond EOF */
>>>>    		if (page->index >= nr_pages)
>>>>    			return false;
>>>> -- 
>>>> 2.29.2
>>> .
>>>
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

On 2021/4/25 8:47, Jaegeuk Kim wrote:
> On 04/22, Chao Yu wrote:
>> On 2021/4/22 12:04, Jaegeuk Kim wrote:
>>> On 04/21, Chao Yu wrote:
>>>> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
>>>> has checked SBI_POR_DOING condition, and also cluster_may_compress()
>>>> has checked CP_ERROR_FLAG condition, so remove redundant check condition
>>>> in __cluster_may_compress() for cleanup.
>>>
>>> I think cp_error can get any time without synchronization. Is it safe to say
>>> it's redundant?
>>
>> Yes,
>>
>> But no matter how late we check cp_error, cp_error can happen after our
>> check points, it won't cause regression if we remove cp_error check there,
>> because for compress write, it uses OPU, it won't overwrite any existed data
>> in device.
>>
>> Seems it will be more appropriate to check cp_error in
>> f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
>> rather than in __cluster_may_compress().
>>
>> BTW, shouldn't we rename __cluster_may_compress() to
>> cluster_beyond_filesize() for better readability?
> 
> f2fs_cluster_has_data()?

Maybe cluster_has_invalid_data()? which indicates there is invalid data
beyond filesize.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>> ---
>>>>    fs/f2fs/compress.c | 5 -----
>>>>    1 file changed, 5 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>>>> index 3c9d797dbdd6..532c311e3a89 100644
>>>> --- a/fs/f2fs/compress.c
>>>> +++ b/fs/f2fs/compress.c
>>>> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>>>>    		f2fs_bug_on(sbi, !page);
>>>> -		if (unlikely(f2fs_cp_error(sbi)))
>>>> -			return false;
>>>> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>>>> -			return false;
>>>> -
>>>>    		/* beyond EOF */
>>>>    		if (page->index >= nr_pages)
>>>>    			return false;
>>>> -- 
>>>> 2.29.2
>>> .
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: compress: remove unneed check condition

[Jaegeuk Kim]

On 04/25, Chao Yu wrote:
> On 2021/4/25 8:47, Jaegeuk Kim wrote:
> > On 04/22, Chao Yu wrote:
> > > On 2021/4/22 12:04, Jaegeuk Kim wrote:
> > > > On 04/21, Chao Yu wrote:
> > > > > In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
> > > > > has checked SBI_POR_DOING condition, and also cluster_may_compress()
> > > > > has checked CP_ERROR_FLAG condition, so remove redundant check condition
> > > > > in __cluster_may_compress() for cleanup.
> > > > 
> > > > I think cp_error can get any time without synchronization. Is it safe to say
> > > > it's redundant?
> > > 
> > > Yes,
> > > 
> > > But no matter how late we check cp_error, cp_error can happen after our
> > > check points, it won't cause regression if we remove cp_error check there,
> > > because for compress write, it uses OPU, it won't overwrite any existed data
> > > in device.
> > > 
> > > Seems it will be more appropriate to check cp_error in
> > > f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
> > > rather than in __cluster_may_compress().
> > > 
> > > BTW, shouldn't we rename __cluster_may_compress() to
> > > cluster_beyond_filesize() for better readability?
> > 
> > f2fs_cluster_has_data()?
> 
> Maybe cluster_has_invalid_data()? which indicates there is invalid data
> beyond filesize.

BTW, we can compress it with zero data?

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > ---
> > > > >    fs/f2fs/compress.c | 5 -----
> > > > >    1 file changed, 5 deletions(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > > > > index 3c9d797dbdd6..532c311e3a89 100644
> > > > > --- a/fs/f2fs/compress.c
> > > > > +++ b/fs/f2fs/compress.c
> > > > > @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
> > > > >    		f2fs_bug_on(sbi, !page);
> > > > > -		if (unlikely(f2fs_cp_error(sbi)))
> > > > > -			return false;
> > > > > -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> > > > > -			return false;
> > > > > -
> > > > >    		/* beyond EOF */
> > > > >    		if (page->index >= nr_pages)
> > > > >    			return false;
> > > > > -- 
> > > > > 2.29.2
> > > > .
> > > > 
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Jaegeuk Kim]

On 04/25, Chao Yu wrote:
> On 2021/4/25 8:47, Jaegeuk Kim wrote:
> > On 04/22, Chao Yu wrote:
> > > On 2021/4/22 12:04, Jaegeuk Kim wrote:
> > > > On 04/21, Chao Yu wrote:
> > > > > In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
> > > > > has checked SBI_POR_DOING condition, and also cluster_may_compress()
> > > > > has checked CP_ERROR_FLAG condition, so remove redundant check condition
> > > > > in __cluster_may_compress() for cleanup.
> > > > 
> > > > I think cp_error can get any time without synchronization. Is it safe to say
> > > > it's redundant?
> > > 
> > > Yes,
> > > 
> > > But no matter how late we check cp_error, cp_error can happen after our
> > > check points, it won't cause regression if we remove cp_error check there,
> > > because for compress write, it uses OPU, it won't overwrite any existed data
> > > in device.
> > > 
> > > Seems it will be more appropriate to check cp_error in
> > > f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
> > > rather than in __cluster_may_compress().
> > > 
> > > BTW, shouldn't we rename __cluster_may_compress() to
> > > cluster_beyond_filesize() for better readability?
> > 
> > f2fs_cluster_has_data()?
> 
> Maybe cluster_has_invalid_data()? which indicates there is invalid data
> beyond filesize.

BTW, we can compress it with zero data?

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > > > > ---
> > > > >    fs/f2fs/compress.c | 5 -----
> > > > >    1 file changed, 5 deletions(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> > > > > index 3c9d797dbdd6..532c311e3a89 100644
> > > > > --- a/fs/f2fs/compress.c
> > > > > +++ b/fs/f2fs/compress.c
> > > > > @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
> > > > >    		f2fs_bug_on(sbi, !page);
> > > > > -		if (unlikely(f2fs_cp_error(sbi)))
> > > > > -			return false;
> > > > > -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
> > > > > -			return false;
> > > > > -
> > > > >    		/* beyond EOF */
> > > > >    		if (page->index >= nr_pages)
> > > > >    			return false;
> > > > > -- 
> > > > > 2.29.2
> > > > .
> > > > 
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

On 2021/4/27 1:05, Jaegeuk Kim wrote:
> On 04/25, Chao Yu wrote:
>> On 2021/4/25 8:47, Jaegeuk Kim wrote:
>>> On 04/22, Chao Yu wrote:
>>>> On 2021/4/22 12:04, Jaegeuk Kim wrote:
>>>>> On 04/21, Chao Yu wrote:
>>>>>> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
>>>>>> has checked SBI_POR_DOING condition, and also cluster_may_compress()
>>>>>> has checked CP_ERROR_FLAG condition, so remove redundant check condition
>>>>>> in __cluster_may_compress() for cleanup.
>>>>>
>>>>> I think cp_error can get any time without synchronization. Is it safe to say
>>>>> it's redundant?
>>>>
>>>> Yes,
>>>>
>>>> But no matter how late we check cp_error, cp_error can happen after our
>>>> check points, it won't cause regression if we remove cp_error check there,
>>>> because for compress write, it uses OPU, it won't overwrite any existed data
>>>> in device.
>>>>
>>>> Seems it will be more appropriate to check cp_error in
>>>> f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
>>>> rather than in __cluster_may_compress().
>>>>
>>>> BTW, shouldn't we rename __cluster_may_compress() to
>>>> cluster_beyond_filesize() for better readability?
>>>
>>> f2fs_cluster_has_data()?
>>
>> Maybe cluster_has_invalid_data()? which indicates there is invalid data
>> beyond filesize.
> 
> BTW, we can compress it with zero data?

I doubt it will cause unnecessary overhead for below condition?
- write 1GB data into file
- truncate file to 0
- writeback 1GB compressed cluster contains zero data

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>>     fs/f2fs/compress.c | 5 -----
>>>>>>     1 file changed, 5 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>>>>>> index 3c9d797dbdd6..532c311e3a89 100644
>>>>>> --- a/fs/f2fs/compress.c
>>>>>> +++ b/fs/f2fs/compress.c
>>>>>> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>>>>>>     		f2fs_bug_on(sbi, !page);
>>>>>> -		if (unlikely(f2fs_cp_error(sbi)))
>>>>>> -			return false;
>>>>>> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>>>>>> -			return false;
>>>>>> -
>>>>>>     		/* beyond EOF */
>>>>>>     		if (page->index >= nr_pages)
>>>>>>     			return false;
>>>>>> -- 
>>>>>> 2.29.2
>>>>> .
>>>>>
>>> .
>>>
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: compress: remove unneed check condition

[Chao Yu]

On 2021/4/27 1:05, Jaegeuk Kim wrote:
> On 04/25, Chao Yu wrote:
>> On 2021/4/25 8:47, Jaegeuk Kim wrote:
>>> On 04/22, Chao Yu wrote:
>>>> On 2021/4/22 12:04, Jaegeuk Kim wrote:
>>>>> On 04/21, Chao Yu wrote:
>>>>>> In only call path of __cluster_may_compress(), __f2fs_write_data_pages()
>>>>>> has checked SBI_POR_DOING condition, and also cluster_may_compress()
>>>>>> has checked CP_ERROR_FLAG condition, so remove redundant check condition
>>>>>> in __cluster_may_compress() for cleanup.
>>>>>
>>>>> I think cp_error can get any time without synchronization. Is it safe to say
>>>>> it's redundant?
>>>>
>>>> Yes,
>>>>
>>>> But no matter how late we check cp_error, cp_error can happen after our
>>>> check points, it won't cause regression if we remove cp_error check there,
>>>> because for compress write, it uses OPU, it won't overwrite any existed data
>>>> in device.
>>>>
>>>> Seems it will be more appropriate to check cp_error in
>>>> f2fs_write_compressed_pages() like we did in f2fs_write_single_data_page()
>>>> rather than in __cluster_may_compress().
>>>>
>>>> BTW, shouldn't we rename __cluster_may_compress() to
>>>> cluster_beyond_filesize() for better readability?
>>>
>>> f2fs_cluster_has_data()?
>>
>> Maybe cluster_has_invalid_data()? which indicates there is invalid data
>> beyond filesize.
> 
> BTW, we can compress it with zero data?

I doubt it will cause unnecessary overhead for below condition?
- write 1GB data into file
- truncate file to 0
- writeback 1GB compressed cluster contains zero data

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>>>>> ---
>>>>>>     fs/f2fs/compress.c | 5 -----
>>>>>>     1 file changed, 5 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>>>>>> index 3c9d797dbdd6..532c311e3a89 100644
>>>>>> --- a/fs/f2fs/compress.c
>>>>>> +++ b/fs/f2fs/compress.c
>>>>>> @@ -906,11 +906,6 @@ static bool __cluster_may_compress(struct compress_ctx *cc)
>>>>>>     		f2fs_bug_on(sbi, !page);
>>>>>> -		if (unlikely(f2fs_cp_error(sbi)))
>>>>>> -			return false;
>>>>>> -		if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>>>>>> -			return false;
>>>>>> -
>>>>>>     		/* beyond EOF */
>>>>>>     		if (page->index >= nr_pages)
>>>>>>     			return false;
>>>>>> -- 
>>>>>> 2.29.2
>>>>> .
>>>>>
>>> .
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel