[PATCH] quota: fix wrong condition in is_quota_modification()

[PATCH] quota: fix wrong condition in is_quota_modification()

[Chao Yu]

Quoted from
commit 3da40c7b0898 ("ext4: only call ext4_truncate when size <= isize")

" At LSF we decided that if we truncate up from isize we shouldn't trim
  fallocated blocks that were fallocated with KEEP_SIZE and are past the
 new i_size.  This patch fixes ext4 to do this. "

And generic/092 of fstest have covered this case for long time, however
is_quota_modification() didn't adjust based on that rule, so that in
below condition, we will lose to quota block change:
- fallocate blocks beyond EOF
- remount
- truncate(file_path, file_size)

Fix it.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 include/linux/quotaops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
index dc905a4ff8d7..bd30acad3a7f 100644
--- a/include/linux/quotaops.h
+++ b/include/linux/quotaops.h
@@ -22,7 +22,7 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
 /* i_mutex must being held */
 static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
 {
-	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
+	return (ia->ia_valid & ATTR_SIZE && ia->ia_size <= inode->i_size) ||
 		(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
 		(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
 }
-- 
2.18.0.rc1

Re: [PATCH] quota: fix wrong condition in is_quota_modification()

[Jan Kara]

On Wed 11-09-19 17:36:50, Chao Yu wrote:
> Quoted from
> commit 3da40c7b0898 ("ext4: only call ext4_truncate when size <= isize")
> 
> " At LSF we decided that if we truncate up from isize we shouldn't trim
>   fallocated blocks that were fallocated with KEEP_SIZE and are past the
>  new i_size.  This patch fixes ext4 to do this. "
> 
> And generic/092 of fstest have covered this case for long time, however
> is_quota_modification() didn't adjust based on that rule, so that in
> below condition, we will lose to quota block change:
> - fallocate blocks beyond EOF
> - remount
> - truncate(file_path, file_size)
> 
> Fix it.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  include/linux/quotaops.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> index dc905a4ff8d7..bd30acad3a7f 100644
> --- a/include/linux/quotaops.h
> +++ b/include/linux/quotaops.h
> @@ -22,7 +22,7 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
>  /* i_mutex must being held */
>  static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
>  {
> -	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
> +	return (ia->ia_valid & ATTR_SIZE && ia->ia_size <= inode->i_size) ||
>  		(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
>  		(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
>  }

OK, but your change makes i_size extension not to be quota modification
which is IMO wrong. So I think the condition should just be:

	return (ia->ia_valid & ATTR_SIZE) || ...

I'll fix the patch up and pull it into my tree.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] quota: fix wrong condition in is_quota_modification()

[Chao Yu]

On 2019/9/12 18:06, Jan Kara wrote:
> On Wed 11-09-19 17:36:50, Chao Yu wrote:
>> Quoted from
>> commit 3da40c7b0898 ("ext4: only call ext4_truncate when size <= isize")
>>
>> " At LSF we decided that if we truncate up from isize we shouldn't trim
>>   fallocated blocks that were fallocated with KEEP_SIZE and are past the
>>  new i_size.  This patch fixes ext4 to do this. "
>>
>> And generic/092 of fstest have covered this case for long time, however
>> is_quota_modification() didn't adjust based on that rule, so that in
>> below condition, we will lose to quota block change:
>> - fallocate blocks beyond EOF
>> - remount
>> - truncate(file_path, file_size)
>>
>> Fix it.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  include/linux/quotaops.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
>> index dc905a4ff8d7..bd30acad3a7f 100644
>> --- a/include/linux/quotaops.h
>> +++ b/include/linux/quotaops.h
>> @@ -22,7 +22,7 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
>>  /* i_mutex must being held */
>>  static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
>>  {
>> -	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
>> +	return (ia->ia_valid & ATTR_SIZE && ia->ia_size <= inode->i_size) ||
>>  		(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
>>  		(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
>>  }
> 
> OK, but your change makes i_size extension not to be quota modification

I just try to adapt below rules covered with generic/092, which restrict to not
trim preallocate blocks beyond i_size, in that case, filesystem won't change
i_blocks.

1) truncate(i_size) will trim all blocks past i_size.
2) truncate(x) where x > i_size will not trim all blocks past i_size.

However, I'm okay with your change, because there could be filesystems won't
follow above rule.

Thanks,

> which is IMO wrong. So I think the condition should just be:
> 
> 	return (ia->ia_valid & ATTR_SIZE) || ...
> 
> I'll fix the patch up and pull it into my tree.
> 
> 									Honza
> 

Re: [PATCH] quota: fix wrong condition in is_quota_modification()

[Jan Kara]

On Mon 16-09-19 10:53:08, Chao Yu wrote:
> On 2019/9/12 18:06, Jan Kara wrote:
> > On Wed 11-09-19 17:36:50, Chao Yu wrote:
> >> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
> >> index dc905a4ff8d7..bd30acad3a7f 100644
> >> --- a/include/linux/quotaops.h
> >> +++ b/include/linux/quotaops.h
> >> @@ -22,7 +22,7 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
> >>  /* i_mutex must being held */
> >>  static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
> >>  {
> >> -	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
> >> +	return (ia->ia_valid & ATTR_SIZE && ia->ia_size <= inode->i_size) ||
> >>  		(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
> >>  		(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
> >>  }
> > 
> > OK, but your change makes i_size extension not to be quota modification
> 
> I just try to adapt below rules covered with generic/092, which restrict
> to not trim preallocate blocks beyond i_size, in that case, filesystem
> won't change i_blocks.
> 
> 1) truncate(i_size) will trim all blocks past i_size.
> 2) truncate(x) where x > i_size will not trim all blocks past i_size.

Ah, OK.

> However, I'm okay with your change, because there could be filesystems won't
> follow above rule.

Yes, I'm concerned that some filesystem may change i_blocks in some corner
case when growing inode size (e.g. when it decides to convert inode from
inline format to a normal block based format or something like that). So I
don't think the optimization is really worth the chance for breakage.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] quota: fix wrong condition in is_quota_modification()

[Chao Yu]

On 2019/9/16 16:23, Jan Kara wrote:
> On Mon 16-09-19 10:53:08, Chao Yu wrote:
>> On 2019/9/12 18:06, Jan Kara wrote:
>>> On Wed 11-09-19 17:36:50, Chao Yu wrote:
>>>> diff --git a/include/linux/quotaops.h b/include/linux/quotaops.h
>>>> index dc905a4ff8d7..bd30acad3a7f 100644
>>>> --- a/include/linux/quotaops.h
>>>> +++ b/include/linux/quotaops.h
>>>> @@ -22,7 +22,7 @@ static inline struct quota_info *sb_dqopt(struct super_block *sb)
>>>>  /* i_mutex must being held */
>>>>  static inline bool is_quota_modification(struct inode *inode, struct iattr *ia)
>>>>  {
>>>> -	return (ia->ia_valid & ATTR_SIZE && ia->ia_size != inode->i_size) ||
>>>> +	return (ia->ia_valid & ATTR_SIZE && ia->ia_size <= inode->i_size) ||
>>>>  		(ia->ia_valid & ATTR_UID && !uid_eq(ia->ia_uid, inode->i_uid)) ||
>>>>  		(ia->ia_valid & ATTR_GID && !gid_eq(ia->ia_gid, inode->i_gid));
>>>>  }
>>>
>>> OK, but your change makes i_size extension not to be quota modification
>>
>> I just try to adapt below rules covered with generic/092, which restrict
>> to not trim preallocate blocks beyond i_size, in that case, filesystem
>> won't change i_blocks.
>>
>> 1) truncate(i_size) will trim all blocks past i_size.
>> 2) truncate(x) where x > i_size will not trim all blocks past i_size.
> 
> Ah, OK.
> 
>> However, I'm okay with your change, because there could be filesystems won't
>> follow above rule.
> 
> Yes, I'm concerned that some filesystem may change i_blocks in some corner
> case when growing inode size (e.g. when it decides to convert inode from
> inline format to a normal block based format or something like that). So I
> don't think the optimization is really worth the chance for breakage.

Agreed, :)

Thanks,

> 
> 								Honza
>