* [f2fs-dev] [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
@ 2021-05-07 9:44 Fengnan Chang
2021-05-10 15:43 ` Jaegeuk Kim
0 siblings, 1 reply; 11+ messages in thread
From: Fengnan Chang @ 2021-05-07 9:44 UTC (permalink / raw)
To: jaegeuk, chao, linux-f2fs-devel; +Cc: Fengnan Chang
when write compressed file with O_TRUNC, there will be a lot of
unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
especially when written in page size, remove it.
This patch will not bring significant performance improvements, I test
this on mobile phone, use androbench, the sequential write test case was
open file with O_TRUNC, set write size to 4KB, performance improved
about 2%-3%. If write size set to 32MB, performance improved about 0.5%.
Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
---
fs/f2fs/data.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index cf935474ffba..b9ec7b182f45 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
#ifdef CONFIG_F2FS_FS_COMPRESSION
if (f2fs_compressed_file(inode)) {
int ret;
+ pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
*fsdata = NULL;
+ /*
+ * when write pos is bigger than inode size ,f2fs_prepare_compress_overwrite
+ * always return 0, so check pos first to avoid this.
+ */
+ if (index >= end)
+ goto repeat;
+
ret = f2fs_prepare_compress_overwrite(inode, pagep,
index, fsdata);
if (ret < 0) {
--
2.29.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-07 9:44 [f2fs-dev] [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite Fengnan Chang
@ 2021-05-10 15:43 ` Jaegeuk Kim
2021-05-11 11:10 ` [f2fs-dev] 答复: " changfengnan
0 siblings, 1 reply; 11+ messages in thread
From: Jaegeuk Kim @ 2021-05-10 15:43 UTC (permalink / raw)
To: Fengnan Chang; +Cc: linux-f2fs-devel
On 05/07, Fengnan Chang wrote:
> when write compressed file with O_TRUNC, there will be a lot of
> unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
> especially when written in page size, remove it.
>
> This patch will not bring significant performance improvements, I test
> this on mobile phone, use androbench, the sequential write test case was
> open file with O_TRUNC, set write size to 4KB, performance improved
> about 2%-3%. If write size set to 32MB, performance improved about 0.5%.
>
> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
> fs/f2fs/data.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index cf935474ffba..b9ec7b182f45 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> #ifdef CONFIG_F2FS_FS_COMPRESSION
> if (f2fs_compressed_file(inode)) {
> int ret;
> + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
>
> *fsdata = NULL;
>
> + /*
> + * when write pos is bigger than inode size ,f2fs_prepare_compress_overwrite
> + * always return 0, so check pos first to avoid this.
> + */
> + if (index >= end)
> + goto repeat;
What if there're existing clusters beyond i_size? Given performance impacts,
do we really need this?
> +
> ret = f2fs_prepare_compress_overwrite(inode, pagep,
> index, fsdata);
> if (ret < 0) {
> --
> 2.29.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-10 15:43 ` Jaegeuk Kim
@ 2021-05-11 11:10 ` changfengnan
2021-05-11 21:50 ` Jaegeuk Kim
0 siblings, 1 reply; 11+ messages in thread
From: changfengnan @ 2021-05-11 11:10 UTC (permalink / raw)
To: 'Jaegeuk Kim'; +Cc: linux-f2fs-devel
Hi Jaegeuk:
If there're existing clusters beyond i_size, may cause data corruption, but
will this happen in normal? maybe some error can cause this, if i_size is
error the data beyond size still can't handle properly. Is there normal
case can casue existing clusters beyond i_size?
Thanks.
-----邮件原件-----
发件人: Jaegeuk Kim <jaegeuk@kernel.org>
发送时间: 2021年5月10日 23:44
收件人: Fengnan Chang <changfengnan@vivo.com>
抄送: chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
主题: Re: [PATCH v4] f2fs: compress: avoid unnecessary check in
f2fs_prepare_compress_overwrite
On 05/07, Fengnan Chang wrote:
> when write compressed file with O_TRUNC, there will be a lot of
> unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
> especially when written in page size, remove it.
>
> This patch will not bring significant performance improvements, I test
> this on mobile phone, use androbench, the sequential write test case
> was open file with O_TRUNC, set write size to 4KB, performance
> improved about 2%-3%. If write size set to 32MB, performance improved
about 0.5%.
>
> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
> fs/f2fs/data.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> cf935474ffba..b9ec7b182f45 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file,
> struct address_space *mapping, #ifdef CONFIG_F2FS_FS_COMPRESSION
> if (f2fs_compressed_file(inode)) {
> int ret;
> + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >>
PAGE_SHIFT;
>
> *fsdata = NULL;
>
> + /*
> + * when write pos is bigger than inode size
,f2fs_prepare_compress_overwrite
> + * always return 0, so check pos first to avoid this.
> + */
> + if (index >= end)
> + goto repeat;
What if there're existing clusters beyond i_size? Given performance impacts,
do we really need this?
> +
> ret = f2fs_prepare_compress_overwrite(inode, pagep,
> index, fsdata);
> if (ret < 0) {
> --
> 2.29.0
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-11 11:10 ` [f2fs-dev] 答复: " changfengnan
@ 2021-05-11 21:50 ` Jaegeuk Kim
2021-05-12 1:20 ` Gao Xiang
2021-05-12 1:52 ` Chao Yu
0 siblings, 2 replies; 11+ messages in thread
From: Jaegeuk Kim @ 2021-05-11 21:50 UTC (permalink / raw)
To: changfengnan; +Cc: linux-f2fs-devel
On 05/11, changfengnan@vivo.com wrote:
> Hi Jaegeuk:
>
> If there're existing clusters beyond i_size, may cause data corruption, but
> will this happen in normal? maybe some error can cause this, if i_size is
> error the data beyond size still can't handle properly. Is there normal
> case can casue existing clusters beyond i_size?
We don't have a rule to sync between i_size and i_blocks.
>
> Thanks.
>
> -----邮件原件-----
> 发件人: Jaegeuk Kim <jaegeuk@kernel.org>
> 发送时间: 2021年5月10日 23:44
> 收件人: Fengnan Chang <changfengnan@vivo.com>
> 抄送: chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
> 主题: Re: [PATCH v4] f2fs: compress: avoid unnecessary check in
> f2fs_prepare_compress_overwrite
>
> On 05/07, Fengnan Chang wrote:
> > when write compressed file with O_TRUNC, there will be a lot of
> > unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
> > especially when written in page size, remove it.
> >
> > This patch will not bring significant performance improvements, I test
> > this on mobile phone, use androbench, the sequential write test case
> > was open file with O_TRUNC, set write size to 4KB, performance
> > improved about 2%-3%. If write size set to 32MB, performance improved
> about 0.5%.
> >
> > Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> > ---
> > fs/f2fs/data.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> > cf935474ffba..b9ec7b182f45 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file,
> > struct address_space *mapping, #ifdef CONFIG_F2FS_FS_COMPRESSION
> > if (f2fs_compressed_file(inode)) {
> > int ret;
> > + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >>
> PAGE_SHIFT;
> >
> > *fsdata = NULL;
> >
> > + /*
> > + * when write pos is bigger than inode size
> ,f2fs_prepare_compress_overwrite
> > + * always return 0, so check pos first to avoid this.
> > + */
> > + if (index >= end)
> > + goto repeat;
>
> What if there're existing clusters beyond i_size? Given performance impacts,
> do we really need this?
>
> > +
> > ret = f2fs_prepare_compress_overwrite(inode, pagep,
> > index, fsdata);
> > if (ret < 0) {
> > --
> > 2.29.0
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-11 21:50 ` Jaegeuk Kim
@ 2021-05-12 1:20 ` Gao Xiang
2021-05-12 1:52 ` Chao Yu
1 sibling, 0 replies; 11+ messages in thread
From: Gao Xiang @ 2021-05-12 1:20 UTC (permalink / raw)
To: changfengnan; +Cc: Jaegeuk Kim, linux-f2fs-devel
On Tue, May 11, 2021 at 02:50:54PM -0700, Jaegeuk Kim wrote:
> On 05/11, changfengnan@vivo.com wrote:
> > Hi Jaegeuk:
> >
> > If there're existing clusters beyond i_size, may cause data corruption, but
> > will this happen in normal? maybe some error can cause this, if i_size is
> > error the data beyond size still can't handle properly. Is there normal
> > case can casue existing clusters beyond i_size?
>
> We don't have a rule to sync between i_size and i_blocks.
Hmmm.. Again, it's still unclear what's the on-disk format like when
post-EOF. Also, corrupted/crafted/malicious on-disk data needs to be
handled at least to make sure it cannot crash the kernel and corrupt
the fs itself even further, especially some optimization patch like
this targeted on the specific logic to challenge the stablility.
So without details, in the beginning, it smelled somewhat dangerous
to me anyway. But considering some performance impact, I just leave
some message here.
Thanks,
Gao Xiang
>
> >
> > Thanks.
> >
> > -----邮件原件-----
> > 发件人: Jaegeuk Kim <jaegeuk@kernel.org>
> > 发送时间: 2021年5月10日 23:44
> > 收件人: Fengnan Chang <changfengnan@vivo.com>
> > 抄送: chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
> > 主题: Re: [PATCH v4] f2fs: compress: avoid unnecessary check in
> > f2fs_prepare_compress_overwrite
> >
> > On 05/07, Fengnan Chang wrote:
> > > when write compressed file with O_TRUNC, there will be a lot of
> > > unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
> > > especially when written in page size, remove it.
> > >
> > > This patch will not bring significant performance improvements, I test
> > > this on mobile phone, use androbench, the sequential write test case
> > > was open file with O_TRUNC, set write size to 4KB, performance
> > > improved about 2%-3%. If write size set to 32MB, performance improved
> > about 0.5%.
> > >
> > > Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> > > ---
> > > fs/f2fs/data.c | 8 ++++++++
> > > 1 file changed, 8 insertions(+)
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
> > > cf935474ffba..b9ec7b182f45 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file,
> > > struct address_space *mapping, #ifdef CONFIG_F2FS_FS_COMPRESSION
> > > if (f2fs_compressed_file(inode)) {
> > > int ret;
> > > + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >>
> > PAGE_SHIFT;
> > >
> > > *fsdata = NULL;
> > >
> > > + /*
> > > + * when write pos is bigger than inode size
> > ,f2fs_prepare_compress_overwrite
> > > + * always return 0, so check pos first to avoid this.
> > > + */
> > > + if (index >= end)
> > > + goto repeat;
> >
> > What if there're existing clusters beyond i_size? Given performance impacts,
> > do we really need this?
> >
> > > +
> > > ret = f2fs_prepare_compress_overwrite(inode, pagep,
> > > index, fsdata);
> > > if (ret < 0) {
> > > --
> > > 2.29.0
> >
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-11 21:50 ` Jaegeuk Kim
2021-05-12 1:20 ` Gao Xiang
@ 2021-05-12 1:52 ` Chao Yu
2021-05-13 1:41 ` [f2fs-dev] 答复: " changfengnan
2021-05-13 21:17 ` [f2fs-dev] " Eric Biggers
1 sibling, 2 replies; 11+ messages in thread
From: Chao Yu @ 2021-05-12 1:52 UTC (permalink / raw)
To: Jaegeuk Kim, changfengnan; +Cc: linux-f2fs-devel
On 2021/5/12 5:50, Jaegeuk Kim wrote:
> On 05/11, changfengnan@vivo.com wrote:
>> Hi Jaegeuk:
>>
>> If there're existing clusters beyond i_size, may cause data corruption, but
>> will this happen in normal? maybe some error can cause this, if i_size is
>> error the data beyond size still can't handle properly. Is there normal
>> case can casue existing clusters beyond i_size?
>
> We don't have a rule to sync between i_size and i_blocks.
I can't image a case that compressed cluster may cross filesize, it looks it's
a bug if that happened, but I'm not sure I have considered all cases. So, I
prefer to add a check condition as below, then testing w/ xfstest/por_fsstress
for a while.
Subject: [PATCH] f2fs: compress: compressed cluster should not cross i_size
---
fs/f2fs/data.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 06d1e58d3882..9acca358d578 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3325,6 +3325,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
err = ret;
goto fail;
} else if (ret) {
+ f2fs_bug_on(sbi, index >=
+ DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
return 0;
}
}
--
2.29.2
>
>>
>> Thanks.
>>
>> -----邮件原件-----
>> 发件人: Jaegeuk Kim <jaegeuk@kernel.org>
>> 发送时间: 2021年5月10日 23:44
>> 收件人: Fengnan Chang <changfengnan@vivo.com>
>> 抄送: chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
>> 主题: Re: [PATCH v4] f2fs: compress: avoid unnecessary check in
>> f2fs_prepare_compress_overwrite
>>
>> On 05/07, Fengnan Chang wrote:
>>> when write compressed file with O_TRUNC, there will be a lot of
>>> unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
>>> especially when written in page size, remove it.
>>>
>>> This patch will not bring significant performance improvements, I test
>>> this on mobile phone, use androbench, the sequential write test case
>>> was open file with O_TRUNC, set write size to 4KB, performance
>>> improved about 2%-3%. If write size set to 32MB, performance improved
>> about 0.5%.
>>>
>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>> ---
>>> fs/f2fs/data.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>>> cf935474ffba..b9ec7b182f45 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file,
>>> struct address_space *mapping, #ifdef CONFIG_F2FS_FS_COMPRESSION
>>> if (f2fs_compressed_file(inode)) {
>>> int ret;
>>> + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >>
>> PAGE_SHIFT;
>>>
>>> *fsdata = NULL;
>>>
>>> + /*
>>> + * when write pos is bigger than inode size
>> ,f2fs_prepare_compress_overwrite
>>> + * always return 0, so check pos first to avoid this.
>>> + */
>>> + if (index >= end)
>>> + goto repeat;
>>
>> What if there're existing clusters beyond i_size? Given performance impacts,
>> do we really need this?
>>
>>> +
>>> ret = f2fs_prepare_compress_overwrite(inode, pagep,
>>> index, fsdata);
>>> if (ret < 0) {
>>> --
>>> 2.29.0
>>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [f2fs-dev] 答复: 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-12 1:52 ` Chao Yu
@ 2021-05-13 1:41 ` changfengnan
2021-05-13 21:17 ` [f2fs-dev] " Eric Biggers
1 sibling, 0 replies; 11+ messages in thread
From: changfengnan @ 2021-05-13 1:41 UTC (permalink / raw)
To: 'Chao Yu', 'Jaegeuk Kim'; +Cc: linux-f2fs-devel
I agree with this plan, I will also test this patch also.
Thanks
-----邮件原件-----
发件人: Chao Yu <yuchao0@huawei.com>
发送时间: 2021年5月12日 9:52
收件人: Jaegeuk Kim <jaegeuk@kernel.org>; changfengnan@vivo.com
抄送: linux-f2fs-devel@lists.sourceforge.net
主题: Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
On 2021/5/12 5:50, Jaegeuk Kim wrote:
> On 05/11, changfengnan@vivo.com wrote:
>> Hi Jaegeuk:
>>
>> If there're existing clusters beyond i_size, may cause data
>> corruption, but will this happen in normal? maybe some error can
>> cause this, if i_size is error the data beyond size still can't
>> handle properly. Is there normal case can casue existing clusters beyond i_size?
>
> We don't have a rule to sync between i_size and i_blocks.
I can't image a case that compressed cluster may cross filesize, it looks it's a bug if that happened, but I'm not sure I have considered all cases. So, I prefer to add a check condition as below, then testing w/ xfstest/por_fsstress for a while.
Subject: [PATCH] f2fs: compress: compressed cluster should not cross i_size
---
fs/f2fs/data.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 06d1e58d3882..9acca358d578 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -3325,6 +3325,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
err = ret;
goto fail;
} else if (ret) {
+ f2fs_bug_on(sbi, index >=
+ DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
return 0;
}
}
--
2.29.2
>
>>
>> Thanks.
>>
>> -----邮件原件-----
>> 发件人: Jaegeuk Kim <jaegeuk@kernel.org>
>> 发送时间: 2021年5月10日 23:44
>> 收件人: Fengnan Chang <changfengnan@vivo.com>
>> 抄送: chao@kernel.org; linux-f2fs-devel@lists.sourceforge.net
>> 主题: Re: [PATCH v4] f2fs: compress: avoid unnecessary check in
>> f2fs_prepare_compress_overwrite
>>
>> On 05/07, Fengnan Chang wrote:
>>> when write compressed file with O_TRUNC, there will be a lot of
>>> unnecessary check valid blocks in f2fs_prepare_compress_overwrite,
>>> especially when written in page size, remove it.
>>>
>>> This patch will not bring significant performance improvements, I test
>>> this on mobile phone, use androbench, the sequential write test case
>>> was open file with O_TRUNC, set write size to 4KB, performance
>>> improved about 2%-3%. If write size set to 32MB, performance improved
>> about 0.5%.
>>>
>>> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
>>> ---
>>> fs/f2fs/data.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index
>>> cf935474ffba..b9ec7b182f45 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -3303,9 +3303,17 @@ static int f2fs_write_begin(struct file *file,
>>> struct address_space *mapping, #ifdef CONFIG_F2FS_FS_COMPRESSION
>>> if (f2fs_compressed_file(inode)) {
>>> int ret;
>>> + pgoff_t end = (i_size_read(inode) + PAGE_SIZE - 1) >>
>> PAGE_SHIFT;
>>>
>>> *fsdata = NULL;
>>>
>>> + /*
>>> + * when write pos is bigger than inode size
>> ,f2fs_prepare_compress_overwrite
>>> + * always return 0, so check pos first to avoid this.
>>> + */
>>> + if (index >= end)
>>> + goto repeat;
>>
>> What if there're existing clusters beyond i_size? Given performance impacts,
>> do we really need this?
>>
>>> +
>>> ret = f2fs_prepare_compress_overwrite(inode, pagep,
>>> index, fsdata);
>>> if (ret < 0) {
>>> --
>>> 2.29.0
>>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-12 1:52 ` Chao Yu
2021-05-13 1:41 ` [f2fs-dev] 答复: " changfengnan
@ 2021-05-13 21:17 ` Eric Biggers
2021-05-14 1:15 ` Chao Yu
1 sibling, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2021-05-13 21:17 UTC (permalink / raw)
To: Chao Yu; +Cc: Jaegeuk Kim, changfengnan, linux-f2fs-devel
On Wed, May 12, 2021 at 09:52:19AM +0800, Chao Yu wrote:
> On 2021/5/12 5:50, Jaegeuk Kim wrote:
> > On 05/11, changfengnan@vivo.com wrote:
> > > Hi Jaegeuk:
> > >
> > > If there're existing clusters beyond i_size, may cause data corruption, but
> > > will this happen in normal? maybe some error can cause this, if i_size is
> > > error the data beyond size still can't handle properly. Is there normal
> > > case can casue existing clusters beyond i_size?
> >
> > We don't have a rule to sync between i_size and i_blocks.
>
> I can't image a case that compressed cluster may cross filesize, it looks it's
> a bug if that happened, but I'm not sure I have considered all cases. So, I
> prefer to add a check condition as below, then testing w/ xfstest/por_fsstress
> for a while.
>
> Subject: [PATCH] f2fs: compress: compressed cluster should not cross i_size
>
> ---
> fs/f2fs/data.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 06d1e58d3882..9acca358d578 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -3325,6 +3325,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> err = ret;
> goto fail;
> } else if (ret) {
> + f2fs_bug_on(sbi, index >=
> + DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
> return 0;
> }
> }
If a file has both fs-verity and compression enabled, it can have compressed
clusters past i_size.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-13 21:17 ` [f2fs-dev] " Eric Biggers
@ 2021-05-14 1:15 ` Chao Yu
2021-05-17 21:18 ` Eric Biggers
0 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2021-05-14 1:15 UTC (permalink / raw)
To: Eric Biggers; +Cc: Jaegeuk Kim, changfengnan, linux-f2fs-devel
On 2021/5/14 5:17, Eric Biggers wrote:
> On Wed, May 12, 2021 at 09:52:19AM +0800, Chao Yu wrote:
>> On 2021/5/12 5:50, Jaegeuk Kim wrote:
>>> On 05/11, changfengnan@vivo.com wrote:
>>>> Hi Jaegeuk:
>>>>
>>>> If there're existing clusters beyond i_size, may cause data corruption, but
>>>> will this happen in normal? maybe some error can cause this, if i_size is
>>>> error the data beyond size still can't handle properly. Is there normal
>>>> case can casue existing clusters beyond i_size?
>>>
>>> We don't have a rule to sync between i_size and i_blocks.
>>
>> I can't image a case that compressed cluster may cross filesize, it looks it's
>> a bug if that happened, but I'm not sure I have considered all cases. So, I
>> prefer to add a check condition as below, then testing w/ xfstest/por_fsstress
>> for a while.
>>
>> Subject: [PATCH] f2fs: compress: compressed cluster should not cross i_size
>>
>> ---
>> fs/f2fs/data.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 06d1e58d3882..9acca358d578 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -3325,6 +3325,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>> err = ret;
>> goto fail;
>> } else if (ret) {
>> + f2fs_bug_on(sbi, index >=
>> + DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
>> return 0;
>> }
>> }
>
> If a file has both fs-verity and compression enabled, it can have compressed
> clusters past i_size.
Correct, any other case we missed for a writable file? let us know.
Thanks,
>
> - Eric
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-14 1:15 ` Chao Yu
@ 2021-05-17 21:18 ` Eric Biggers
2021-05-18 1:23 ` Chao Yu
0 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2021-05-17 21:18 UTC (permalink / raw)
To: Chao Yu; +Cc: Jaegeuk Kim, changfengnan, linux-f2fs-devel
On Fri, May 14, 2021 at 09:15:54AM +0800, Chao Yu wrote:
> On 2021/5/14 5:17, Eric Biggers wrote:
> > On Wed, May 12, 2021 at 09:52:19AM +0800, Chao Yu wrote:
> > > On 2021/5/12 5:50, Jaegeuk Kim wrote:
> > > > On 05/11, changfengnan@vivo.com wrote:
> > > > > Hi Jaegeuk:
> > > > >
> > > > > If there're existing clusters beyond i_size, may cause data corruption, but
> > > > > will this happen in normal? maybe some error can cause this, if i_size is
> > > > > error the data beyond size still can't handle properly. Is there normal
> > > > > case can casue existing clusters beyond i_size?
> > > >
> > > > We don't have a rule to sync between i_size and i_blocks.
> > >
> > > I can't image a case that compressed cluster may cross filesize, it looks it's
> > > a bug if that happened, but I'm not sure I have considered all cases. So, I
> > > prefer to add a check condition as below, then testing w/ xfstest/por_fsstress
> > > for a while.
> > >
> > > Subject: [PATCH] f2fs: compress: compressed cluster should not cross i_size
> > >
> > > ---
> > > fs/f2fs/data.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index 06d1e58d3882..9acca358d578 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -3325,6 +3325,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> > > err = ret;
> > > goto fail;
> > > } else if (ret) {
> > > + f2fs_bug_on(sbi, index >=
> > > + DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
> > > return 0;
> > > }
> > > }
> >
> > If a file has both fs-verity and compression enabled, it can have compressed
> > clusters past i_size.
>
> Correct, any other case we missed for a writable file? let us know.
>
> Thanks,
>
Well, files become read-only once fs-verity is enabled on them, but that happens
after the blocks past i_size are written in the first place. That part still
uses ->write_begin(), ->write_end(), ->writepages(), etc.
- Eric
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] 答复: [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite
2021-05-17 21:18 ` Eric Biggers
@ 2021-05-18 1:23 ` Chao Yu
0 siblings, 0 replies; 11+ messages in thread
From: Chao Yu @ 2021-05-18 1:23 UTC (permalink / raw)
To: Eric Biggers; +Cc: Jaegeuk Kim, changfengnan, linux-f2fs-devel
On 2021/5/18 5:18, Eric Biggers wrote:
> On Fri, May 14, 2021 at 09:15:54AM +0800, Chao Yu wrote:
>> On 2021/5/14 5:17, Eric Biggers wrote:
>>> On Wed, May 12, 2021 at 09:52:19AM +0800, Chao Yu wrote:
>>>> On 2021/5/12 5:50, Jaegeuk Kim wrote:
>>>>> On 05/11, changfengnan@vivo.com wrote:
>>>>>> Hi Jaegeuk:
>>>>>>
>>>>>> If there're existing clusters beyond i_size, may cause data corruption, but
>>>>>> will this happen in normal? maybe some error can cause this, if i_size is
>>>>>> error the data beyond size still can't handle properly. Is there normal
>>>>>> case can casue existing clusters beyond i_size?
>>>>>
>>>>> We don't have a rule to sync between i_size and i_blocks.
>>>>
>>>> I can't image a case that compressed cluster may cross filesize, it looks it's
>>>> a bug if that happened, but I'm not sure I have considered all cases. So, I
>>>> prefer to add a check condition as below, then testing w/ xfstest/por_fsstress
>>>> for a while.
>>>>
>>>> Subject: [PATCH] f2fs: compress: compressed cluster should not cross i_size
>>>>
>>>> ---
>>>> fs/f2fs/data.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>> index 06d1e58d3882..9acca358d578 100644
>>>> --- a/fs/f2fs/data.c
>>>> +++ b/fs/f2fs/data.c
>>>> @@ -3325,6 +3325,8 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>>>> err = ret;
>>>> goto fail;
>>>> } else if (ret) {
>>>> + f2fs_bug_on(sbi, index >=
>>>> + DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
>>>> return 0;
>>>> }
>>>> }
>>>
>>> If a file has both fs-verity and compression enabled, it can have compressed
>>> clusters past i_size.
>>
>> Correct, any other case we missed for a writable file? let us know.
>>
>> Thanks,
>>
>
> Well, files become read-only once fs-verity is enabled on them, but that happens
> after the blocks past i_size are written in the first place. That part still
> uses ->write_begin(), ->write_end(), ->writepages(), etc.
Yup, the scenario we concern here is there is a writable file which has compressed
cluster crossing filesize, and then it appends data starting from filesize.
Thanks,
>
> - Eric
> .
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-05-18 1:23 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 9:44 [f2fs-dev] [PATCH v4] f2fs: compress: avoid unnecessary check in f2fs_prepare_compress_overwrite Fengnan Chang
2021-05-10 15:43 ` Jaegeuk Kim
2021-05-11 11:10 ` [f2fs-dev] 答复: " changfengnan
2021-05-11 21:50 ` Jaegeuk Kim
2021-05-12 1:20 ` Gao Xiang
2021-05-12 1:52 ` Chao Yu
2021-05-13 1:41 ` [f2fs-dev] 答复: " changfengnan
2021-05-13 21:17 ` [f2fs-dev] " Eric Biggers
2021-05-14 1:15 ` Chao Yu
2021-05-17 21:18 ` Eric Biggers
2021-05-18 1:23 ` Chao Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.