[PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Roman Anufriev]

Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
ext4_statfs() output incorrect (function does not apply quota limits
on used/available space, etc) when called on dentry of regular file
with project quota enabled.

This patch fixes this by comparing inode's i_projid with
EXT4_DEF_PROJID, as there is no point in calling ext4_statfs_project()
for inode with default project id.

$ sudo project_quota info dir/
project   2147516417
usage     4096
limit     5242880
inodes    4
ilimit    0
$ sudo project_quota info dir/file | grep project
project   2147516417
$ df -h /dev/loop0
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0      232M  2.1M  214M   1% /mnt/ext4img

without patch:
$ df -h dir/
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0      5.0M  4.0K  5.0M   1% /mnt/ext4img
$ df -h dir/file
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0      232M  2.1M  214M   1% /mnt/ext4img

with patch:
$ df -h dir/
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0      5.0M  4.0K  5.0M   1% /mnt/ext4img
$ df -h dir/file
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0      5.0M  4.0K  5.0M   1% /mnt/ext4img

Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
---
 fs/ext4/super.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 79b6a0c..682d675 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6074,6 +6074,7 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf)
 	struct super_block *sb = dentry->d_sb;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_super_block *es = sbi->s_es;
+	kprojid_t kprojid;
 	ext4_fsblk_t overhead = 0, resv_blocks;
 	s64 bfree;
 	resv_blocks = EXT4_C2B(sbi, atomic64_read(&sbi->s_resv_clusters));
@@ -6098,9 +6099,10 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_fsid = uuid_to_fsid(es->s_uuid);
 
 #ifdef CONFIG_QUOTA
-	if (ext4_test_inode_flag(dentry->d_inode, EXT4_INODE_PROJINHERIT) &&
+	kprojid = EXT4_I(dentry->d_inode)->i_projid;
+	if ((from_kprojid(current_user_ns(), kprojid) != EXT4_DEF_PROJID) &&
 	    sb_has_quota_limits_enabled(sb, PRJQUOTA))
-		ext4_statfs_project(sb, EXT4_I(dentry->d_inode)->i_projid, buf);
+		ext4_statfs_project(sb, kprojid, buf);
 #endif
 	return 0;
 }
-- 
2.7.4

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Roman Anufriev]

On Tue, 7 Dec 2021, Roman Anufriev wrote:

> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
> ext4_statfs() output incorrect (function does not apply quota limits
> on used/available space, etc) when called on dentry of regular file
> with project quota enabled.
>
> This patch fixes this by comparing inode's i_projid with
> EXT4_DEF_PROJID, as there is no point in calling ext4_statfs_project()
> for inode with default project id.
>
> $ sudo project_quota info dir/
> project   2147516417
> usage     4096
> limit     5242880
> inodes    4
> ilimit    0
> $ sudo project_quota info dir/file | grep project
> project   2147516417
> $ df -h /dev/loop0
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0      232M  2.1M  214M   1% /mnt/ext4img
>
> without patch:
> $ df -h dir/
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0      5.0M  4.0K  5.0M   1% /mnt/ext4img
> $ df -h dir/file
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0      232M  2.1M  214M   1% /mnt/ext4img
>
> with patch:
> $ df -h dir/
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0      5.0M  4.0K  5.0M   1% /mnt/ext4img
> $ df -h dir/file
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0      5.0M  4.0K  5.0M   1% /mnt/ext4img
>
> Signed-off-by: Roman Anufriev <dotdot@yandex-team.ru>
> ---
> fs/ext4/super.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 79b6a0c..682d675 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6074,6 +6074,7 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf)
> 	struct super_block *sb = dentry->d_sb;
> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
> 	struct ext4_super_block *es = sbi->s_es;
> +	kprojid_t kprojid;
> 	ext4_fsblk_t overhead = 0, resv_blocks;
> 	s64 bfree;
> 	resv_blocks = EXT4_C2B(sbi, atomic64_read(&sbi->s_resv_clusters));
> @@ -6098,9 +6099,10 @@ static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf)
> 	buf->f_fsid = uuid_to_fsid(es->s_uuid);
>
> #ifdef CONFIG_QUOTA
> -	if (ext4_test_inode_flag(dentry->d_inode, EXT4_INODE_PROJINHERIT) &&
> +	kprojid = EXT4_I(dentry->d_inode)->i_projid;
> +	if ((from_kprojid(current_user_ns(), kprojid) != EXT4_DEF_PROJID) &&
> 	    sb_has_quota_limits_enabled(sb, PRJQUOTA))
> -		ext4_statfs_project(sb, EXT4_I(dentry->d_inode)->i_projid, buf);
> +		ext4_statfs_project(sb, kprojid, buf);
> #endif
> 	return 0;
> }
> -- 
> 2.7.4
>
>

+Cc Wang Shilong <wshilong@ddn.com> author of 7ddf79a10395

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Theodore Y. Ts'o]

On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
> > Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
> > removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
> > ext4_statfs() output incorrect (function does not apply quota limits
> > on used/available space, etc) when called on dentry of regular file
> > with project quota enabled.

Under what circumstance is userspace trying to call statfs on a file
descriptor?

Removing the test for EXT4_INODE_PROJINHERIT will cause
incorrect/misleading results being returned in the case where we have
a directory where a directory hierarchy is using project id's, but
which is *not* using PROJINHERIT.

				- Ted

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On Dec 7, 2021, at 12:34 PM, Theodore Y. Ts'o <tytso@MIT.EDU> wrote:
> 
> On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
>>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
>>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
>>> ext4_statfs() output incorrect (function does not apply quota limits
>>> on used/available space, etc) when called on dentry of regular file
>>> with project quota enabled.
> 
> Under what circumstance is userspace trying to call statfs on a file
> descriptor?

Who knows what users do?  Calling statfs() on a regular file works fine
(returns stats for the filesystem), so I don't see why it wouldn't be
consistent when calling statfs() on a file with projid set?

Darrick, how does XFS handle this case?  I think it makes sense to be
consistent with that implementation, since that was the main reason to
remove PROJINHERIT from regular files in the first place.

> Removing the test for EXT4_INODE_PROJINHERIT will cause
> incorrect/misleading results being returned in the case where we have
> a directory where a directory hierarchy is using project id's, but
> which is *not* using PROJINHERIT.

One alternative would be to check the PROJINHERIT status of the parent
directory after calling statfs() on the regular file?  That should
keep the semantics for PROJINHERIT the same, but avoid inconsistent
results if called on a regular file:

#ifdef CONFIG_QUOTA
-	if (ext4_test_inode_flag(dentry->d_inode, EXT4_INODE_PROJINHERIT) &&
+	if (ext4_test_inode_flag(S_ISDIR(dentry->d_inode) ? dentry->d_inode :
+			   dentry->d_parent->d_inode, EXT4_INODE_PROJINHERIT) &&
	    sb_has_quota_limits_enabled(sb, PRJQUOTA))
		ext4_statfs_project(sb, EXT4_I(dentry->d_inode)->i_projid, buf);
#endif

Roman, does that work for you?

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Dave Chinner]

On Thu, Dec 09, 2021 at 03:53:55PM -0700, Andreas Dilger wrote:
> On Dec 7, 2021, at 12:34 PM, Theodore Y. Ts'o <tytso@MIT.EDU> wrote:
> > 
> > On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
> >>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
> >>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
> >>> ext4_statfs() output incorrect (function does not apply quota limits
> >>> on used/available space, etc) when called on dentry of regular file
> >>> with project quota enabled.
> > 
> > Under what circumstance is userspace trying to call statfs on a file
> > descriptor?
> 
> Who knows what users do?  Calling statfs() on a regular file works fine
> (returns stats for the filesystem), so I don't see why it wouldn't be
> consistent when calling statfs() on a file with projid set?
> 
> Darrick, how does XFS handle this case?  I think it makes sense to be
> consistent with that implementation, since that was the main reason to
> remove PROJINHERIT from regular files in the first place.

If PROJINHERIT is set on the inode, it will return the information
for the projid on that inode. XFS doesn't care what type of inode it
is, just whether the PROJINHERIT flag is set.

That said, on XFS, only directory inodes will have the PROJINHERIT
flag set. So, in effect, only statfs() on directory inodes can
report project quota limits.

PROJINHERIT just indicates the default projid that an inode is
created with; it does not mean that directory tree quotas are what
the user it doing with them...

> > Removing the test for EXT4_INODE_PROJINHERIT will cause
> > incorrect/misleading results being returned in the case where we have
> > a directory where a directory hierarchy is using project id's, but
> > which is *not* using PROJINHERIT.
> 
> One alternative would be to check the PROJINHERIT status of the parent
> directory after calling statfs() on the regular file?  That should
> keep the semantics for PROJINHERIT the same, but avoid inconsistent
> results if called on a regular file:

This just opens a bigger can of worms that still has no consistent
solution.

What if the user has changed the projid of the file and it doesn't
match the parent directory? That then reports something irrelevant
to the user.

What if there are hard links and the parent directories have
different projid state? This can happen - we don't allow hard links
into a new projid controlled directory, but we allow them into
non-projid controlled directories even if the source is from a
projid controlled heirarchy. We can add PROJINHERIT after a
directory has already been populated. We can remove PROJINHERIT,
too, after hardlinks within the same projid have been created. Hence
a regular file inode can have different parent PROJINHERIT depending
on path.  How do you do consistency then, because it's clearly not a
directory quota controlled setup and there's no way of detecting
that from statfs() context?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Roman Anufriev]

On Fri, 10 Dec 2021, Dave Chinner wrote:

> On Thu, Dec 09, 2021 at 03:53:55PM -0700, Andreas Dilger wrote:
>> On Dec 7, 2021, at 12:34 PM, Theodore Y. Ts'o <tytso@MIT.EDU> wrote:
>>>
>>> On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
>>>>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
>>>>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
>>>>> ext4_statfs() output incorrect (function does not apply quota limits
>>>>> on used/available space, etc) when called on dentry of regular file
>>>>> with project quota enabled.
>>>
>>> Under what circumstance is userspace trying to call statfs on a file
>>> descriptor?
>>
>> Who knows what users do?  Calling statfs() on a regular file works fine
>> (returns stats for the filesystem), so I don't see why it wouldn't be
>> consistent when calling statfs() on a file with projid set?
>>
>> Darrick, how does XFS handle this case?  I think it makes sense to be
>> consistent with that implementation, since that was the main reason to
>> remove PROJINHERIT from regular files in the first place.
>
> If PROJINHERIT is set on the inode, it will return the information
> for the projid on that inode. XFS doesn't care what type of inode it
> is, just whether the PROJINHERIT flag is set.
>
> That said, on XFS, only directory inodes will have the PROJINHERIT
> flag set. So, in effect, only statfs() on directory inodes can
> report project quota limits.

This is the thing that confused our users. It basically means that user
program should always trim paths up to directories to get true available
space, etc.

> PROJINHERIT just indicates the default projid that an inode is
> created with; it does not mean that directory tree quotas are what
> the user it doing with them...
>
>>> Removing the test for EXT4_INODE_PROJINHERIT will cause
>>> incorrect/misleading results being returned in the case where we have
>>> a directory where a directory hierarchy is using project id's, but
>>> which is *not* using PROJINHERIT.
>>
>> One alternative would be to check the PROJINHERIT status of the parent
>> directory after calling statfs() on the regular file?  That should
>> keep the semantics for PROJINHERIT the same, but avoid inconsistent
>> results if called on a regular file:
>
> This just opens a bigger can of worms that still has no consistent
> solution.
>
> What if the user has changed the projid of the file and it doesn't
> match the parent directory? That then reports something irrelevant
> to the user.
>
> What if there are hard links and the parent directories have
> different projid state? This can happen - we don't allow hard links
> into a new projid controlled directory, but we allow them into
> non-projid controlled directories even if the source is from a
> projid controlled heirarchy. We can add PROJINHERIT after a
> directory has already been populated. We can remove PROJINHERIT,
> too, after hardlinks within the same projid have been created. Hence
> a regular file inode can have different parent PROJINHERIT depending
> on path.  How do you do consistency then, because it's clearly not a
> directory quota controlled setup and there's no way of detecting
> that from statfs() context?

I think that part of these concerns are solved by the fact that we
check PROJINHERIT on parent directory, but use our own dentry/inode for
all calculations later (e.g. non-matching project ids of parent
directory and file is not an issue - statfs() will produce right output).
So, this approach is kinda useful for simple cases.

 								Roman

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Roman Anufriev]

On Thu, 9 Dec 2021, Andreas Dilger wrote:

> On Dec 7, 2021, at 12:34 PM, Theodore Y. Ts'o <tytso@MIT.EDU> wrote:
>>
>> On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
>>>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
>>>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
>>>> ext4_statfs() output incorrect (function does not apply quota limits
>>>> on used/available space, etc) when called on dentry of regular file
>>>> with project quota enabled.
>>
>> Under what circumstance is userspace trying to call statfs on a file
>> descriptor?
>
> Who knows what users do?  Calling statfs() on a regular file works fine
> (returns stats for the filesystem), so I don't see why it wouldn't be
> consistent when calling statfs() on a file with projid set?

This is exactly my reasoning for this patch.

> Darrick, how does XFS handle this case?  I think it makes sense to be
> consistent with that implementation, since that was the main reason to
> remove PROJINHERIT from regular files in the first place.
>
>> Removing the test for EXT4_INODE_PROJINHERIT will cause
>> incorrect/misleading results being returned in the case where we have
>> a directory where a directory hierarchy is using project id's, but
>> which is *not* using PROJINHERIT.
>
> One alternative would be to check the PROJINHERIT status of the parent
> directory after calling statfs() on the regular file?  That should
> keep the semantics for PROJINHERIT the same, but avoid inconsistent
> results if called on a regular file:
>
> #ifdef CONFIG_QUOTA
> -	if (ext4_test_inode_flag(dentry->d_inode, EXT4_INODE_PROJINHERIT) &&
> +	if (ext4_test_inode_flag(S_ISDIR(dentry->d_inode) ? dentry->d_inode :
> +			   dentry->d_parent->d_inode, EXT4_INODE_PROJINHERIT) &&
> 	    sb_has_quota_limits_enabled(sb, PRJQUOTA))
> 		ext4_statfs_project(sb, EXT4_I(dentry->d_inode)->i_projid, buf);
> #endif
>
> Roman, does that work for you?

Yes, it was actually the first thing that came to my mind. But later I
realised, that there may be some pitfalls and it would probably make more
sense to check inode's own project id and report stats based on that. As
I thought that we check presense of EXT4_INODE_PROJINHERIT flag only to
make sure that this inode belongs to some project.

 								Roman

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Darrick J. Wong]

On Thu, Dec 09, 2021 at 03:53:55PM -0700, Andreas Dilger wrote:
> On Dec 7, 2021, at 12:34 PM, Theodore Y. Ts'o <tytso@MIT.EDU> wrote:
> > 
> > On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
> >>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
> >>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
> >>> ext4_statfs() output incorrect (function does not apply quota limits
> >>> on used/available space, etc) when called on dentry of regular file
> >>> with project quota enabled.
> > 
> > Under what circumstance is userspace trying to call statfs on a file
> > descriptor?
> 
> Who knows what users do?  Calling statfs() on a regular file works fine
> (returns stats for the filesystem), so I don't see why it wouldn't be
> consistent when calling statfs() on a file with projid set?
> 
> Darrick, how does XFS handle this case?  I think it makes sense to be
> consistent with that implementation, since that was the main reason to
> remove PROJINHERIT from regular files in the first place.

As far as I can tell, the existing ext4 implementation handles this
exactly the same that XFS does.  I would leave this alone on the grounds
that we don't really want inconsistent behavior.

--D

> 
> > Removing the test for EXT4_INODE_PROJINHERIT will cause
> > incorrect/misleading results being returned in the case where we have
> > a directory where a directory hierarchy is using project id's, but
> > which is *not* using PROJINHERIT.
> 
> One alternative would be to check the PROJINHERIT status of the parent
> directory after calling statfs() on the regular file?  That should
> keep the semantics for PROJINHERIT the same, but avoid inconsistent
> results if called on a regular file:
> 
> #ifdef CONFIG_QUOTA
> -	if (ext4_test_inode_flag(dentry->d_inode, EXT4_INODE_PROJINHERIT) &&
> +	if (ext4_test_inode_flag(S_ISDIR(dentry->d_inode) ? dentry->d_inode :
> +			   dentry->d_parent->d_inode, EXT4_INODE_PROJINHERIT) &&
> 	    sb_has_quota_limits_enabled(sb, PRJQUOTA))
> 		ext4_statfs_project(sb, EXT4_I(dentry->d_inode)->i_projid, buf);
> #endif
> 
> Roman, does that work for you?
> 
> Cheers, Andreas
> 
> 
> 
> 
> 

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Roman Anufriev]

On Tue, 7 Dec 2021, Theodore Y. Ts'o wrote:

> On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
>>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
>>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
>>> ext4_statfs() output incorrect (function does not apply quota limits
>>> on used/available space, etc) when called on dentry of regular file
>>> with project quota enabled.
>
> Under what circumstance is userspace trying to call statfs on a file
> descriptor?
>
> Removing the test for EXT4_INODE_PROJINHERIT will cause
> incorrect/misleading results being returned in the case where we have
> a directory where a directory hierarchy is using project id's, but
> which is *not* using PROJINHERIT.

I'm not sure I quite understood what will be wrong in that case, because
as Dave mentioned:

> PROJINHERIT just indicates the default projid that an inode is
> created with; ...

 								Roman

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Dave Chinner]

On Fri, Dec 10, 2021 at 10:55:10PM +0300, Roman Anufriev wrote:
> 
> On Tue, 7 Dec 2021, Theodore Y. Ts'o wrote:
> 
> > On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
> > > > Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
> > > > removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
> > > > ext4_statfs() output incorrect (function does not apply quota limits
> > > > on used/available space, etc) when called on dentry of regular file
> > > > with project quota enabled.
> > 
> > Under what circumstance is userspace trying to call statfs on a file
> > descriptor?
> > 
> > Removing the test for EXT4_INODE_PROJINHERIT will cause
> > incorrect/misleading results being returned in the case where we have
> > a directory where a directory hierarchy is using project id's, but
> > which is *not* using PROJINHERIT.
> 
> I'm not sure I quite understood what will be wrong in that case, because
> as Dave mentioned:
> 
> > PROJINHERIT just indicates the default projid that an inode is
> > created with; ...

Directory inodes can have a project ID set without PROJINHERIT, it
just means they are accounted to that specific project and have no
special behaviour w.r.t. newly created children in the directory.
i.e. without PROJINHERIT, all children will be created with a
proj ID of zero rather than the projid of the parent directory.

i.e. I can do `xfs_io -c "chproj -R 42" /mnt/test` and it will set
all filesystem and directories to have a projid = 42, but
PROJINHERIT is not set on any directory. The tree gets accounted to
project 42, but it isn't a directory tree quota - it's just a user
controlled aggregation of random files associated with the same
project ID.

Hence the statfs behaviour of "report project quota limits for
directory tree" should only be triggered if PROJINHERIT is set on
the directory, because that's the only viable indicator that
directory tree quotas *may* be in use on the filesystem.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2786 bytes --]

On Dec 13, 2021, at 10:06 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Fri, Dec 10, 2021 at 10:55:10PM +0300, Roman Anufriev wrote:
>> 
>> On Tue, 7 Dec 2021, Theodore Y. Ts'o wrote:
>> 
>>> On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
>>>>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
>>>>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
>>>>> ext4_statfs() output incorrect (function does not apply quota limits
>>>>> on used/available space, etc) when called on dentry of regular file
>>>>> with project quota enabled.
>>> 
>>> Under what circumstance is userspace trying to call statfs on a file
>>> descriptor?
>>> 
>>> Removing the test for EXT4_INODE_PROJINHERIT will cause
>>> incorrect/misleading results being returned in the case where we have
>>> a directory where a directory hierarchy is using project id's, but
>>> which is *not* using PROJINHERIT.
>> 
>> I'm not sure I quite understood what will be wrong in that case, because
>> as Dave mentioned:
>> 
>>> PROJINHERIT just indicates the default projid that an inode is
>>> created with; ...
> 
> Directory inodes can have a project ID set without PROJINHERIT, it
> just means they are accounted to that specific project and have no
> special behaviour w.r.t. newly created children in the directory.
> i.e. without PROJINHERIT, all children will be created with a
> proj ID of zero rather than the projid of the parent directory.
> 
> i.e. I can do `xfs_io -c "chproj -R 42" /mnt/test` and it will set
> all filesystem and directories to have a projid = 42, but
> PROJINHERIT is not set on any directory. The tree gets accounted to
> project 42, but it isn't a directory tree quota - it's just a user
> controlled aggregation of random files associated with the same
> project ID.
> 
> Hence the statfs behaviour of "report project quota limits for
> directory tree" should only be triggered if PROJINHERIT is set on
> the directory, because that's the only viable indicator that
> directory tree quotas *may* be in use on the filesystem.

Sure, I think the question is if statfs() is called on a regular
file in a parent directory with PROJINHERIT set (which is easily
checked) should it return the project limits in the same way as
if statfs() is called on the directory itself?

It seems inconsistent for that statfs("/home/adilger/file") returns
full-filesystem information, but statfs("/home/adilger") and
statfs("/home/adilger/dir") would return project information, if
PROJINHERIT are set on "adilger/" and "dir/".  It kind of ruins
the "tree" aspect, especially for processes that are in a container
that has limits on the subdirectory it is mounting.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: compare inode's i_projid with EXT4_DEF_PROJID rather than check EXT4_INODE_PROJINHERIT flag

[Dave Chinner]

On Thu, Dec 16, 2021 at 05:17:28PM -0700, Andreas Dilger wrote:
> On Dec 13, 2021, at 10:06 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Fri, Dec 10, 2021 at 10:55:10PM +0300, Roman Anufriev wrote:
> >> 
> >> On Tue, 7 Dec 2021, Theodore Y. Ts'o wrote:
> >> 
> >>> On Tue, Dec 07, 2021 at 05:05:19PM +0300, Roman Anufriev wrote:
> >>>>> Commit 7ddf79a10395 ("ext4: only set project inherit bit for directory")
> >>>>> removes EXT4_INODE_PROJINHERIT flag from regular files. This makes
> >>>>> ext4_statfs() output incorrect (function does not apply quota limits
> >>>>> on used/available space, etc) when called on dentry of regular file
> >>>>> with project quota enabled.
> >>> 
> >>> Under what circumstance is userspace trying to call statfs on a file
> >>> descriptor?
> >>> 
> >>> Removing the test for EXT4_INODE_PROJINHERIT will cause
> >>> incorrect/misleading results being returned in the case where we have
> >>> a directory where a directory hierarchy is using project id's, but
> >>> which is *not* using PROJINHERIT.
> >> 
> >> I'm not sure I quite understood what will be wrong in that case, because
> >> as Dave mentioned:
> >> 
> >>> PROJINHERIT just indicates the default projid that an inode is
> >>> created with; ...
> > 
> > Directory inodes can have a project ID set without PROJINHERIT, it
> > just means they are accounted to that specific project and have no
> > special behaviour w.r.t. newly created children in the directory.
> > i.e. without PROJINHERIT, all children will be created with a
> > proj ID of zero rather than the projid of the parent directory.
> > 
> > i.e. I can do `xfs_io -c "chproj -R 42" /mnt/test` and it will set
> > all filesystem and directories to have a projid = 42, but
> > PROJINHERIT is not set on any directory. The tree gets accounted to
> > project 42, but it isn't a directory tree quota - it's just a user
> > controlled aggregation of random files associated with the same
> > project ID.
> > 
> > Hence the statfs behaviour of "report project quota limits for
> > directory tree" should only be triggered if PROJINHERIT is set on
> > the directory, because that's the only viable indicator that
> > directory tree quotas *may* be in use on the filesystem.
> 
> Sure, I think the question is if statfs() is called on a regular
> file in a parent directory with PROJINHERIT set (which is easily
> checked) should it return the project limits in the same way as
> if statfs() is called on the directory itself?

It's more complex than that. If the file and parent projid match,
then maybe it is a directory tree quota, but if they differ then
what?

If the inode has multiple parents (i.e. hard linked) and only some
of them PROJINHERIT and/or matching projids, then what?

IOWs, we're way off into the heuristics realm of guessing what the
user has configured project IDs for and what behaviour they might
want. And given the flexibility of of project quotas, we're going to
lose that guessing game if we start to play it.

However, this guessing game is largely irrelevant because we can't
change the existing user visible behaviour without risking breakage
of existing systems.  The user visible behaviour was defined in the
first commits that introduced directory tree emulation with XFS
project quotas 15 years ago:

commit 932f2c323196c214e645d5a572a1d7b562c0f93f
Author: Nathan Scott <nathans@sgi.com>
Date:   Fri Jun 9 15:29:58 2006 +1000

    [XFS] statvfs component of directory/project quota support, code
    originally by Glen.

    SGI-PV: 932952
    SGI-Modid: xfs-linux-melb:xfs-kern:26105a

    Signed-off-by: Nathan Scott <nathans@sgi.com>

and so it's highly likely that in those 15 years someone now relies
on the behaviour we defined way back then. 

> It seems inconsistent for that statfs("/home/adilger/file") returns
> full-filesystem information, but statfs("/home/adilger") and
> statfs("/home/adilger/dir") would return project information, if
> PROJINHERIT are set on "adilger/" and "dir/".  It kind of ruins
> the "tree" aspect, especially for processes that are in a container
> that has limits on the subdirectory it is mounting.

Yup, but as I keep saying, project quotas are *not* directory tree
quotas. What might "ruin" the tree aspect for you may be the feature
that "makes" it for someone else....

In reality, we had to walk a fine line between the unrestricted
freedom project quotas give users with and the bare minimum
restrictions needed to allow directory tree based propagation and
reporting of space usage that was required at the time.  So while
the behaviour might be less than optimal for specific use cases
we have now, the horse bolted a long, long time ago....


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com