Re: [RFC PATCH v1 2/2] xfs: don't set warns on the id==0 dquot

[Dave Chinner <david@fromorbit.com>]

On Mon, Apr 25, 2022 at 03:34:52PM +0000, Catherine Hoang wrote:
> > On Apr 21, 2022, at 6:40 PM, Dave Chinner <david@fromorbit.com> 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@oracle.com>
> >> ---
> >> 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?

> 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.
-- 
Dave Chinner
david@fromorbit.com