On Tue, 2022-04-26 at 08:42 +1000, Dave Chinner wrote: > On Mon, Apr 25, 2022 at 03:34:52PM +0000, Catherine Hoang wrote: > > > On Apr 21, 2022, at 6:40 PM, Dave Chinner <david@xxxxxxxxxxxxx> > > > wrote: > > > > > > On Thu, Apr 21, 2022 at 09:58:15AM -0700, Catherine Hoang wrote: > > > > Quotas are not enforced on the id==0 dquot, so the quota code > > > > uses it > > > > to store warning limits and timeouts. Having just dropped > > > > support for > > > > warning limits, this field no longer has any meaning. Return > > > > -EINVAL > > > > for this dquot id if the fieldmask has any of the QC_*_WARNS > > > > set. > > > > > > > > Signed-off-by: Catherine Hoang <catherine.hoang@xxxxxxxxxx> > > > > --- > > > > fs/xfs/xfs_qm_syscalls.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/fs/xfs/xfs_qm_syscalls.c > > > > b/fs/xfs/xfs_qm_syscalls.c > > > > index e7f3ac60ebd9..bdbd5c83b08e 100644 > > > > --- a/fs/xfs/xfs_qm_syscalls.c > > > > +++ b/fs/xfs/xfs_qm_syscalls.c > > > > @@ -290,6 +290,8 @@ xfs_qm_scall_setqlim( > > > > return -EINVAL; > > > > if ((newlim->d_fieldmask & XFS_QC_MASK) == 0) > > > > return 0; > > > > + if ((newlim->d_fieldmask & QC_WARNS_MASK) && id == 0) > > > > + return -EINVAL; > > > > > > Why would we do this for only id == 0? This will still allow > > > non-zero warning values to be written to dquots that have id != > > > 0, > > > but I'm not sure why we'd allow this given that the previous > > > patch just removed all the warning limit checking? > > > > > > Which then makes me ask: why are we still reading the warning > > > counts > > > from on disk dquots and writing in-memory values back to dquots? > > > Shouldn't xfs_dquot_to_disk() just write zeros to the warning > > > fields > > > now, and xfs_dquot_from_disk() elide reading the warning counts > > > altogether? i.e. can we remove d_bwarns, d_iwarns and d_rtbwarns > > > from the struct fs_disk_quota altogether now? > > > > > > Which then raises the question of whether copy_from_xfs_dqblk() > > > and > > > friends should still support warn counts up in > > > fs/quota/quota.c...? > > > > The intent of this patchset is to only remove warning limits, not > > warning > > counts. The id == 0 dquot stores warning limits, so we no longer > > want > > warning values to be written here. Dquots with id != 0 store > > warning > > counts, so we still allow these values to be updated. > > Why? What uses the value? After this patchset the warning count is > not used anywhere in the kernel. Absent in-kernel enforced limits, > the warning count fundamentally useless as a userspace decision tool > because of the warning count is not deterministic in any way. > Nothing can be inferred about the absolute value of the count > because it can't be related back to individual user operations. > > i.e. A single write() can get different numbers of warnings issuedi > simply because of file layout or inode config (e.g. DIO vs buffered, > writes into sparse regions, unwritten regions, if extent size hints > are enabled, etc) and so the absolute magnitude of the warning count > doesn't carry any significant meaning. > > I still haven't seen a use case for these quota warning counts, > either with or without the limiting code. Who needs this, and why? Hmm, I wonder if this is eluding to the idea of just removing the function all together then? And replacing it with a -EINVAL or maybe -EOPNOTSUPP? I only see a few calls to it in xfs_quotaops.c Allison > > > In regards to whether or not warning counts should be removed, it > > seems > > like they currently do serve a purpose. Although there's some > > debate about > > this in a previous thread: > > https://lore.kernel.org/linux-xfs/20220303003808.GM117732@magnolia/ > > Once the warning count increment is removed, there are zero users of > the warning counts. > > Legacy functionality policy (as defined by ALLOCSP removal) kicks in > here: remove anything questionable ASAP, before it bites us hard. > And this has already bitten us real hard... > > Cheers, > > Dave.
