| Message ID | 159477799812.3263162.13957383827318048593.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: remove xfs_disk_quot from incore dquot | expand |
On Wednesday 15 July 2020 7:23:18 AM IST Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Currently, xfs quotas have the ability to send netlink warnings when a > user exceeds the limits. They also have all the support code necessary > to convert softlimit warnings into failures if the number of warnings > exceeds a limit set by the administrator. Unfortunately, we never > actually increase the warning counter, so this never actually happens. > Make it so we actually do something useful with the warning counts. > The changes look good to me. Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_trans_dquot.c | 1 + > 1 file changed, 1 insertion(+) > > > diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c > index 78201ff3696b..cbd92d8b693d 100644 > --- a/fs/xfs/xfs_trans_dquot.c > +++ b/fs/xfs/xfs_trans_dquot.c > @@ -596,6 +596,7 @@ xfs_dqresv_check( > return QUOTA_NL_ISOFTLONGWARN; > } > > + res->warnings++; > return QUOTA_NL_ISOFTWARN; > } > > >
On 7/14/20 8:53 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Currently, xfs quotas have the ability to send netlink warnings when a > user exceeds the limits. They also have all the support code necessary > to convert softlimit warnings into failures if the number of warnings > exceeds a limit set by the administrator. Unfortunately, we never > actually increase the warning counter, so this never actually happens. > Make it so we actually do something useful with the warning counts. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> Sooo I got a bug report that this essentially breaks the timer for soft quota, because we now (and quite rapidly) hit the default 5-warning limit well before we hit any reasonable timer that may have been set, and disallow more space usage. And those warnings rack up in somewhat unexpected (to me, anyway) ways. With a default max warning count of 5, I go over soft quota exactly once, touch/create 2 more empty inodes, and I'm done: # rm -f /mnt/xfs/* # xfs_quota -x -c 'report -h' /mnt/xfs User quota on /mnt/xfs (/dev/loop1) Blocks User ID Used Soft Hard Warn/Grace ---------- --------------------------------- root 0 0 0 05 [0 days] quota_test 0 1M 550M 00 [------] # sudo -u quota_test dd bs=1100k count=1 if=/dev/zero of=/mnt/xfs/test 1126400 bytes (1.1 MB) copied, 0.00136115 s, 828 MB/s # xfs_quota -x -c 'report -h' /mnt/xfs User quota on /mnt/xfs (/dev/loop1) Blocks User ID Used Soft Hard Warn/Grace ---------- --------------------------------- root 0 0 0 05 [0 days] quota_test 1.1M 1M 550M 01 [------] # sudo -u quota_test touch /mnt/xfs/a # xfs_quota -x -c 'report -h' /mnt/xfs User quota on /mnt/xfs (/dev/loop1) Blocks User ID Used Soft Hard Warn/Grace ---------- --------------------------------- root 0 0 0 05 [0 days] quota_test 1.1M 1M 550M 03 [6 days] # sudo -u quota_test touch /mnt/xfs/b # xfs_quota -x -c 'report -h' /mnt/xfs User quota on /mnt/xfs (/dev/loop1) Blocks User ID Used Soft Hard Warn/Grace ---------- --------------------------------- root 0 0 0 05 [0 days] quota_test 1.1M 1M 550M 05 [6 days] # sudo -u quota_test touch /mnt/xfs/c touch: cannot touch ‘/mnt/xfs/c’: Disk quota exceeded And the xfs_quota manpage doesn't even say that this is supposed to be a transition to a hard limit, although the code does seem to think so ... "Allows the quota warnings limit (i.e. the number of times a warning will be send to someone over quota) to be viewed and modified." There are other oddities too, like a (default) 0 day timer means "no timer" but a 0 warning count means "you get no warnings. you're done when you hit the soft quota." And the xfs_quota interface for setting the warnings is unexpectedly different from the interface for timers, as well. So ... thoughts? TBH I'd almost suggest reverting the increment until this is sorted, but I presume you changed this for a reason. :) (And it's been there a pretty long time, now.) Thanks, -Erifc
On 3/1/22 1:31 PM, Eric Sandeen wrote: > On 7/14/20 8:53 PM, Darrick J. Wong wrote: >> From: Darrick J. Wong <darrick.wong@oracle.com> >> >> Currently, xfs quotas have the ability to send netlink warnings when a >> user exceeds the limits. They also have all the support code necessary >> to convert softlimit warnings into failures if the number of warnings >> exceeds a limit set by the administrator. Unfortunately, we never >> actually increase the warning counter, so this never actually happens. >> Make it so we actually do something useful with the warning counts. >> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> > > Sooo I got a bug report that this essentially breaks the timer for > soft quota, because we now (and quite rapidly) hit the default > 5-warning limit well before we hit any reasonable timer that may > have been set, and disallow more space usage. > > And those warnings rack up in somewhat unexpected (to me, anyway) > ways. With a default max warning count of 5, I go over soft quota > exactly once, touch/create 2 more empty inodes, and I'm done: Looking at this some more, I think it was never clear when the warnings should get incremented. An old IRIX document[1] says: "With soft limits, whenever a user logs in with a usage greater than his soft limit, he or she will be warned (via/bin/login(1))." Which seems to indicate that perhaps the warning was intended to be once per login, not once per allocation attempt. Also ... Ancient XFS code had a "xfs_qm_dqwarn()" function which incremented the warning count, but it never had any callers until the day it was removed in 2005, so it's not at all clear what the warning frequency was supposed to be or what should trigger it, from the code archives. Hence, my modest proposal would be to just remove the warning limits infrastructure altogether. It's never worked, nobody has ever asked for it (?), and its intent is not clear. My only hesitation is that Darrick added the warning increment, so perhaps he knows of a current use case that matters? thanks, -Eric [1] https://irix7.com/techpubs/007-0603-100.pdf
On Wed, Mar 02, 2022 at 12:19:21PM -0600, Eric Sandeen wrote: > On 3/1/22 1:31 PM, Eric Sandeen wrote: > > On 7/14/20 8:53 PM, Darrick J. Wong wrote: > >> From: Darrick J. Wong <darrick.wong@oracle.com> > >> > >> Currently, xfs quotas have the ability to send netlink warnings when a > >> user exceeds the limits. They also have all the support code necessary > >> to convert softlimit warnings into failures if the number of warnings > >> exceeds a limit set by the administrator. Unfortunately, we never > >> actually increase the warning counter, so this never actually happens. > >> Make it so we actually do something useful with the warning counts. > >> > >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > >> Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > Sooo I got a bug report that this essentially breaks the timer for > > soft quota, because we now (and quite rapidly) hit the default > > 5-warning limit well before we hit any reasonable timer that may > > have been set, and disallow more space usage. > > > > And those warnings rack up in somewhat unexpected (to me, anyway) > > ways. With a default max warning count of 5, I go over soft quota > > exactly once, touch/create 2 more empty inodes, and I'm done: > > Looking at this some more, I think it was never clear when the warnings > should get incremented. An old IRIX document[1] says: > > "With soft limits, whenever a user logs in with a usage greater than his > soft limit, he or she will be warned (via/bin/login(1))." > > Which seems to indicate that perhaps the warning was intended to be > once per login, not once per allocation attempt. Also ... > > Ancient XFS code had a "xfs_qm_dqwarn()" function which incremented the > warning count, but it never had any callers until the day it was removed > in 2005, so it's not at all clear what the warning frequency was supposed > to be or what should trigger it, from the code archives. > > Hence, my modest proposal would be to just remove the warning limits > infrastructure altogether. It's never worked, nobody has ever asked for it > (?), and its intent is not clear. My only hesitation is that Darrick added > the warning increment, so perhaps he knows of a current use case that > matters? None specifically, but it's a feature, albeit a poorly documented and previously broken one. VFS quotas don't seem to have any warning limits, so I suppose there's not a lot of precedent to go on. That said -- I don't how gutting a feature (especially since it's now been *working* for a year and a half) is the solution here. If you think the default warning limit is too low, then perhaps we should increase it. If you don't like that a single user operation can bump the warning counter multiple times, then propose adding a flag to the dqtrx structure so that we only bump the warning counter *once* per transaction. "It's never worked" is not true -- this fix was added for 5.9, and it's now shipped in two LTS kernels. On the other hand, if you think this feature is totally worthless and it should go away, there's a deprecation process for that. --D > > thanks, > -Eric > > [1] https://irix7.com/techpubs/007-0603-100.pdf
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c index 78201ff3696b..cbd92d8b693d 100644 --- a/fs/xfs/xfs_trans_dquot.c +++ b/fs/xfs/xfs_trans_dquot.c @@ -596,6 +596,7 @@ xfs_dqresv_check( return QUOTA_NL_ISOFTLONGWARN; } + res->warnings++; return QUOTA_NL_ISOFTWARN; }