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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Apr 29, 2022 at 10:20:18AM -0700, Alli wrote:
> 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@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?
> 
> 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

I don't think /anyone/ needs it, but we're spending a lot of time
speculating and arguing about a poorly defined feature that has never
been satisfactorily implemented.  I broke it two LTSes ago based on my
misunderstanding (and nobody said anything during review) by making the
kernel bump the warning count, and nobody noticed until RH QA hit it.

I spoke to Jan Kara yesterday about removing the warning counts, and he
said he was fine if we simply made the VFS quotactl code return EINVAL
if you try to set the warning values, and then just remove everything
else behind that.

--D

> 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.
>