Re: [PATCH 25/25] xfs: scrub quota information

[Dave Chinner <david@fromorbit.com>]

On Mon, Oct 09, 2017 at 01:03:28PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 09, 2017 at 01:51:51PM +1100, Dave Chinner wrote:
> > On Tue, Oct 03, 2017 at 01:43:27PM -0700, Darrick J. Wong wrote:
> > > +xfs_scrub_quota_to_dqtype(
> > > +	struct xfs_scrub_context	*sc)
> > > +{
> > > +	switch (sc->sm->sm_type) {
> > > +	case XFS_SCRUB_TYPE_UQUOTA:
> > > +		return XFS_DQ_USER;
> > > +	case XFS_SCRUB_TYPE_GQUOTA:
> > > +		return XFS_DQ_GROUP;
> > > +	case XFS_SCRUB_TYPE_PQUOTA:
> > > +		return XFS_DQ_PROJ;
> > > +	default:
> > > +		return 0;
> > > +	}
> > > +}
> > > +
> > > +/* Set us up to scrub a quota. */
> > > +int
> > > +xfs_scrub_setup_quota(
> > > +	struct xfs_scrub_context	*sc,
> > > +	struct xfs_inode		*ip)
> > > +{
> > > +	uint				dqtype;
> > > +
> > > +	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> > > +		return -EINVAL;
> > > +
> > > +	dqtype = xfs_scrub_quota_to_dqtype(sc);
> > > +	if (dqtype == 0)
> > > +		return -EINVAL;
> > > +	return 0;
> > > +}
> > 
> > Should this check whether the quota type is actually enabled, and
> > return ENOENT if it's not? i.e move the check out of
> > xfs_scrub_quota() and into the setup function?
> 
> I can add a xfs_this_quota_on check to the setup function, but don't we
> need xfs_scrub_quota to lock qi_quotaofflock and then recheck that the
> quota type is still enabled?

The qi_quotaofflock is held across the entire scrub, right?

Ah, this is called before the qi_quotaofflock is held - is there
a teardown callback? If so, would it be better to lock
qi_quotaofflock in the setup, release it in the teardown? That way
we can check here in the setup code, and not have to double check
the user input it in the scrub function itself?

> > > +	/*
> > > +	 * Warn if the limits are larger than the fs.  Administrators
> > > +	 * can do this, though in production this seems suspect.
> > > +	 */
> > > +	if (bhard > mp->m_sb.sb_dblocks || bsoft > mp->m_sb.sb_dblocks)
> > > +		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> > > +	if (ihard > inodes || isoft > inodes)
> > > +		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> > > +	if (rhard > mp->m_sb.sb_rblocks || rsoft > mp->m_sb.sb_rblocks)
> > > +		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> > 
> > Can you stack these so there's one per line? i.e.:
> 
> Will do.  I'll also change the ihard/isoft check here to check against
> mp->m_maxicount directly.
> 
> > 	if (bhard > mp->m_sb.sb_dblocks ||
> > 	    bsoft > mp->m_sb.sb_dblocks)
> > 		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
> > 
> > > +
> > > +	/* Soft limit must be less than the hard limit. */
> > > +	if (bsoft > bhard || isoft > ihard || rsoft > rhard)
> > > +		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
> > 
> > Though with this check, I suspect you only need to check the hard
> > limits against their upper limits because if the hard limit is valid
> > and the soft is above then it's going to trigger corruption. Do we
> > need a warning as well in that case?
> 
> I don't follow here ... if the soft limit is above the hard limit, what
> will trigger corruption? 

What I mean was this:

	if (bhard > mp->m_sb.sb_dblocks)
		xfs_scrub_fblock_set_warning(sc, XFS_DATA_FORK, offset);
	if (bsoft > bhard)
		xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);

That is, if bhard is over the valid limit, throw a warning and we
now don't care about bsoft because we've already signalled to
userspace there's a problem.

If bsoft is now greater than bhard, regardless of whether it's over
the valid size, we signal a corruption. If bhard is already an
invalid number, then this corruption report also implies that bsoft
is over the limit...

> The quota syscalls enforce that bhard > bsoft
> when the admin tries to set new limits, but if the disk is corrupt such
> that dblocks = 300, bhard = 250, and bsoft = 280, the checks for
> (bhard > dblocks || bsoft > dblocks) checks won't trigger.  That's why
> there's an explicit bsoft > bhard check here.

Right, I wasn't suggesting removing it - I didn't make it very clear
what I meant.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com