On Wed, Feb 12, 2020 at 05:51:18PM -0600, Eric Sandeen wrote:
> On 12/31/19 7:11 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Refactor the open-coded test for whether or not we're over quota.
>
> Ooh, nice. This was horrible.
>
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/xfs_dquot.c | 61 +++++++++++++++++++++-------------------------------
> > 1 file changed, 25 insertions(+), 36 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index e50c75d9d788..54e7fdcd1d4d 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -99,6 +99,17 @@ xfs_qm_adjust_dqlimits(
> > xfs_dquot_set_prealloc_limits(dq);
> > }
> >
> > +static inline bool
> > +xfs_quota_exceeded(
> > + const __be64 *count,
> > + const __be64 *softlimit,
> > + const __be64 *hardlimit) {
>
> why pass these all as pointers?
I don't remember. I think a previous iteration of bigtime had something
to do with messing with the dquot directly?
> > +
> > + if (*softlimit && be64_to_cpup(count) > be64_to_cpup(softlimit))
> > + return true;
> > + return *hardlimit && be64_to_cpup(count) > be64_to_cpup(hardlimit);
>
> The asymmetry bothers me a little but maybe that's just me. Is
>
> > + if ((*softlimit && be64_to_cpup(count) > be64_to_cpup(softlimit)) ||
> > + (*hardlimit && be64_to_cpup(count) > be64_to_cpup(hardlimit)))
> > + return true;
> > + return false;
>
> any better? *shrug*
Yeah, I could fix that function.
> > +}
> > +
> > /*
> > * Check the limits and timers of a dquot and start or reset timers
> > * if necessary.
> > @@ -117,6 +128,8 @@ xfs_qm_adjust_dqtimers(
> > struct xfs_mount *mp,
> > struct xfs_disk_dquot *d)
> > {
> > + bool over;
> > +
> > ASSERT(d->d_id);
> >
> > #ifdef DEBUG
> > @@ -131,71 +144,47 @@ xfs_qm_adjust_dqtimers(
> > be64_to_cpu(d->d_rtb_hardlimit));
> > #endif
> >
> > + over = xfs_quota_exceeded(&d->d_bcount, &d->d_blk_softlimit,
> > + &d->d_blk_hardlimit);
> > if (!d->d_btimer) {
> > - if ((d->d_blk_softlimit && (be64_to_cpu(d->d_bcount) > be64_to_cpu(d->d_blk_softlimit))) ||
> > - (d->d_blk_hardlimit && (be64_to_cpu(d->d_bcount) > be64_to_cpu(d->d_blk_hardlimit)))) {
> > + if (over) {
>
> I wonder why we check the hard limit. Isn't exceeding the soft limit
> enough to start the timer? Unrelated to the refactoring tho.
Suppose there's only a hard limit set?
> > d->d_btimer = cpu_to_be32(get_seconds() +
> > mp->m_quotainfo->qi_btimelimit);
> > } else {
> > d->d_bwarns = 0;
> > }
> > } else {
> > - if ((!d->d_blk_softlimit || (be64_to_cpu(d->d_bcount) <= be64_to_cpu(d->d_blk_softlimit))) &&
> > - (!d->d_blk_hardlimit || (be64_to_cpu(d->d_bcount) <= be64_to_cpu(d->d_blk_hardlimit)))) {
> > + if (!over) {
> > d->d_btimer = 0;
> > }
>
> I guess that could be
>
> > } else if (!over) {
> > d->d_btimer = 0;
> > }
>
> ? but again *shrug* and that's beyond refactoring, isn't it.
Strictly speaking, yes, but I think they're logically equivalent.
--D
