* Re: [PATCH] xfs: use more suitable method to get the quota limit value
2020-03-17 9:59 [PATCH] xfs: use more suitable method to get the quota limit value xiakaixu1987
@ 2020-03-17 14:34 ` Eric Sandeen
2020-03-17 14:53 ` Bill O'Donnell
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Eric Sandeen @ 2020-03-17 14:34 UTC (permalink / raw)
To: xiakaixu1987, linux-xfs; +Cc: darrick.wong, Kaixu Xia
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 =
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xfs: use more suitable method to get the quota limit value
2020-03-17 9:59 [PATCH] xfs: use more suitable method to get the quota limit value xiakaixu1987
2020-03-17 14:34 ` Eric Sandeen
@ 2020-03-17 14:53 ` Bill O'Donnell
2020-03-17 18:58 ` Christoph Hellwig
2020-03-17 22:45 ` Dave Chinner
3 siblings, 0 replies; 5+ messages in thread
From: Bill O'Donnell @ 2020-03-17 14:53 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xfs: use more suitable method to get the quota limit value
2020-03-17 9:59 [PATCH] xfs: use more suitable method to get the quota limit value xiakaixu1987
2020-03-17 14:34 ` Eric Sandeen
2020-03-17 14:53 ` Bill O'Donnell
@ 2020-03-17 18:58 ` Christoph Hellwig
2020-03-17 22:45 ` Dave Chinner
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2020-03-17 18:58 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
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..
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: use more suitable method to get the quota limit value
2020-03-17 9:59 [PATCH] xfs: use more suitable method to get the quota limit value xiakaixu1987
` (2 preceding siblings ...)
2020-03-17 18:58 ` Christoph Hellwig
@ 2020-03-17 22:45 ` Dave Chinner
3 siblings, 0 replies; 5+ messages in thread
From: Dave Chinner @ 2020-03-17 22:45 UTC (permalink / raw)
To: xiakaixu1987; +Cc: linux-xfs, darrick.wong, Kaixu Xia
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
^ permalink raw reply [flat|nested] 5+ messages in thread