[f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Fengnan Chang]

Since cluster is basic unit of compression, one cluster is compressed or
not, so we can calculate valid blocks only for first page in cluster, the
other pages just skip.

Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
 fs/f2fs/data.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index d2cf48c5a2e4..a0099d8329f0 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode *inode,
 				if (ret)
 					goto set_error_page;
 			}
-			ret = f2fs_is_compressed_cluster(inode, page->index);
-			if (ret < 0)
-				goto set_error_page;
-			else if (!ret)
-				goto read_single_page;
-
+			if (cc.cluster_idx == NULL_CLUSTER) {
+				ret = f2fs_is_compressed_cluster(inode, page->index);
+				if (ret < 0)
+					goto set_error_page;
+				else if (!ret)
+					goto read_single_page;
+			}
 			ret = f2fs_init_compress_ctx(&cc);
 			if (ret)
 				goto set_error_page;
-- 
2.29.0



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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Chao Yu]

On 2021/7/22 11:25, Fengnan Chang wrote:
> Since cluster is basic unit of compression, one cluster is compressed or
> not, so we can calculate valid blocks only for first page in cluster, the
> other pages just skip.
> 
> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
>   fs/f2fs/data.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index d2cf48c5a2e4..a0099d8329f0 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode *inode,
>   				if (ret)
>   					goto set_error_page;
>   			}
> -			ret = f2fs_is_compressed_cluster(inode, page->index);
> -			if (ret < 0)
> -				goto set_error_page;
> -			else if (!ret)
> -				goto read_single_page;

How about truncation races with read?

Thanks,

> -
> +			if (cc.cluster_idx == NULL_CLUSTER) {
> +				ret = f2fs_is_compressed_cluster(inode, page->index);
> +				if (ret < 0)
> +					goto set_error_page;
> +				else if (!ret)
> +					goto read_single_page;
> +			}
>   			ret = f2fs_init_compress_ctx(&cc);
>   			if (ret)
>   				goto set_error_page;
> 


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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Fengnan Chang]

f2fs_read_multi_pages will handle，all truncate page will be zero out, 
Whether partial or all page in cluster.


On 2021/7/22 21:47, Chao Yu wrote:
> On 2021/7/22 11:25, Fengnan Chang wrote:
>> Since cluster is basic unit of compression, one cluster is compressed or
>> not, so we can calculate valid blocks only for first page in cluster, the
>> other pages just skip.
>>
>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>> ---
>>   fs/f2fs/data.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index d2cf48c5a2e4..a0099d8329f0 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode 
>> *inode,
>>                   if (ret)
>>                       goto set_error_page;
>>               }
>> -            ret = f2fs_is_compressed_cluster(inode, page->index);
>> -            if (ret < 0)
>> -                goto set_error_page;
>> -            else if (!ret)
>> -                goto read_single_page;
> 
> How about truncation races with read?
> 
> Thanks,
> 
>> -
>> +            if (cc.cluster_idx == NULL_CLUSTER) {
>> +                ret = f2fs_is_compressed_cluster(inode, page->index);
>> +                if (ret < 0)
>> +                    goto set_error_page;
>> +                else if (!ret)
>> +                    goto read_single_page;
>> +            }
>>               ret = f2fs_init_compress_ctx(&cc);
>>               if (ret)
>>                   goto set_error_page;
>>
> 


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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Chao Yu]

On 2021/7/23 11:18, Fengnan Chang wrote:
> f2fs_read_multi_pages will handle，all truncate page will be zero out,
> Whether partial or all page in cluster.
> 
> 
> On 2021/7/22 21:47, Chao Yu wrote:
>> On 2021/7/22 11:25, Fengnan Chang wrote:
>>> Since cluster is basic unit of compression, one cluster is compressed or
>>> not, so we can calculate valid blocks only for first page in cluster, the
>>> other pages just skip.
>>>
>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>> ---
>>>    fs/f2fs/data.c | 13 +++++++------
>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index d2cf48c5a2e4..a0099d8329f0 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode
>>> *inode,
>>>                    if (ret)
>>>                        goto set_error_page;
>>>                }
>>> -            ret = f2fs_is_compressed_cluster(inode, page->index);
>>> -            if (ret < 0)
>>> -                goto set_error_page;
>>> -            else if (!ret)
>>> -                goto read_single_page;
>>
>> How about truncation races with read?

Look into this again, it looks fine, truncation tries to grab all pages lock
of cluster, but readahead has already held some/all of them, so there is no
such race condition.

So compressed cluster case looks fine to me, but we still need to call
f2fs_is_compressed_cluster() every time for non-compressed cluster, could
you please check that as well?

Thanks,

>>
>> Thanks,
>>
>>> -
>>> +            if (cc.cluster_idx == NULL_CLUSTER) {
>>> +                ret = f2fs_is_compressed_cluster(inode, page->index);
>>> +                if (ret < 0)
>>> +                    goto set_error_page;
>>> +                else if (!ret)
>>> +                    goto read_single_page;
>>> +            }
>>>                ret = f2fs_init_compress_ctx(&cc);
>>>                if (ret)
>>>                    goto set_error_page;
>>>
>>


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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Fengnan Chang]

I'll check this later.

Thanks.

On 2021/8/6 8:57, Chao Yu wrote:
> On 2021/7/23 11:18, Fengnan Chang wrote:
>> f2fs_read_multi_pages will handle，all truncate page will be zero out,
>> Whether partial or all page in cluster.
>>
>>
>> On 2021/7/22 21:47, Chao Yu wrote:
>>> On 2021/7/22 11:25, Fengnan Chang wrote:
>>>> Since cluster is basic unit of compression, one cluster is 
>>>> compressed or
>>>> not, so we can calculate valid blocks only for first page in 
>>>> cluster, the
>>>> other pages just skip.
>>>>
>>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>>> ---
>>>>    fs/f2fs/data.c | 13 +++++++------
>>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index d2cf48c5a2e4..a0099d8329f0 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode
>>>> *inode,
>>>>                    if (ret)
>>>>                        goto set_error_page;
>>>>                }
>>>> -            ret = f2fs_is_compressed_cluster(inode, page->index);
>>>> -            if (ret < 0)
>>>> -                goto set_error_page;
>>>> -            else if (!ret)
>>>> -                goto read_single_page;
>>>
>>> How about truncation races with read?
> 
> Look into this again, it looks fine, truncation tries to grab all pages 
> lock
> of cluster, but readahead has already held some/all of them, so there is no
> such race condition.
> 
> So compressed cluster case looks fine to me, but we still need to call
> f2fs_is_compressed_cluster() every time for non-compressed cluster, could
> you please check that as well?
> 
> Thanks,
> 
>>>
>>> Thanks,
>>>
>>>> -
>>>> +            if (cc.cluster_idx == NULL_CLUSTER) {
>>>> +                ret = f2fs_is_compressed_cluster(inode, page->index);
>>>> +                if (ret < 0)
>>>> +                    goto set_error_page;
>>>> +                else if (!ret)
>>>> +                    goto read_single_page;
>>>> +            }
>>>>                ret = f2fs_init_compress_ctx(&cc);
>>>>                if (ret)
>>>>                    goto set_error_page;
>>>>
>>>
> 


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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Fengnan Chang]

Hi chao:
    Since cc.cluster_idx only will be set in f2fs_compress_ctx_add_page, 
so for non-compressed cluster, cc.cluster_idx should always be NULL. it 
means that the handling process of non-compressed cluster is same as older.

On 2021/8/6 8:57, Chao Yu wrote:
> On 2021/7/23 11:18, Fengnan Chang wrote:
>> f2fs_read_multi_pages will handle，all truncate page will be zero out,
>> Whether partial or all page in cluster.
>>
>>
>> On 2021/7/22 21:47, Chao Yu wrote:
>>> On 2021/7/22 11:25, Fengnan Chang wrote:
>>>> Since cluster is basic unit of compression, one cluster is 
>>>> compressed or
>>>> not, so we can calculate valid blocks only for first page in 
>>>> cluster, the
>>>> other pages just skip.
>>>>
>>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>>> ---
>>>>    fs/f2fs/data.c | 13 +++++++------
>>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index d2cf48c5a2e4..a0099d8329f0 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode
>>>> *inode,
>>>>                    if (ret)
>>>>                        goto set_error_page;
>>>>                }
>>>> -            ret = f2fs_is_compressed_cluster(inode, page->index);
>>>> -            if (ret < 0)
>>>> -                goto set_error_page;
>>>> -            else if (!ret)
>>>> -                goto read_single_page;
>>>
>>> How about truncation races with read?
> 
> Look into this again, it looks fine, truncation tries to grab all pages 
> lock
> of cluster, but readahead has already held some/all of them, so there is no
> such race condition.
> 
> So compressed cluster case looks fine to me, but we still need to call
> f2fs_is_compressed_cluster() every time for non-compressed cluster, could
> you please check that as well?
> 
> Thanks,
> 
>>>
>>> Thanks,
>>>
>>>> -
>>>> +            if (cc.cluster_idx == NULL_CLUSTER) {
>>>> +                ret = f2fs_is_compressed_cluster(inode, page->index);
>>>> +                if (ret < 0)
>>>> +                    goto set_error_page;
>>>> +                else if (!ret)
>>>> +                    goto read_single_page;
>>>> +            }
>>>>                ret = f2fs_init_compress_ctx(&cc);
>>>>                if (ret)
>>>>                    goto set_error_page;
>>>>
>>>
> 


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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Chao Yu]

On 2021/8/9 11:46, Fengnan Chang wrote:
> Hi chao:
>      Since cc.cluster_idx only will be set in f2fs_compress_ctx_add_page,
> so for non-compressed cluster, cc.cluster_idx should always be NULL. it
> means that the handling process of non-compressed cluster is same as older.

Yup, so what I mean is why not skipping f2fs_is_compressed_cluster() check
for non-compressed cluster as we did for compressed cluster.

Thanks,

> 
> On 2021/8/6 8:57, Chao Yu wrote:
>> On 2021/7/23 11:18, Fengnan Chang wrote:
>>> f2fs_read_multi_pages will handle，all truncate page will be zero out,
>>> Whether partial or all page in cluster.
>>>
>>>
>>> On 2021/7/22 21:47, Chao Yu wrote:
>>>> On 2021/7/22 11:25, Fengnan Chang wrote:
>>>>> Since cluster is basic unit of compression, one cluster is
>>>>> compressed or
>>>>> not, so we can calculate valid blocks only for first page in
>>>>> cluster, the
>>>>> other pages just skip.
>>>>>
>>>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>>>> ---
>>>>>     fs/f2fs/data.c | 13 +++++++------
>>>>>     1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index d2cf48c5a2e4..a0099d8329f0 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode
>>>>> *inode,
>>>>>                     if (ret)
>>>>>                         goto set_error_page;
>>>>>                 }
>>>>> -            ret = f2fs_is_compressed_cluster(inode, page->index);
>>>>> -            if (ret < 0)
>>>>> -                goto set_error_page;
>>>>> -            else if (!ret)
>>>>> -                goto read_single_page;
>>>>
>>>> How about truncation races with read?
>>
>> Look into this again, it looks fine, truncation tries to grab all pages
>> lock
>> of cluster, but readahead has already held some/all of them, so there is no
>> such race condition.
>>
>> So compressed cluster case looks fine to me, but we still need to call
>> f2fs_is_compressed_cluster() every time for non-compressed cluster, could
>> you please check that as well?
>>
>> Thanks,
>>
>>>>
>>>> Thanks,
>>>>
>>>>> -
>>>>> +            if (cc.cluster_idx == NULL_CLUSTER) {
>>>>> +                ret = f2fs_is_compressed_cluster(inode, page->index);
>>>>> +                if (ret < 0)
>>>>> +                    goto set_error_page;
>>>>> +                else if (!ret)
>>>>> +                    goto read_single_page;
>>>>> +            }
>>>>>                 ret = f2fs_init_compress_ctx(&cc);
>>>>>                 if (ret)
>>>>>                     goto set_error_page;
>>>>>
>>>>
>>


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

Re: [f2fs-dev] [RFC PATCH] f2fs: compress: avoid duplicate counting of valid blocks when read compressed file

[Fengnan Chang]

Um, in previous thought, consider of random read for non-compressed
cluster, I didn't handle non-compressed cluster, after your remind, I 
think we can skip f2fs_is_compressed_cluster() check for sequential read 
for non-compressed cluster.

On 2021/8/9 22:38, Chao Yu wrote:
> On 2021/8/9 11:46, Fengnan Chang wrote:
>> Hi chao:
>>      Since cc.cluster_idx only will be set in f2fs_compress_ctx_add_page,
>> so for non-compressed cluster, cc.cluster_idx should always be NULL. it
>> means that the handling process of non-compressed cluster is same as 
>> older.
> 
> Yup, so what I mean is why not skipping f2fs_is_compressed_cluster() check
> for non-compressed cluster as we did for compressed cluster.
> 
> Thanks,
> 
>>
>> On 2021/8/6 8:57, Chao Yu wrote:
>>> On 2021/7/23 11:18, Fengnan Chang wrote:
>>>> f2fs_read_multi_pages will handle，all truncate page will be zero out,
>>>> Whether partial or all page in cluster.
>>>>
>>>>
>>>> On 2021/7/22 21:47, Chao Yu wrote:
>>>>> On 2021/7/22 11:25, Fengnan Chang wrote:
>>>>>> Since cluster is basic unit of compression, one cluster is
>>>>>> compressed or
>>>>>> not, so we can calculate valid blocks only for first page in
>>>>>> cluster, the
>>>>>> other pages just skip.
>>>>>>
>>>>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>>>>> ---
>>>>>>     fs/f2fs/data.c | 13 +++++++------
>>>>>>     1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>>> index d2cf48c5a2e4..a0099d8329f0 100644
>>>>>> --- a/fs/f2fs/data.c
>>>>>> +++ b/fs/f2fs/data.c
>>>>>> @@ -2304,12 +2304,13 @@ static int f2fs_mpage_readpages(struct inode
>>>>>> *inode,
>>>>>>                     if (ret)
>>>>>>                         goto set_error_page;
>>>>>>                 }
>>>>>> -            ret = f2fs_is_compressed_cluster(inode, page->index);
>>>>>> -            if (ret < 0)
>>>>>> -                goto set_error_page;
>>>>>> -            else if (!ret)
>>>>>> -                goto read_single_page;
>>>>>
>>>>> How about truncation races with read?
>>>
>>> Look into this again, it looks fine, truncation tries to grab all pages
>>> lock
>>> of cluster, but readahead has already held some/all of them, so there 
>>> is no
>>> such race condition.
>>>
>>> So compressed cluster case looks fine to me, but we still need to call
>>> f2fs_is_compressed_cluster() every time for non-compressed cluster, 
>>> could
>>> you please check that as well?
>>>
>>> Thanks,
>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>> -
>>>>>> +            if (cc.cluster_idx == NULL_CLUSTER) {
>>>>>> +                ret = f2fs_is_compressed_cluster(inode, 
>>>>>> page->index);
>>>>>> +                if (ret < 0)
>>>>>> +                    goto set_error_page;
>>>>>> +                else if (!ret)
>>>>>> +                    goto read_single_page;
>>>>>> +            }
>>>>>>                 ret = f2fs_init_compress_ctx(&cc);
>>>>>>                 if (ret)
>>>>>>                     goto set_error_page;
>>>>>>
>>>>>
>>>
> 


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