[f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Jaegeuk Kim]

This can guarantee inline_data has smaller i_size.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 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 */
+			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);
 	}
-- 
2.19.0.605.g01d371f741-goog



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Chao Yu]

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) {

-->

		/* recover old i_size */
		i_size_write(inode, old_size);
		return err;

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@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 */

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);
>  	}
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Jaegeuk Kim]

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.

> 
> 		/* recover old i_size */
> 		i_size_write(inode, old_size);
> 		return err;
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> 
> Reviewed-by: Chao Yu <yuchao0@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.
+                        */

> 
> 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);
> >  	}
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Chao Yu]

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()?

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 <jaegeuk@kernel.org>
>>
>> Reviewed-by: Chao Yu <yuchao0@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);
>>>  	}
>>>
> .
> 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Jaegeuk Kim]

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 <jaegeuk@kernel.org>
> >>
> >> Reviewed-by: Chao Yu <yuchao0@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);
> >>>  	}
> >>>
> > .
> > 


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Chao Yu]

On 2019-9-3 7:07, Jaegeuk Kim wrote:
> 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.

No problem, let merge them in v2. :)

Thanks,

> 
>>
>> 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 <jaegeuk@kernel.org>
>>>>
>>>> Reviewed-by: Chao Yu <yuchao0@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);
>>>>>  	}
>>>>>
>>> .
>>>
> 
> 
> _______________________________________________
> 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

Re: [f2fs-dev] [PATCH] f2fs: convert inline_data in prior to i_size_write

[Chao Yu]

On 2019/9/3 7:35, Chao Yu wrote:
> On 2019-9-3 7:07, Jaegeuk Kim wrote:
>> 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.
> 
> No problem, let merge them in v2. :)

Still send as two separated patches which can be easily maintained in those LTS
stable kernel. :P

Thanks,

> 
> Thanks,
> 
>>
>>>
>>> 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 <jaegeuk@kernel.org>
>>>>>
>>>>> Reviewed-by: Chao Yu <yuchao0@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);
>>>>>>  	}
>>>>>>
>>>> .
>>>>
>>
>>
>> _______________________________________________
>> 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