* [PATCH] quota: fix wrong condition in is_quota_modification()
@ 2019-09-11 9:36 Chao Yu
2019-09-12 10:06 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu @ 2019-09-11 9:36 UTC (permalink / raw)
To: jack; +Cc: linux-kernel, linux-fsdevel, chao, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] quota: fix wrong condition in is_quota_modification()
2019-09-11 9:36 [PATCH] quota: fix wrong condition in is_quota_modification() Chao Yu
@ 2019-09-12 10:06 ` Jan Kara
2019-09-16 2:53 ` Chao Yu
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2019-09-12 10:06 UTC (permalink / raw)
To: Chao Yu; +Cc: Jan Kara, chao, linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] quota: fix wrong condition in is_quota_modification()
2019-09-12 10:06 ` Jan Kara
@ 2019-09-16 2:53 ` Chao Yu
2019-09-16 8:23 ` Jan Kara
0 siblings, 1 reply; 5+ messages in thread
From: Chao Yu @ 2019-09-16 2:53 UTC (permalink / raw)
To: Jan Kara; +Cc: Jan Kara, chao, linux-fsdevel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] quota: fix wrong condition in is_quota_modification()
2019-09-16 2:53 ` Chao Yu
@ 2019-09-16 8:23 ` Jan Kara
2019-09-17 1:44 ` Chao Yu
0 siblings, 1 reply; 5+ messages in thread
From: Jan Kara @ 2019-09-16 8:23 UTC (permalink / raw)
To: Chao Yu; +Cc: Jan Kara, Jan Kara, chao, linux-fsdevel, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] quota: fix wrong condition in is_quota_modification()
2019-09-16 8:23 ` Jan Kara
@ 2019-09-17 1:44 ` Chao Yu
0 siblings, 0 replies; 5+ messages in thread
From: Chao Yu @ 2019-09-17 1:44 UTC (permalink / raw)
To: Jan Kara; +Cc: Jan Kara, chao, linux-fsdevel, linux-kernel
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-17 1:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 9:36 [PATCH] quota: fix wrong condition in is_quota_modification() Chao Yu
2019-09-12 10:06 ` Jan Kara
2019-09-16 2:53 ` Chao Yu
2019-09-16 8:23 ` Jan Kara
2019-09-17 1:44 ` Chao Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).