2021-04-21 08:57:24

by Chao Yu

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

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 <[email protected]>
---
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

2021-04-22 04:05:51

by Jaegeuk Kim

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

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 <[email protected]>
> ---
> 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

2021-04-22 06:52:52

by Chao Yu

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

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 <[email protected]>
>> ---
>> 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
> .
>

2021-04-25 00:48:50

by Jaegeuk Kim

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

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 <[email protected]>
> > > ---
> > > 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
> > .
> >

2021-04-25 01:30:33

by Chao Yu

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

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 <[email protected]>
>>>> ---
>>>> 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
>>> .
>>>
> .
>

2021-04-26 17:06:43

by Jaegeuk Kim

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

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 <[email protected]>
> > > > > ---
> > > > > 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
> > > > .
> > > >
> > .
> >

2021-04-27 02:44:27

by Chao Yu

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

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 <[email protected]>
>>>>>> ---
>>>>>> 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
>>>>> .
>>>>>
>>> .
>>>
> .
>