* [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.