| Message ID | 1584439170-20993-1-git-send-email-kaixuxia@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: use more suitable method to get the quota limit value | expand |
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 = >
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 >
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..
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.
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 =