diff mbox series

[03/14] xfs: refactor quota exceeded test

Message ID 157784108138.1364230.6221331077843589601.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs: widen timestamps to deal with y2038 | expand

Commit Message

Darrick J. Wong Jan. 1, 2020, 1:11 a.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor the open-coded test for whether or not we're over quota.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_dquot.c |   61 +++++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

Comments

Eric Sandeen Feb. 12, 2020, 11:51 p.m. UTC | #1 
On 12/31/19 7:11 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> 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@oracle.com>
> ---
>  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?

> +
> +	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*

> +}
> +
>  /*
>   * 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.

>  			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.
Darrick J. Wong Feb. 13, 2020, 1:41 a.m. UTC | #2 
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@oracle.com>
> > 
> > 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@oracle.com>
> > ---
> >  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
Eric Sandeen Feb. 13, 2020, 1:52 a.m. UTC | #3 
On 2/12/20 7:41 PM, Darrick J. Wong wrote:
> 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@oracle.com>
>>>
>>> Refactor the open-coded test for whether or not we're over quota.
>>
>> Ooh, nice.  This was horrible.

...

>>> +}
>>> +
>>>  /*
>>>   * 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?

then you get EDQUOT straightaway and timers don't matter?

I guess if you set a soft limit after you go over a hard-limit-only and ...
meh, ain't broke don't fix?

-Eric
Darrick J. Wong Feb. 13, 2020, 1:59 a.m. UTC | #4 
On Wed, Feb 12, 2020 at 07:52:30PM -0600, Eric Sandeen wrote:
> On 2/12/20 7:41 PM, Darrick J. Wong wrote:
> > 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@oracle.com>
> >>>
> >>> Refactor the open-coded test for whether or not we're over quota.
> >>
> >> Ooh, nice.  This was horrible.
> 
> ...
> 
> >>> +}
> >>> +
> >>>  /*
> >>>   * 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?
> 
> then you get EDQUOT straightaway and timers don't matter?

Hm.  Maybe the idea here was that you always start the timer even if you
just hard-failed the operation?  So that we don't have to deal with
weird cases where timers don't always get started?

> I guess if you set a soft limit after you go over a hard-limit-only and ...
> meh, ain't broke don't fix?

"Behavior changes should be not be in refactoring patches"? :)

> -Eric
>
Amir Goldstein May 31, 2020, 2:04 p.m. UTC | #5 
> > >     } 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.
>

Of course they are.. chiming in to agree with Eric that else if
looks better after the nice cleanup.
But I won't stand in your way to keep the else { if {

Thanks,
Amir.
diff mbox series

Patch

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) {
+
+	if (*softlimit && be64_to_cpup(count) > be64_to_cpup(softlimit))
+		return true;
+	return *hardlimit && be64_to_cpup(count) > be64_to_cpup(hardlimit);
+}
+
 /*
  * 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) {
 			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;
 		}
 	}
 
+	over = xfs_quota_exceeded(&d->d_icount, &d->d_ino_softlimit,
+			&d->d_ino_hardlimit);
 	if (!d->d_itimer) {
-		if ((d->d_ino_softlimit &&
-		     (be64_to_cpu(d->d_icount) >
-		      be64_to_cpu(d->d_ino_softlimit))) ||
-		    (d->d_ino_hardlimit &&
-		     (be64_to_cpu(d->d_icount) >
-		      be64_to_cpu(d->d_ino_hardlimit)))) {
+		if (over) {
 			d->d_itimer = cpu_to_be32(get_seconds() +
 					mp->m_quotainfo->qi_itimelimit);
 		} else {
 			d->d_iwarns = 0;
 		}
 	} else {
-		if ((!d->d_ino_softlimit ||
-		     (be64_to_cpu(d->d_icount) <=
-		      be64_to_cpu(d->d_ino_softlimit)))  &&
-		    (!d->d_ino_hardlimit ||
-		     (be64_to_cpu(d->d_icount) <=
-		      be64_to_cpu(d->d_ino_hardlimit)))) {
+		if (!over) {
 			d->d_itimer = 0;
 		}
 	}
 
+	over = xfs_quota_exceeded(&d->d_rtbcount, &d->d_rtb_softlimit,
+			&d->d_rtb_hardlimit);
 	if (!d->d_rtbtimer) {
-		if ((d->d_rtb_softlimit &&
-		     (be64_to_cpu(d->d_rtbcount) >
-		      be64_to_cpu(d->d_rtb_softlimit))) ||
-		    (d->d_rtb_hardlimit &&
-		     (be64_to_cpu(d->d_rtbcount) >
-		      be64_to_cpu(d->d_rtb_hardlimit)))) {
+		if (over) {
 			d->d_rtbtimer = cpu_to_be32(get_seconds() +
 					mp->m_quotainfo->qi_rtbtimelimit);
 		} else {
 			d->d_rtbwarns = 0;
 		}
 	} else {
-		if ((!d->d_rtb_softlimit ||
-		     (be64_to_cpu(d->d_rtbcount) <=
-		      be64_to_cpu(d->d_rtb_softlimit))) &&
-		    (!d->d_rtb_hardlimit ||
-		     (be64_to_cpu(d->d_rtbcount) <=
-		      be64_to_cpu(d->d_rtb_hardlimit)))) {
+		if (!over) {
 			d->d_rtbtimer = 0;
 		}
 	}