On 09/02, Chao Yu wrote:
> On 2019/9/1 15:25, Jaegeuk Kim wrote:
> > On 08/31, Chao Yu wrote:
> >> On 2019/8/30 23:34, Jaegeuk Kim wrote:
> >>> This can guarantee inline_data has smaller i_size.
> >>
> >> So I guess "f2fs: fix to avoid corruption during inline conversion" didn't
> >> fix
> >> such corruption right, I guess checkpoint & SPO before i_size recovery will
> >> cause this issue?
> >>
> >> err = f2fs_convert_inline_inode(inode);
> >> if (err) {
> >>
> >> -->
> >
> > Yup, I think so.
>
> Oh, we'd better to avoid wrong fixing patch as much as possible, so what
> about
> letting me change that patch to just fix error path of
> f2fs_convert_inline_page() by adding missing
> f2fs_truncate_data_blocks_range()?
Could you post another patch? I'm okay to combine them.
>
> Meanwhile I need to add below 'Fixes' tag line:
> 7735730d39d7 ("f2fs: fix to propagate error from __get_meta_page()")
>
>
> And if possible, could you add:
>
> In below call path, if we failed to convert inline inode, inline inode may
> have
> wrong i_size which is larger than max inline size.
> - f2fs_setattr
> - truncate_setsize
> - f2fs_convert_inline_inode
>
> Fixes: 0cab80ee0c9e ("f2fs: fix to convert inline inode in ->setattr")
>
> >
> >>
> >> /* recover old i_size */
> >> i_size_write(inode, old_size);
> >> return err;
> >>
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaeg...@kernel.org>
> >>
> >> Reviewed-by: Chao Yu <yuch...@huawei.com>
> >>
> >>> ---
> >>> fs/f2fs/file.c | 25 +++++++++----------------
> >>> 1 file changed, 9 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>> index 08caaead6f16..a43193dd27cb 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -815,14 +815,20 @@ int f2fs_setattr(struct dentry *dentry, struct
> >>> iattr *attr)
> >>>
> >>> if (attr->ia_valid & ATTR_SIZE) {
> >>> loff_t old_size = i_size_read(inode);
> >>> - bool to_smaller = (attr->ia_size <= old_size);
> >>> +
> >>> + if (attr->ia_size > MAX_INLINE_DATA(inode)) {
> >>> + /* should convert inline inode here */
> >>
> >> Would it be better:
> >>
> >> /* should convert inline inode here in piror to i_size_write to avoid
> >> inconsistent status in between inline flag and i_size */
> >
> > Put like this.
> >
> > + /*
> > + * should convert inline inode before i_size_write
> > to
> > + * keep smaller than inline_data size with inline
> > flag.
> > + */
>
> Better, :)
>
> thanks,
>
> >
> >>
> >> Thanks,
> >>
> >>> + err = f2fs_convert_inline_inode(inode);
> >>> + if (err)
> >>> + return err;
> >>> + }
> >>>
> >>> down_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>> down_write(&F2FS_I(inode)->i_mmap_sem);
> >>>
> >>> truncate_setsize(inode, attr->ia_size);
> >>>
> >>> - if (to_smaller)
> >>> + if (attr->ia_size <= old_size)
> >>> err = f2fs_truncate(inode);
> >>> /*
> >>> * do not trim all blocks after i_size if target size is
> >>> @@ -830,24 +836,11 @@ int f2fs_setattr(struct dentry *dentry, struct
> >>> iattr *attr)
> >>> */
> >>> up_write(&F2FS_I(inode)->i_mmap_sem);
> >>> up_write(&F2FS_I(inode)->i_gc_rwsem[WRITE]);
> >>> -
> >>> if (err)
> >>> return err;
> >>>
> >>> - if (!to_smaller) {
> >>> - /* should convert inline inode here */
> >>> - if (!f2fs_may_inline_data(inode)) {
> >>> - err = f2fs_convert_inline_inode(inode);
> >>> - if (err) {
> >>> - /* recover old i_size */
> >>> - i_size_write(inode, old_size);
> >>> - return err;
> >>> - }
> >>> - }
> >>> - inode->i_mtime = inode->i_ctime = current_time(inode);
> >>> - }
> >>> -
> >>> down_write(&F2FS_I(inode)->i_sem);
> >>> + inode->i_mtime = inode->i_ctime = current_time(inode);
> >>> F2FS_I(inode)->last_disk_size = i_size_read(inode);
> >>> up_write(&F2FS_I(inode)->i_sem);
> >>> }
> >>>
> > .
> >
