On Tue, Oct 10, 2017 at 09:17:51AM +1100, Dave Chinner wrote:
> 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?
There isn't a generic callback in the teardown.
> > > > + /*
> > > > + * 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.
Ah, ok, I get it now, thanks.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
