diff mbox series

xfs: make quota default to no warning limit at all

Message ID 20220314180914.GN8224@magnolia (mailing list archive)
State New, archived
Headers show
Series xfs: make quota default to no warning limit at all | expand

Commit Message

Darrick J. Wong March 14, 2022, 6:09 p.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Historically, the quota warning counter was never incremented on a
softlimit violation, and hence was never enforced.  Now that the counter
works, the default of 5 warnings is getting enforced, which is a
breakage that people aren't used to.  In the interest of not introducing
new fail to things that used to work, make the default warning limit of
zero, and make zero mean there is no limit.

Sorta-fixes: 4b8628d57b72 ("xfs: actually bump warning counts when we send warnings")
Reported-by: Eric Sandeen <sandeen@sandeen.net>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_qm.h          |   11 ++++++++---
 fs/xfs/xfs_trans_dquot.c |    3 ++-
 2 files changed, 10 insertions(+), 4 deletions(-)

Comments

Eric Sandeen March 16, 2022, 5:41 p.m. UTC | #1 
On 3/14/22 1:09 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Historically, the quota warning counter was never incremented on a
> softlimit violation, and hence was never enforced.  Now that the counter
> works, the default of 5 warnings is getting enforced, which is a
> breakage that people aren't used to.  In the interest of not introducing
> new fail to things that used to work, make the default warning limit of
> zero, and make zero mean there is no limit.
> 
> Sorta-fixes: 4b8628d57b72 ("xfs: actually bump warning counts when we send warnings")
> Reported-by: Eric Sandeen <sandeen@sandeen.net>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Darrick and I talked about this offline a bit yesterday, and I think
we reached an understanding/agreement on this .... 

While this patch will solve the problem of low warning thresholds
rendering timer thresholds useless, I'm still of the opinion that
this is not a feature to fix, but an inadvertent/broken behavior to
remove.

The concept of a warning limit in xfs quota has been documented as
unimplemented for about 20+ years. Digging through ancient IRIX docs,
the intent may have been to warn once per login session
(which would make more sense with the current limit of 5.) However,
nothing can be found in code archives to indicate that the warning
counter was ever bumped by anything (until the semi-recent change in
Linux.)

This feature is still documented as unimplemented in the xfs_quota
man page.

And although there are skeletal functions to manipulate warning limits
in xfs_quota, they cannot be disabled, and the interface differs from
timer limits, so is barely usable.

There is no concept of a "warning limit" in non-xfs quota tools, either.

There is no documentation on what constitutes a warning event, or when
it should be incremented.

tl;dr: While the warning counter bump has been upstream for some time
now, I think we can argue that that does not constitute a feature that
needs fixing or careful deprecation; TBH it looks more like a bug that
should be fixed by removing the increment altogether.

And then I think we can agree that if warning limits hae been documented
as unimplemented for 20+ years, we can also just remove any other code
that is related to this unimplemented feature.

I /think/ that's more or less where Darrick and I ended up on this one.

If I misremembered or misrepresented anything, Darrick, please correct me :)

Thanks,
-Eric

> ---
>  fs/xfs/xfs_qm.h          |   11 ++++++++---
>  fs/xfs/xfs_trans_dquot.c |    3 ++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> index 5bb12717ea28..2013f6100067 100644
> --- a/fs/xfs/xfs_qm.h
> +++ b/fs/xfs/xfs_qm.h
> @@ -134,9 +134,14 @@ struct xfs_dquot_acct {
>  #define XFS_QM_RTBTIMELIMIT	(7 * 24*60*60)          /* 1 week */
>  #define XFS_QM_ITIMELIMIT	(7 * 24*60*60)          /* 1 week */
>  
> -#define XFS_QM_BWARNLIMIT	5
> -#define XFS_QM_IWARNLIMIT	5
> -#define XFS_QM_RTBWARNLIMIT	5
> +/*
> + * Histerically, the quota warning counter never incremented and hence was
> + * never enforced.  Now that the counter works, we set a default warning limit
> + * of zero, which means there is no limit.
> + */
> +#define XFS_QM_BWARNLIMIT	0
> +#define XFS_QM_IWARNLIMIT	0
> +#define XFS_QM_RTBWARNLIMIT	0
>  
>  extern void		xfs_qm_destroy_quotainfo(struct xfs_mount *);
>  
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index 9ba7e6b9bed3..32da74cf0768 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -598,7 +598,8 @@ xfs_dqresv_check(
>  		time64_t	now = ktime_get_real_seconds();
>  
>  		if ((res->timer != 0 && now > res->timer) ||
> -		    (res->warnings != 0 && res->warnings >= qlim->warn)) {
> +		    (res->warnings != 0 && qlim->warn != 0 &&
> +		     res->warnings >= qlim->warn)) {
>  			*fatal = true;
>  			return QUOTA_NL_ISOFTLONGWARN;
>  		}
>
Dave Chinner March 17, 2022, 2:22 a.m. UTC | #2 
On Wed, Mar 16, 2022 at 12:41:08PM -0500, Eric Sandeen wrote:
> On 3/14/22 1:09 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Historically, the quota warning counter was never incremented on a
> > softlimit violation, and hence was never enforced.  Now that the counter
> > works, the default of 5 warnings is getting enforced, which is a
> > breakage that people aren't used to.  In the interest of not introducing
> > new fail to things that used to work, make the default warning limit of
> > zero, and make zero mean there is no limit.
> > 
> > Sorta-fixes: 4b8628d57b72 ("xfs: actually bump warning counts when we send warnings")
> > Reported-by: Eric Sandeen <sandeen@sandeen.net>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> Darrick and I talked about this offline a bit yesterday, and I think
> we reached an understanding/agreement on this .... 
> 
> While this patch will solve the problem of low warning thresholds
> rendering timer thresholds useless, I'm still of the opinion that
> this is not a feature to fix, but an inadvertent/broken behavior to
> remove.
> 
> The concept of a warning limit in xfs quota has been documented as
> unimplemented for about 20+ years. Digging through ancient IRIX docs,
> the intent may have been to warn once per login session
> (which would make more sense with the current limit of 5.) However,
> nothing can be found in code archives to indicate that the warning
> counter was ever bumped by anything (until the semi-recent change in
> Linux.)
> 
> This feature is still documented as unimplemented in the xfs_quota
> man page.
> 
> And although there are skeletal functions to manipulate warning limits
> in xfs_quota, they cannot be disabled, and the interface differs from
> timer limits, so is barely usable.
> 
> There is no concept of a "warning limit" in non-xfs quota tools, either.
> 
> There is no documentation on what constitutes a warning event, or when
> it should be incremented.
> 
> tl;dr: While the warning counter bump has been upstream for some time
> now, I think we can argue that that does not constitute a feature that
> needs fixing or careful deprecation; TBH it looks more like a bug that
> should be fixed by removing the increment altogether.
> 
> And then I think we can agree that if warning limits hae been documented
> as unimplemented for 20+ years, we can also just remove any other code
> that is related to this unimplemented feature.

Sounds fine to me. THe less untested, undefined legacy code with
custom user APIs we have to carry around the better. Remove it all
before someone starts poking at it with a sharp stick and finds a
zany zero-day....

Cheers,

Dave.
Darrick J. Wong March 17, 2022, 2:53 a.m. UTC | #3 
On Thu, Mar 17, 2022 at 01:22:19PM +1100, Dave Chinner wrote:
> On Wed, Mar 16, 2022 at 12:41:08PM -0500, Eric Sandeen wrote:
> > On 3/14/22 1:09 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Historically, the quota warning counter was never incremented on a
> > > softlimit violation, and hence was never enforced.  Now that the counter
> > > works, the default of 5 warnings is getting enforced, which is a
> > > breakage that people aren't used to.  In the interest of not introducing
> > > new fail to things that used to work, make the default warning limit of
> > > zero, and make zero mean there is no limit.
> > > 
> > > Sorta-fixes: 4b8628d57b72 ("xfs: actually bump warning counts when we send warnings")
> > > Reported-by: Eric Sandeen <sandeen@sandeen.net>
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > 
> > Darrick and I talked about this offline a bit yesterday, and I think
> > we reached an understanding/agreement on this .... 
> > 
> > While this patch will solve the problem of low warning thresholds
> > rendering timer thresholds useless, I'm still of the opinion that
> > this is not a feature to fix, but an inadvertent/broken behavior to
> > remove.
> > 
> > The concept of a warning limit in xfs quota has been documented as
> > unimplemented for about 20+ years. Digging through ancient IRIX docs,
> > the intent may have been to warn once per login session
> > (which would make more sense with the current limit of 5.) However,
> > nothing can be found in code archives to indicate that the warning
> > counter was ever bumped by anything (until the semi-recent change in
> > Linux.)
> > 
> > This feature is still documented as unimplemented in the xfs_quota
> > man page.
> > 
> > And although there are skeletal functions to manipulate warning limits
> > in xfs_quota, they cannot be disabled, and the interface differs from
> > timer limits, so is barely usable.
> > 
> > There is no concept of a "warning limit" in non-xfs quota tools, either.
> > 
> > There is no documentation on what constitutes a warning event, or when
> > it should be incremented.
> > 
> > tl;dr: While the warning counter bump has been upstream for some time
> > now, I think we can argue that that does not constitute a feature that
> > needs fixing or careful deprecation; TBH it looks more like a bug that
> > should be fixed by removing the increment altogether.
> > 
> > And then I think we can agree that if warning limits hae been documented
> > as unimplemented for 20+ years, we can also just remove any other code
> > that is related to this unimplemented feature.
> 
> Sounds fine to me. THe less untested, undefined legacy code with
> custom user APIs we have to carry around the better. Remove it all
> before someone starts poking at it with a sharp stick and finds a
> zany zero-day....

LOLYUP.

Hey Catherine, are you interested in /removing/ the quota warning limit
code from XFS?  Note: just the limits, not the actually issuance of
quota warnings (xfs_quota_warn) nor the warning counter itself.

I think a good place to start would be to remove the 'warn' field from
struct xfs_quota_limits, and then remove code as necessary to fix all
the compilation errors.  I think you can leave the actual warning
counter itself (struct xfs_dquot_res.warnings) since it (roughly) tracks
how many times we've sent a warning over netlink to ... wherever they
go.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Eric Sandeen March 17, 2022, 3:10 p.m. UTC | #4 
On 3/16/22 9:53 PM, Darrick J. Wong wrote:
> On Thu, Mar 17, 2022 at 01:22:19PM +1100, Dave Chinner wrote:
>> On Wed, Mar 16, 2022 at 12:41:08PM -0500, Eric Sandeen wrote:
>>> On 3/14/22 1:09 PM, Darrick J. Wong wrote:
>>>> From: Darrick J. Wong <djwong@kernel.org>
>>>>
>>>> Historically, the quota warning counter was never incremented on a
>>>> softlimit violation, and hence was never enforced.  Now that the counter
>>>> works, the default of 5 warnings is getting enforced, which is a
>>>> breakage that people aren't used to.  In the interest of not introducing
>>>> new fail to things that used to work, make the default warning limit of
>>>> zero, and make zero mean there is no limit.
>>>>
>>>> Sorta-fixes: 4b8628d57b72 ("xfs: actually bump warning counts when we send warnings")
>>>> Reported-by: Eric Sandeen <sandeen@sandeen.net>
>>>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>>>
>>> Darrick and I talked about this offline a bit yesterday, and I think
>>> we reached an understanding/agreement on this .... 
>>>
>>> While this patch will solve the problem of low warning thresholds
>>> rendering timer thresholds useless, I'm still of the opinion that
>>> this is not a feature to fix, but an inadvertent/broken behavior to
>>> remove.
>>>
>>> The concept of a warning limit in xfs quota has been documented as
>>> unimplemented for about 20+ years. Digging through ancient IRIX docs,
>>> the intent may have been to warn once per login session
>>> (which would make more sense with the current limit of 5.) However,
>>> nothing can be found in code archives to indicate that the warning
>>> counter was ever bumped by anything (until the semi-recent change in
>>> Linux.)
>>>
>>> This feature is still documented as unimplemented in the xfs_quota
>>> man page.
>>>
>>> And although there are skeletal functions to manipulate warning limits
>>> in xfs_quota, they cannot be disabled, and the interface differs from
>>> timer limits, so is barely usable.
>>>
>>> There is no concept of a "warning limit" in non-xfs quota tools, either.
>>>
>>> There is no documentation on what constitutes a warning event, or when
>>> it should be incremented.
>>>
>>> tl;dr: While the warning counter bump has been upstream for some time
>>> now, I think we can argue that that does not constitute a feature that
>>> needs fixing or careful deprecation; TBH it looks more like a bug that
>>> should be fixed by removing the increment altogether.
>>>
>>> And then I think we can agree that if warning limits hae been documented
>>> as unimplemented for 20+ years, we can also just remove any other code
>>> that is related to this unimplemented feature.
>>
>> Sounds fine to me. THe less untested, undefined legacy code with
>> custom user APIs we have to carry around the better. Remove it all
>> before someone starts poking at it with a sharp stick and finds a
>> zany zero-day....
> 
> LOLYUP.
> 
> Hey Catherine, are you interested in /removing/ the quota warning limit
> code from XFS?  Note: just the limits, not the actually issuance of
> quota warnings (xfs_quota_warn) nor the warning counter itself.
> 
> I think a good place to start would be to remove the 'warn' field from
> struct xfs_quota_limits, and then remove code as necessary to fix all
> the compilation errors.  I think you can leave the actual warning
> counter itself (struct xfs_dquot_res.warnings) since it (roughly) tracks
> how many times we've sent a warning over netlink to ... wherever they
> go.

I think we also discussed a separate patch to simply remove the counter bump,
which is easily backportable to distros and stable kernels?

thanks,
-Eric
diff mbox series

Patch

diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 5bb12717ea28..2013f6100067 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -134,9 +134,14 @@  struct xfs_dquot_acct {
 #define XFS_QM_RTBTIMELIMIT	(7 * 24*60*60)          /* 1 week */
 #define XFS_QM_ITIMELIMIT	(7 * 24*60*60)          /* 1 week */
 
-#define XFS_QM_BWARNLIMIT	5
-#define XFS_QM_IWARNLIMIT	5
-#define XFS_QM_RTBWARNLIMIT	5
+/*
+ * Histerically, the quota warning counter never incremented and hence was
+ * never enforced.  Now that the counter works, we set a default warning limit
+ * of zero, which means there is no limit.
+ */
+#define XFS_QM_BWARNLIMIT	0
+#define XFS_QM_IWARNLIMIT	0
+#define XFS_QM_RTBWARNLIMIT	0
 
 extern void		xfs_qm_destroy_quotainfo(struct xfs_mount *);
 
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 9ba7e6b9bed3..32da74cf0768 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -598,7 +598,8 @@  xfs_dqresv_check(
 		time64_t	now = ktime_get_real_seconds();
 
 		if ((res->timer != 0 && now > res->timer) ||
-		    (res->warnings != 0 && res->warnings >= qlim->warn)) {
+		    (res->warnings != 0 && qlim->warn != 0 &&
+		     res->warnings >= qlim->warn)) {
 			*fatal = true;
 			return QUOTA_NL_ISOFTLONGWARN;
 		}