Re: [f2fs-dev] [PATCH v3 2/3] f2fs: inline: remove redundant FI_DATA_EXIST set

[Jaegeuk Kim <jaegeuk@kernel.org>]

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