* [f2fs-dev] [PATCH v3 0/3] f2fs: inline: fix minor bugs in f2fs_recover_inline_data @ 2020-12-07 12:01 Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 1/3] f2fs: inline: correct comment " Jack Qiu ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Jack Qiu @ 2020-12-07 12:01 UTC (permalink / raw) To: linux-f2fs-devel Jack Qiu (3): f2fs: inline: correct comment in f2fs_recover_inline_data f2fs: inline: remove redundant FI_DATA_EXIST set f2fs: inline: fix wrong inline inode stat fs/f2fs/inline.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.17.1 _______________________________________________ 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] 12+ messages in thread
* [f2fs-dev] [PATCH v3 1/3] f2fs: inline: correct comment in f2fs_recover_inline_data 2020-12-07 12:01 [f2fs-dev] [PATCH v3 0/3] f2fs: inline: fix minor bugs in f2fs_recover_inline_data Jack Qiu @ 2020-12-07 12:01 ` Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 3/3] f2fs: inline: fix wrong inline inode stat Jack Qiu 2 siblings, 0 replies; 12+ messages in thread From: Jack Qiu @ 2020-12-07 12:01 UTC (permalink / raw) To: linux-f2fs-devel In 3rd scene, it should remove data blocks instead of inline_data. Signed-off-by: Jack Qiu <jack.qiu@huawei.com> Reviewed-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/inline.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index 70384e31788d..0399531efcd3 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -266,7 +266,7 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) * [prev.] [next] of inline_data flag * o o -> recover inline_data * o x -> remove inline_data, and then recover data blocks - * x o -> remove inline_data, and then recover inline_data + * x o -> remove data blocks, and then recover inline_data * x x -> recover data blocks */ if (IS_INODE(npage)) -- 2.17.1 _______________________________________________ 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] 12+ messages in thread
* [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-07 12:01 [f2fs-dev] [PATCH v3 0/3] f2fs: inline: fix minor bugs in f2fs_recover_inline_data Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 1/3] f2fs: inline: correct comment " Jack Qiu @ 2020-12-07 12:01 ` Jack Qiu 2020-12-07 14:47 ` Chao Yu 2020-12-07 16:49 ` Jaegeuk Kim 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 3/3] f2fs: inline: fix wrong inline inode stat Jack Qiu 2 siblings, 2 replies; 12+ messages in thread From: Jack Qiu @ 2020-12-07 12:01 UTC (permalink / raw) To: linux-f2fs-devel FI_DATA_EXIST has been set in recover_inline_flags, no need set in f2fs_recover_inline_data again. Just remove it. Signed-off-by: Jack Qiu <jack.qiu@huawei.com> --- fs/f2fs/inline.c | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index 0399531efcd3..0a8f7eefca7d 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); set_inode_flag(inode, FI_INLINE_DATA); - set_inode_flag(inode, FI_DATA_EXIST); set_page_dirty(ipage); f2fs_put_page(ipage, 1); -- 2.17.1 _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set Jack Qiu @ 2020-12-07 14:47 ` Chao Yu 2020-12-07 16:49 ` Jaegeuk Kim 1 sibling, 0 replies; 12+ messages in thread From: Chao Yu @ 2020-12-07 14:47 UTC (permalink / raw) To: Jack Qiu, linux-f2fs-devel On 2020/12/7 20:01, Jack Qiu wrote: > FI_DATA_EXIST has been set in recover_inline_flags, no need set in > f2fs_recover_inline_data again. Just remove it. > > Signed-off-by: Jack Qiu <jack.qiu@huawei.com> Reviewed-by: Chao Yu <yuchao0@huawei.com> Thanks, _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set Jack Qiu 2020-12-07 14:47 ` Chao Yu @ 2020-12-07 16:49 ` Jaegeuk Kim 2020-12-08 1:25 ` Chao Yu 1 sibling, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-12-07 16:49 UTC (permalink / raw) To: Jack Qiu; +Cc: linux-f2fs-devel On 12/07, Jack Qiu wrote: > FI_DATA_EXIST has been set in recover_inline_flags, no need set in > f2fs_recover_inline_data again. Just remove it. > > Signed-off-by: Jack Qiu <jack.qiu@huawei.com> > --- > fs/f2fs/inline.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > index 0399531efcd3..0a8f7eefca7d 100644 > --- a/fs/f2fs/inline.c > +++ b/fs/f2fs/inline.c > @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) > memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); > > set_inode_flag(inode, FI_INLINE_DATA); > - set_inode_flag(inode, FI_DATA_EXIST); Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to recover it back. > > set_page_dirty(ipage); > f2fs_put_page(ipage, 1); > -- > 2.17.1 > > > > _______________________________________________ > 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-07 16:49 ` Jaegeuk Kim @ 2020-12-08 1:25 ` Chao Yu 2020-12-08 1:38 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-12-08 1:25 UTC (permalink / raw) To: Jaegeuk Kim, Jack Qiu; +Cc: linux-f2fs-devel On 2020/12/8 0:49, Jaegeuk Kim wrote: > On 12/07, Jack Qiu wrote: >> FI_DATA_EXIST has been set in recover_inline_flags, no need set in >> f2fs_recover_inline_data again. Just remove it. >> >> Signed-off-by: Jack Qiu <jack.qiu@huawei.com> >> --- >> fs/f2fs/inline.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c >> index 0399531efcd3..0a8f7eefca7d 100644 >> --- a/fs/f2fs/inline.c >> +++ b/fs/f2fs/inline.c >> @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) >> memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); >> >> set_inode_flag(inode, FI_INLINE_DATA); >> - set_inode_flag(inode, FI_DATA_EXIST); > > Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to > recover it back. Should this flag only be recovered with the way like __recover_inline_status()? otherwise, the data_exist status may be not be consistent with real condition. Thanks, > >> >> set_page_dirty(ipage); >> f2fs_put_page(ipage, 1); >> -- >> 2.17.1 >> >> >> >> _______________________________________________ >> 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 > . > _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-08 1:25 ` Chao Yu @ 2020-12-08 1:38 ` Jaegeuk Kim 2020-12-08 2:15 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-12-08 1:38 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel On 12/08, Chao Yu wrote: > On 2020/12/8 0:49, Jaegeuk Kim wrote: > > On 12/07, Jack Qiu wrote: > > > FI_DATA_EXIST has been set in recover_inline_flags, no need set in > > > f2fs_recover_inline_data again. Just remove it. > > > > > > Signed-off-by: Jack Qiu <jack.qiu@huawei.com> > > > --- > > > fs/f2fs/inline.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > index 0399531efcd3..0a8f7eefca7d 100644 > > > --- a/fs/f2fs/inline.c > > > +++ b/fs/f2fs/inline.c > > > @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) > > > memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); > > > > > > set_inode_flag(inode, FI_INLINE_DATA); > > > - set_inode_flag(inode, FI_DATA_EXIST); > > > > Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to > > recover it back. > > Should this flag only be recovered with the way like __recover_inline_status()? > otherwise, the data_exist status may be not be consistent with real condition. Well, we cannot say consistency on this, since user can write zero data. This can avoid __recover_inline_state() regardless of there-in data which is zero or not. > > Thanks, > > > > > > > > > set_page_dirty(ipage); > > > f2fs_put_page(ipage, 1); > > > -- > > > 2.17.1 > > > > > > > > > > > > _______________________________________________ > > > 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 > > . > > _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-08 1:38 ` Jaegeuk Kim @ 2020-12-08 2:15 ` Chao Yu 2020-12-08 7:07 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-12-08 2:15 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel On 2020/12/8 9:38, Jaegeuk Kim wrote: > On 12/08, Chao Yu wrote: >> On 2020/12/8 0:49, Jaegeuk Kim wrote: >>> On 12/07, Jack Qiu wrote: >>>> FI_DATA_EXIST has been set in recover_inline_flags, no need set in >>>> f2fs_recover_inline_data again. Just remove it. >>>> >>>> Signed-off-by: Jack Qiu <jack.qiu@huawei.com> >>>> --- >>>> fs/f2fs/inline.c | 1 - >>>> 1 file changed, 1 deletion(-) >>>> >>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c >>>> index 0399531efcd3..0a8f7eefca7d 100644 >>>> --- a/fs/f2fs/inline.c >>>> +++ b/fs/f2fs/inline.c >>>> @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) >>>> memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); >>>> >>>> set_inode_flag(inode, FI_INLINE_DATA); >>>> - set_inode_flag(inode, FI_DATA_EXIST); >>> >>> Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to >>> recover it back. >> >> Should this flag only be recovered with the way like __recover_inline_status()? >> otherwise, the data_exist status may be not be consistent with real condition. > > Well, we cannot say consistency on this, since user can write zero data. This I can see that FI_DATA_EXIST flag only decide that whether f2fs_convert_inline_page() will copy inline data into first page, if user write all zero data in inline area, there is no need to do the copy, as first page contains all zero already. So, IMO, the flag indicates more like a FI_NON_ZERO_DATA_EXIST status? that would be consistent with our implementation in __recover_inline_status(). Thanks, > can avoid __recover_inline_state() regardless of there-in data which is zero or > not. > >> >> Thanks, >> >>> >>>> >>>> set_page_dirty(ipage); >>>> f2fs_put_page(ipage, 1); >>>> -- >>>> 2.17.1 >>>> >>>> >>>> >>>> _______________________________________________ >>>> 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 >>> . >>> > . > _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-08 2:15 ` Chao Yu @ 2020-12-08 7:07 ` Jaegeuk Kim 2020-12-09 1:31 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Jaegeuk Kim @ 2020-12-08 7:07 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel On 12/08, Chao Yu wrote: > On 2020/12/8 9:38, Jaegeuk Kim wrote: > > On 12/08, Chao Yu wrote: > > > On 2020/12/8 0:49, Jaegeuk Kim wrote: > > > > On 12/07, Jack Qiu wrote: > > > > > FI_DATA_EXIST has been set in recover_inline_flags, no need set in > > > > > f2fs_recover_inline_data again. Just remove it. > > > > > > > > > > Signed-off-by: Jack Qiu <jack.qiu@huawei.com> > > > > > --- > > > > > fs/f2fs/inline.c | 1 - > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > > index 0399531efcd3..0a8f7eefca7d 100644 > > > > > --- a/fs/f2fs/inline.c > > > > > +++ b/fs/f2fs/inline.c > > > > > @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) > > > > > memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); > > > > > > > > > > set_inode_flag(inode, FI_INLINE_DATA); > > > > > - set_inode_flag(inode, FI_DATA_EXIST); > > > > > > > > Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to > > > > recover it back. > > > > > > Should this flag only be recovered with the way like __recover_inline_status()? > > > otherwise, the data_exist status may be not be consistent with real condition. > > > > Well, we cannot say consistency on this, since user can write zero data. This > > I can see that FI_DATA_EXIST flag only decide that whether f2fs_convert_inline_page() > will copy inline data into first page, if user write all zero data in inline area, > there is no need to do the copy, as first page contains all zero already. > > So, IMO, the flag indicates more like a FI_NON_ZERO_DATA_EXIST status? that would be > consistent with our implementation in __recover_inline_status(). IIRC, original intention is FI_DATA_EXIST, and non-zero case was added to avoid potential inconsistency case. So, it's not intened for FI_NON_ZERO_DATA_EXIST. Still I don't see any problem on this, but looking at the iteration overhead on do_read_inode() -> __recover_inlint_status(). > > Thanks, > > > can avoid __recover_inline_state() regardless of there-in data which is zero or > > not. > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > set_page_dirty(ipage); > > > > > f2fs_put_page(ipage, 1); > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > 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 > > > > . > > > > > > . > > _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-08 7:07 ` Jaegeuk Kim @ 2020-12-09 1:31 ` Chao Yu 2020-12-09 2:51 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2020-12-09 1:31 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-f2fs-devel On 2020/12/8 15:07, Jaegeuk Kim wrote: > On 12/08, Chao Yu wrote: >> On 2020/12/8 9:38, Jaegeuk Kim wrote: >>> On 12/08, Chao Yu wrote: >>>> On 2020/12/8 0:49, Jaegeuk Kim wrote: >>>>> On 12/07, Jack Qiu wrote: >>>>>> FI_DATA_EXIST has been set in recover_inline_flags, no need set in >>>>>> f2fs_recover_inline_data again. Just remove it. >>>>>> >>>>>> Signed-off-by: Jack Qiu <jack.qiu@huawei.com> >>>>>> --- >>>>>> fs/f2fs/inline.c | 1 - >>>>>> 1 file changed, 1 deletion(-) >>>>>> >>>>>> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c >>>>>> index 0399531efcd3..0a8f7eefca7d 100644 >>>>>> --- a/fs/f2fs/inline.c >>>>>> +++ b/fs/f2fs/inline.c >>>>>> @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) >>>>>> memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); >>>>>> >>>>>> set_inode_flag(inode, FI_INLINE_DATA); >>>>>> - set_inode_flag(inode, FI_DATA_EXIST); >>>>> >>>>> Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to >>>>> recover it back. >>>> >>>> Should this flag only be recovered with the way like __recover_inline_status()? >>>> otherwise, the data_exist status may be not be consistent with real condition. >>> >>> Well, we cannot say consistency on this, since user can write zero data. This >> >> I can see that FI_DATA_EXIST flag only decide that whether f2fs_convert_inline_page() >> will copy inline data into first page, if user write all zero data in inline area, >> there is no need to do the copy, as first page contains all zero already. >> >> So, IMO, the flag indicates more like a FI_NON_ZERO_DATA_EXIST status? that would be >> consistent with our implementation in __recover_inline_status(). > > IIRC, original intention is FI_DATA_EXIST, and non-zero case was added to avoid > potential inconsistency case. So, it's not intened for FI_NON_ZERO_DATA_EXIST. Why we can encounter such inconsistency? is there any bug before adding that patch? > Still I don't see any problem on this, but looking at the iteration overhead on > do_read_inode() -> __recover_inlint_status(). Yup, it's trivial, I'm fine to keep it as it is. Thanks, > >> >> Thanks, >> >>> can avoid __recover_inline_state() regardless of there-in data which is zero or >>> not. >>> >>>> >>>> Thanks, >>>> >>>>> >>>>>> >>>>>> set_page_dirty(ipage); >>>>>> f2fs_put_page(ipage, 1); >>>>>> -- >>>>>> 2.17.1 >>>>>> >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> 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 >>>>> . >>>>> >>> . >>> > . > _______________________________________________ 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] 12+ messages in thread
* Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set 2020-12-09 1:31 ` Chao Yu @ 2020-12-09 2:51 ` Jaegeuk Kim 0 siblings, 0 replies; 12+ messages in thread From: Jaegeuk Kim @ 2020-12-09 2:51 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel On 12/09, Chao Yu wrote: > On 2020/12/8 15:07, Jaegeuk Kim wrote: > > On 12/08, Chao Yu wrote: > > > On 2020/12/8 9:38, Jaegeuk Kim wrote: > > > > On 12/08, Chao Yu wrote: > > > > > On 2020/12/8 0:49, Jaegeuk Kim wrote: > > > > > > On 12/07, Jack Qiu wrote: > > > > > > > FI_DATA_EXIST has been set in recover_inline_flags, no need set in > > > > > > > f2fs_recover_inline_data again. Just remove it. > > > > > > > > > > > > > > Signed-off-by: Jack Qiu <jack.qiu@huawei.com> > > > > > > > --- > > > > > > > fs/f2fs/inline.c | 1 - > > > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > > > > > diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c > > > > > > > index 0399531efcd3..0a8f7eefca7d 100644 > > > > > > > --- a/fs/f2fs/inline.c > > > > > > > +++ b/fs/f2fs/inline.c > > > > > > > @@ -286,7 +286,6 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) > > > > > > > memcpy(dst_addr, src_addr, MAX_INLINE_DATA(inode)); > > > > > > > > > > > > > > set_inode_flag(inode, FI_INLINE_DATA); > > > > > > > - set_inode_flag(inode, FI_DATA_EXIST); > > > > > > > > > > > > Wait, recover_inline_flags() sets this based on on-disk flag, but this tries to > > > > > > recover it back. > > > > > > > > > > Should this flag only be recovered with the way like __recover_inline_status()? > > > > > otherwise, the data_exist status may be not be consistent with real condition. > > > > > > > > Well, we cannot say consistency on this, since user can write zero data. This > > > > > > I can see that FI_DATA_EXIST flag only decide that whether f2fs_convert_inline_page() > > > will copy inline data into first page, if user write all zero data in inline area, > > > there is no need to do the copy, as first page contains all zero already. > > > > > > So, IMO, the flag indicates more like a FI_NON_ZERO_DATA_EXIST status? that would be > > > consistent with our implementation in __recover_inline_status(). > > > > IIRC, original intention is FI_DATA_EXIST, and non-zero case was added to avoid > > potential inconsistency case. So, it's not intened for FI_NON_ZERO_DATA_EXIST. > > Why we can encounter such inconsistency? is there any bug before adding that patch? I can't remember exactly on it. > > > Still I don't see any problem on this, but looking at the iteration overhead on > > do_read_inode() -> __recover_inlint_status(). > > Yup, it's trivial, I'm fine to keep it as it is. > > Thanks, > > > > > > > > > Thanks, > > > > > > > can avoid __recover_inline_state() regardless of there-in data which is zero or > > > > not. > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > > set_page_dirty(ipage); > > > > > > > f2fs_put_page(ipage, 1); > > > > > > > -- > > > > > > > 2.17.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > > 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 > > > > > > . > > > > > > > > > > . > > > > > > . > > _______________________________________________ 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] 12+ messages in thread
* [f2fs-dev] [PATCH v3 3/3] f2fs: inline: fix wrong inline inode stat 2020-12-07 12:01 [f2fs-dev] [PATCH v3 0/3] f2fs: inline: fix minor bugs in f2fs_recover_inline_data Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 1/3] f2fs: inline: correct comment " Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set Jack Qiu @ 2020-12-07 12:01 ` Jack Qiu 2 siblings, 0 replies; 12+ messages in thread From: Jack Qiu @ 2020-12-07 12:01 UTC (permalink / raw) To: linux-f2fs-devel Miss to stat inline inode in f2fs_recover_inline_data. Signed-off-by: Jack Qiu <jack.qiu@huawei.com> Reviewed-by: Chao Yu <yuchao0@huawei.com> --- fs/f2fs/inline.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c index 0a8f7eefca7d..c37747f7028c 100644 --- a/fs/f2fs/inline.c +++ b/fs/f2fs/inline.c @@ -297,6 +297,7 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) if (IS_ERR(ipage)) return PTR_ERR(ipage); f2fs_truncate_inline_inode(inode, ipage, 0); + stat_dec_inline_inode(inode); clear_inode_flag(inode, FI_INLINE_DATA); f2fs_put_page(ipage, 1); } else if (ri && (ri->i_inline & F2FS_INLINE_DATA)) { @@ -305,6 +306,7 @@ int f2fs_recover_inline_data(struct inode *inode, struct page *npage) ret = f2fs_truncate_blocks(inode, 0, false); if (ret) return ret; + stat_inc_inline_inode(inode); goto process_inline; } return 0; -- 2.17.1 _______________________________________________ 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] 12+ messages in thread
end of thread, other threads:[~2020-12-09 2:51 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-07 12:01 [f2fs-dev] [PATCH v3 0/3] f2fs: inline: fix minor bugs in f2fs_recover_inline_data Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 1/3] f2fs: inline: correct comment " Jack Qiu 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set Jack Qiu 2020-12-07 14:47 ` Chao Yu 2020-12-07 16:49 ` Jaegeuk Kim 2020-12-08 1:25 ` Chao Yu 2020-12-08 1:38 ` Jaegeuk Kim 2020-12-08 2:15 ` Chao Yu 2020-12-08 7:07 ` Jaegeuk Kim 2020-12-09 1:31 ` Chao Yu 2020-12-09 2:51 ` Jaegeuk Kim 2020-12-07 12:01 ` [f2fs-dev] [PATCH v3 3/3] f2fs: inline: fix wrong inline inode stat Jack Qiu
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.