linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ext4: choose hardlimit when softlimit is larger than hardlimit in ext4_statfs_project()
@ 2019-10-10  5:14 Chengguang Xu
  2019-10-15  8:20 ` Jan Kara
  0 siblings, 1 reply; 3+ messages in thread
From: Chengguang Xu @ 2019-10-10  5:14 UTC (permalink / raw)
  To: tytso, adilger.kernel; +Cc: linux-ext4, Chengguang Xu

Setting softlimit larger than hardlimit seems meaningless
for disk quota but currently it is allowed. In this case,
there may be a bit of comfusion for users when they run
df comamnd to directory which has project quota.

For example, we set 20M softlimit and 10M hardlimit of
block usage limit for project quota of test_dir(project id 123).

[root@hades mnt_ext4]# repquota -P -a
*** Report for project quotas on device /dev/loop0
Block grace time: 7days; Inode grace time: 7days
                        Block limits                File limits
Project         used    soft    hard  grace    used  soft  hard  grace
----------------------------------------------------------------------
 0        --      13       0       0              2     0     0
 123      --   10237   20480   10240              5   200   100

The result of df command as below:

[root@hades mnt_ext4]# df -h test_dir
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0       20M   10M   10M  50% /home/cgxu/test/mnt_ext4

Even though it looks like there is another 10M free space to use,
if we write new data to diretory test_dir(inherit project id),
the write will fail with errno(-EDQUOT).

After this patch, the df result looks like below.

[root@hades mnt_ext4]# df -h test_dir
Filesystem      Size  Used Avail Use% Mounted on
/dev/loop0       10M   10M  3.0K 100% /home/cgxu/test/mnt_ext4

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/ext4/super.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dd654e53ba3d..08d4f993b365 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5546,9 +5546,11 @@ static int ext4_statfs_project(struct super_block *sb,
 		return PTR_ERR(dquot);
 	spin_lock(&dquot->dq_dqb_lock);
 
-	limit = (dquot->dq_dqb.dqb_bsoftlimit ?
-		 dquot->dq_dqb.dqb_bsoftlimit :
-		 dquot->dq_dqb.dqb_bhardlimit) >> sb->s_blocksize_bits;
+	limit = ((dquot->dq_dqb.dqb_bhardlimit &&
+		(dquot->dq_dqb.dqb_bhardlimit < dquot->dq_dqb.dqb_bsoftlimit)) ?
+		dquot->dq_dqb.dqb_bhardlimit :
+		dquot->dq_dqb.dqb_bsoftlimit) >> sb->s_blocksize_bits;
+
 	if (limit && buf->f_blocks > limit) {
 		curblock = (dquot->dq_dqb.dqb_curspace +
 			    dquot->dq_dqb.dqb_rsvspace) >> sb->s_blocksize_bits;
@@ -5558,9 +5560,11 @@ static int ext4_statfs_project(struct super_block *sb,
 			 (buf->f_blocks - curblock) : 0;
 	}
 
-	limit = dquot->dq_dqb.dqb_isoftlimit ?
-		dquot->dq_dqb.dqb_isoftlimit :
-		dquot->dq_dqb.dqb_ihardlimit;
+	limit = (dquot->dq_dqb.dqb_ihardlimit &&
+		(dquot->dq_dqb.dqb_ihardlimit < dquot->dq_dqb.dqb_isoftlimit)) ?
+		dquot->dq_dqb.dqb_ihardlimit :
+		dquot->dq_dqb.dqb_isoftlimit;
+
 	if (limit && buf->f_files > limit) {
 		buf->f_files = limit;
 		buf->f_ffree =
-- 
2.20.1




^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] ext4: choose hardlimit when softlimit is larger than hardlimit in ext4_statfs_project()
  2019-10-10  5:14 [PATCH] ext4: choose hardlimit when softlimit is larger than hardlimit in ext4_statfs_project() Chengguang Xu
@ 2019-10-15  8:20 ` Jan Kara
  2019-10-15  9:34   ` Chengguang Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2019-10-15  8:20 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: tytso, adilger.kernel, linux-ext4

On Thu 10-10-19 13:14:26, Chengguang Xu wrote:
> Setting softlimit larger than hardlimit seems meaningless
> for disk quota but currently it is allowed. In this case,
> there may be a bit of comfusion for users when they run
> df comamnd to directory which has project quota.
> 
> For example, we set 20M softlimit and 10M hardlimit of
> block usage limit for project quota of test_dir(project id 123).
> 
> [root@hades mnt_ext4]# repquota -P -a
> *** Report for project quotas on device /dev/loop0
> Block grace time: 7days; Inode grace time: 7days
>                         Block limits                File limits
> Project         used    soft    hard  grace    used  soft  hard  grace
> ----------------------------------------------------------------------
>  0        --      13       0       0              2     0     0
>  123      --   10237   20480   10240              5   200   100
> 
> The result of df command as below:
> 
> [root@hades mnt_ext4]# df -h test_dir
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0       20M   10M   10M  50% /home/cgxu/test/mnt_ext4
> 
> Even though it looks like there is another 10M free space to use,
> if we write new data to diretory test_dir(inherit project id),
> the write will fail with errno(-EDQUOT).
> 
> After this patch, the df result looks like below.
> 
> [root@hades mnt_ext4]# df -h test_dir
> Filesystem      Size  Used Avail Use% Mounted on
> /dev/loop0       10M   10M  3.0K 100% /home/cgxu/test/mnt_ext4
> 
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

Good spotting! But the patch has a bug:

> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index dd654e53ba3d..08d4f993b365 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5546,9 +5546,11 @@ static int ext4_statfs_project(struct super_block *sb,
>  		return PTR_ERR(dquot);
>  	spin_lock(&dquot->dq_dqb_lock);
>  
> -	limit = (dquot->dq_dqb.dqb_bsoftlimit ?
> -		 dquot->dq_dqb.dqb_bsoftlimit :
> -		 dquot->dq_dqb.dqb_bhardlimit) >> sb->s_blocksize_bits;
> +	limit = ((dquot->dq_dqb.dqb_bhardlimit &&
> +		(dquot->dq_dqb.dqb_bhardlimit < dquot->dq_dqb.dqb_bsoftlimit)) ?
> +		dquot->dq_dqb.dqb_bhardlimit :
> +		dquot->dq_dqb.dqb_bsoftlimit) >> sb->s_blocksize_bits;
> +

This is wrong in case softlimit isn't set and hardlimit is. In that case
you'd have 'limit' equal to 0, which is wrong. Also the formula is rather
hard to parse already. So I'd rather go with something like:

	limit = 0;
	if (dquot->dq_dqb.dqb_bsoftlimit &&
	    (!limit || dquot->dq_dqb.dqb_bsoftlimit < limit))
		limit = dquot->dq_dqb.dqb_bsoftlimit;
	if (dquot->dq_dqb.dqb_bhardlimit &&
	    (!limit || dquot->dq_dqb.dqb_bhardlimit < limit))
		limit = dquot->dq_dqb.dqb_bhardlimit;
	limit >>= sb->s_blocksize_bits;

and similarly for inode limit...

								Honza

>  	if (limit && buf->f_blocks > limit) {
>  		curblock = (dquot->dq_dqb.dqb_curspace +
>  			    dquot->dq_dqb.dqb_rsvspace) >> sb->s_blocksize_bits;
> @@ -5558,9 +5560,11 @@ static int ext4_statfs_project(struct super_block *sb,
>  			 (buf->f_blocks - curblock) : 0;
>  	}
>  
> -	limit = dquot->dq_dqb.dqb_isoftlimit ?
> -		dquot->dq_dqb.dqb_isoftlimit :
> -		dquot->dq_dqb.dqb_ihardlimit;
> +	limit = (dquot->dq_dqb.dqb_ihardlimit &&
> +		(dquot->dq_dqb.dqb_ihardlimit < dquot->dq_dqb.dqb_isoftlimit)) ?
> +		dquot->dq_dqb.dqb_ihardlimit :
> +		dquot->dq_dqb.dqb_isoftlimit;
> +
>  	if (limit && buf->f_files > limit) {
>  		buf->f_files = limit;
>  		buf->f_ffree =
> -- 
> 2.20.1
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] ext4: choose hardlimit when softlimit is larger than hardlimit in ext4_statfs_project()
  2019-10-15  8:20 ` Jan Kara
@ 2019-10-15  9:34   ` Chengguang Xu
  0 siblings, 0 replies; 3+ messages in thread
From: Chengguang Xu @ 2019-10-15  9:34 UTC (permalink / raw)
  To: Jan Kara; +Cc: tytso, adilger.kernel, linux-ext4

 ---- 在 星期二, 2019-10-15 16:20:01 Jan Kara <jack@suse.cz> 撰写 ----
 > On Thu 10-10-19 13:14:26, Chengguang Xu wrote:
 > > Setting softlimit larger than hardlimit seems meaningless
 > > for disk quota but currently it is allowed. In this case,
 > > there may be a bit of comfusion for users when they run
 > > df comamnd to directory which has project quota.
 > > 
 > > For example, we set 20M softlimit and 10M hardlimit of
 > > block usage limit for project quota of test_dir(project id 123).
 > > 
 > > [root@hades mnt_ext4]# repquota -P -a
 > > *** Report for project quotas on device /dev/loop0
 > > Block grace time: 7days; Inode grace time: 7days
 > >                         Block limits                File limits
 > > Project         used    soft    hard  grace    used  soft  hard  grace
 > > ----------------------------------------------------------------------
 > >  0        --      13       0       0              2     0     0
 > >  123      --   10237   20480   10240              5   200   100
 > > 
 > > The result of df command as below:
 > > 
 > > [root@hades mnt_ext4]# df -h test_dir
 > > Filesystem      Size  Used Avail Use% Mounted on
 > > /dev/loop0       20M   10M   10M  50% /home/cgxu/test/mnt_ext4
 > > 
 > > Even though it looks like there is another 10M free space to use,
 > > if we write new data to diretory test_dir(inherit project id),
 > > the write will fail with errno(-EDQUOT).
 > > 
 > > After this patch, the df result looks like below.
 > > 
 > > [root@hades mnt_ext4]# df -h test_dir
 > > Filesystem      Size  Used Avail Use% Mounted on
 > > /dev/loop0       10M   10M  3.0K 100% /home/cgxu/test/mnt_ext4
 > > 
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > 
 > Good spotting! But the patch has a bug:
 > 
 > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
 > > index dd654e53ba3d..08d4f993b365 100644
 > > --- a/fs/ext4/super.c
 > > +++ b/fs/ext4/super.c
 > > @@ -5546,9 +5546,11 @@ static int ext4_statfs_project(struct super_block *sb,
 > >          return PTR_ERR(dquot);
 > >      spin_lock(&dquot->dq_dqb_lock);
 > >  
 > > -    limit = (dquot->dq_dqb.dqb_bsoftlimit ?
 > > -         dquot->dq_dqb.dqb_bsoftlimit :
 > > -         dquot->dq_dqb.dqb_bhardlimit) >> sb->s_blocksize_bits;
 > > +    limit = ((dquot->dq_dqb.dqb_bhardlimit &&
 > > +        (dquot->dq_dqb.dqb_bhardlimit < dquot->dq_dqb.dqb_bsoftlimit)) ?
 > > +        dquot->dq_dqb.dqb_bhardlimit :
 > > +        dquot->dq_dqb.dqb_bsoftlimit) >> sb->s_blocksize_bits;
 > > +
 > 
 > This is wrong in case softlimit isn't set and hardlimit is. In that case
 > you'd have 'limit' equal to 0, which is wrong. Also the formula is rather
 > hard to parse already. So I'd rather go with something like:

Ah, you are right, I overlooked it. 
I'll fix in next version. Thanks for your review.

Thanks,
Chengguang

 > 
 >     limit = 0;
 >     if (dquot->dq_dqb.dqb_bsoftlimit &&
 >         (!limit || dquot->dq_dqb.dqb_bsoftlimit < limit))
 >         limit = dquot->dq_dqb.dqb_bsoftlimit;
 >     if (dquot->dq_dqb.dqb_bhardlimit &&
 >         (!limit || dquot->dq_dqb.dqb_bhardlimit < limit))
 >         limit = dquot->dq_dqb.dqb_bhardlimit;
 >     limit >>= sb->s_blocksize_bits;
 > 
 > and similarly for inode limit...
 > 
 >                                 Honza
 > 
 > >      if (limit && buf->f_blocks > limit) {
 > >          curblock = (dquot->dq_dqb.dqb_curspace +
 > >                  dquot->dq_dqb.dqb_rsvspace) >> sb->s_blocksize_bits;
 > > @@ -5558,9 +5560,11 @@ static int ext4_statfs_project(struct super_block *sb,
 > >               (buf->f_blocks - curblock) : 0;
 > >      }
 > >  
 > > -    limit = dquot->dq_dqb.dqb_isoftlimit ?
 > > -        dquot->dq_dqb.dqb_isoftlimit :
 > > -        dquot->dq_dqb.dqb_ihardlimit;
 > > +    limit = (dquot->dq_dqb.dqb_ihardlimit &&
 > > +        (dquot->dq_dqb.dqb_ihardlimit < dquot->dq_dqb.dqb_isoftlimit)) ?
 > > +        dquot->dq_dqb.dqb_ihardlimit :
 > > +        dquot->dq_dqb.dqb_isoftlimit;
 > > +
 > >      if (limit && buf->f_files > limit) {
 > >          buf->f_files = limit;
 > >          buf->f_ffree =
 > > -- 
 > > 2.20.1
 > > 
 > > 
 > > 
 > -- 
 > Jan Kara <jack@suse.com>
 > SUSE Labs, CR
 >


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-15  9:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10  5:14 [PATCH] ext4: choose hardlimit when softlimit is larger than hardlimit in ext4_statfs_project() Chengguang Xu
2019-10-15  8:20 ` Jan Kara
2019-10-15  9:34   ` Chengguang Xu

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