[PATCH] xfs: use more suitable method to get the quota limit value

[PATCH] xfs: use more suitable method to get the quota limit value

[xiakaixu1987]

From: Kaixu Xia <kaixuxia@tencent.com>

It is more suitable to use min_not_zero() to get the quota limit
value, means to choose the minimum value not the softlimit firstly.
And the meaning is more clear even though the hardlimit value must
be larger than softlimit value.

Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
---
 fs/xfs/xfs_qm_bhv.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
index fc2fa41..f1711f5 100644
--- a/fs/xfs/xfs_qm_bhv.c
+++ b/fs/xfs/xfs_qm_bhv.c
@@ -23,9 +23,8 @@
 {
 	uint64_t		limit;
 
-	limit = dqp->q_core.d_blk_softlimit ?
-		be64_to_cpu(dqp->q_core.d_blk_softlimit) :
-		be64_to_cpu(dqp->q_core.d_blk_hardlimit);
+	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_blk_softlimit),
+				be64_to_cpu(dqp->q_core.d_blk_hardlimit));
 	if (limit && statp->f_blocks > limit) {
 		statp->f_blocks = limit;
 		statp->f_bfree = statp->f_bavail =
@@ -33,9 +32,8 @@
 			 (statp->f_blocks - dqp->q_res_bcount) : 0;
 	}
 
-	limit = dqp->q_core.d_ino_softlimit ?
-		be64_to_cpu(dqp->q_core.d_ino_softlimit) :
-		be64_to_cpu(dqp->q_core.d_ino_hardlimit);
+	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_ino_softlimit),
+				be64_to_cpu(dqp->q_core.d_ino_hardlimit));
 	if (limit && statp->f_files > limit) {
 		statp->f_files = limit;
 		statp->f_ffree =
-- 
1.8.3.1

Re: [PATCH] xfs: use more suitable method to get the quota limit value

[Eric Sandeen]

On 3/17/20 4:59 AM, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> It is more suitable to use min_not_zero() to get the quota limit
> value, means to choose the minimum value not the softlimit firstly.
> And the meaning is more clear even though the hardlimit value must
> be larger than softlimit value.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

I think that you are correct that as long as we can never have
soft > hard, this is equivalent logic.

However, I had to think pretty hard to convince myself that this is
true, and the current code is obvious and easy to understand.

Others might have a different idea, but I'm not sure this change is an
improvement.

I think of Brian Kernighan, "...if you write the code as cleverly as possible,
you are, by definition, not smart enough to debug it." ;)

> ---
>  fs/xfs/xfs_qm_bhv.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index fc2fa41..f1711f5 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -23,9 +23,8 @@
>  {
>  	uint64_t		limit;
>  
> -	limit = dqp->q_core.d_blk_softlimit ?
> -		be64_to_cpu(dqp->q_core.d_blk_softlimit) :
> -		be64_to_cpu(dqp->q_core.d_blk_hardlimit);
> +	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_blk_softlimit),
> +				be64_to_cpu(dqp->q_core.d_blk_hardlimit));
>  	if (limit && statp->f_blocks > limit) {
>  		statp->f_blocks = limit;
>  		statp->f_bfree = statp->f_bavail =
> @@ -33,9 +32,8 @@
>  			 (statp->f_blocks - dqp->q_res_bcount) : 0;
>  	}
>  
> -	limit = dqp->q_core.d_ino_softlimit ?
> -		be64_to_cpu(dqp->q_core.d_ino_softlimit) :
> -		be64_to_cpu(dqp->q_core.d_ino_hardlimit);
> +	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_ino_softlimit),
> +				be64_to_cpu(dqp->q_core.d_ino_hardlimit));
>  	if (limit && statp->f_files > limit) {
>  		statp->f_files = limit;
>  		statp->f_ffree =
> 

Re: [PATCH] xfs: use more suitable method to get the quota limit value

[Bill O'Donnell]

On Tue, Mar 17, 2020 at 05:59:30PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> It is more suitable to use min_not_zero() to get the quota limit
> value, means to choose the minimum value not the softlimit firstly.
> And the meaning is more clear even though the hardlimit value must
> be larger than softlimit value.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>

Hi -
I think the original code is more clear and readable. Just my take.
Thanks-
Bill


> ---
>  fs/xfs/xfs_qm_bhv.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index fc2fa41..f1711f5 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -23,9 +23,8 @@
>  {
>  	uint64_t		limit;
>  
> -	limit = dqp->q_core.d_blk_softlimit ?
> -		be64_to_cpu(dqp->q_core.d_blk_softlimit) :
> -		be64_to_cpu(dqp->q_core.d_blk_hardlimit);
> +	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_blk_softlimit),
> +				be64_to_cpu(dqp->q_core.d_blk_hardlimit));
>  	if (limit && statp->f_blocks > limit) {
>  		statp->f_blocks = limit;
>  		statp->f_bfree = statp->f_bavail =
> @@ -33,9 +32,8 @@
>  			 (statp->f_blocks - dqp->q_res_bcount) : 0;
>  	}
>  
> -	limit = dqp->q_core.d_ino_softlimit ?
> -		be64_to_cpu(dqp->q_core.d_ino_softlimit) :
> -		be64_to_cpu(dqp->q_core.d_ino_hardlimit);
> +	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_ino_softlimit),
> +				be64_to_cpu(dqp->q_core.d_ino_hardlimit));
>  	if (limit && statp->f_files > limit) {
>  		statp->f_files = limit;
>  		statp->f_ffree =
> -- 
> 1.8.3.1
> 

Re: [PATCH] xfs: use more suitable method to get the quota limit value

[Christoph Hellwig]

On Tue, Mar 17, 2020 at 05:59:30PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> It is more suitable to use min_not_zero() to get the quota limit
> value, means to choose the minimum value not the softlimit firstly.
> And the meaning is more clear even though the hardlimit value must
> be larger than softlimit value.

Suitable seems like a rather overblown word, but the actual change looks
fine to me, min_not_zero has grown on me..

Re: [PATCH] xfs: use more suitable method to get the quota limit value

[Dave Chinner]

On Tue, Mar 17, 2020 at 05:59:30PM +0800, xiakaixu1987@gmail.com wrote:
> From: Kaixu Xia <kaixuxia@tencent.com>
> 
> It is more suitable to use min_not_zero() to get the quota limit
> value, means to choose the minimum value not the softlimit firstly.
> And the meaning is more clear even though the hardlimit value must
> be larger than softlimit value.
> 
> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com>
> ---
>  fs/xfs/xfs_qm_bhv.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm_bhv.c b/fs/xfs/xfs_qm_bhv.c
> index fc2fa41..f1711f5 100644
> --- a/fs/xfs/xfs_qm_bhv.c
> +++ b/fs/xfs/xfs_qm_bhv.c
> @@ -23,9 +23,8 @@
>  {
>  	uint64_t		limit;
>  
> -	limit = dqp->q_core.d_blk_softlimit ?
> -		be64_to_cpu(dqp->q_core.d_blk_softlimit) :
> -		be64_to_cpu(dqp->q_core.d_blk_hardlimit);
> +	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_blk_softlimit),
> +				be64_to_cpu(dqp->q_core.d_blk_hardlimit));
>  	if (limit && statp->f_blocks > limit) {
>  		statp->f_blocks = limit;
>  		statp->f_bfree = statp->f_bavail =
> @@ -33,9 +32,8 @@
>  			 (statp->f_blocks - dqp->q_res_bcount) : 0;
>  	}
>  
> -	limit = dqp->q_core.d_ino_softlimit ?
> -		be64_to_cpu(dqp->q_core.d_ino_softlimit) :
> -		be64_to_cpu(dqp->q_core.d_ino_hardlimit);
> +	limit = min_not_zero(be64_to_cpu(dqp->q_core.d_ino_softlimit),
> +				be64_to_cpu(dqp->q_core.d_ino_hardlimit));

Which variable is "not zero"? The first, the second, or both?

Oh, it's both, so this is actually a change of logic:

	if (softlimit == 0)
		limit = hardlimit;
	else if (hardlimit == 0)
		limit = softlimit;
	else
		limit = min(softlimit, hardlimit);

So now if both soft and hard limit are set, the hard limit overrides
the soft limit, even when only the soft limit should apply (e.g.
during the grace period).

Hence this ends up being a user visible change in behaviour. And,
IMO, this doesn't make the code better or clearer as now I have go
find the definition of min_not_zero() to understand what the soft
limit vs hard limit quota policy logic is. That's not an improvement.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com