From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 25/25] xfs: scrub quota information
Date: Mon, 9 Oct 2017 13:03:28 -0700 [thread overview]
Message-ID: <20171009200328.GT7122@magnolia> (raw)
In-Reply-To: <20171009025151.GF3666@dastard>
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?
> > +/* Quotas. */
> > +
> > +/* Scrub the fields in an individual quota item. */
> > +STATIC void
> > +xfs_scrub_quota_item(
> > + struct xfs_scrub_context *sc,
> > + uint dqtype,
> > + struct xfs_dquot *dq,
> > + xfs_dqid_t id)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + struct xfs_disk_dquot *d = &dq->q_core;
> > + struct xfs_quotainfo *qi = mp->m_quotainfo;
> > + xfs_fileoff_t offset;
> > + unsigned long long bsoft;
> > + unsigned long long isoft;
> > + unsigned long long rsoft;
> > + unsigned long long bhard;
> > + unsigned long long ihard;
> > + unsigned long long rhard;
> > + unsigned long long bcount;
> > + unsigned long long icount;
> > + unsigned long long rcount;
> > + xfs_ino_t inodes;
> > +
> > + /* Did we get the dquot we wanted? */
> > + offset = id * qi->qi_dqperchunk;
> > + if (id > be32_to_cpu(d->d_id) ||
>
> Why is this a ">" check rather than "!="?
/*
* We fed id and DQNEXT into the xfs_qm_dqget call, which means
* that the actual dquot we got must either have the same id or
* the next higher id.
*/
>
> > + dqtype != (d->d_flags & XFS_DQ_ALLTYPES))
> > + xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, offset);
> > +
> > + /* Check the limits. */
> > + bhard = be64_to_cpu(d->d_blk_hardlimit);
> > + ihard = be64_to_cpu(d->d_ino_hardlimit);
> > + rhard = be64_to_cpu(d->d_rtb_hardlimit);
> > +
> > + bsoft = be64_to_cpu(d->d_blk_softlimit);
> > + isoft = be64_to_cpu(d->d_ino_softlimit);
> > + rsoft = be64_to_cpu(d->d_rtb_softlimit);
> > +
> > + inodes = XFS_AGINO_TO_INO(mp, mp->m_sb.sb_agcount, 0);
>
> Allocated inode counts should check against the filesystem inode
> limit (mp->m_maxicount) rather than the physical last inode number
> (which is wrong, anyway, for a small last AG).
Oops, ok.
> > +
> > + /*
> > + * 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? 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.
I found a softlimit/hardlimit comparison in a debugging ASSERT in
xfs_qm_adjust_dqtimers, but I didn't find anything that looked like a
direct comparison of softlimit <= hardlimit leading to a corruption
message in the xfs_qm_dqget path.
<shrug> I might be missing something here.
> > + /* Check the resource counts. */
> > + bcount = be64_to_cpu(d->d_bcount);
> > + icount = be64_to_cpu(d->d_icount);
> > + rcount = be64_to_cpu(d->d_rtbcount);
> > + inodes = percpu_counter_sum(&mp->m_icount);
>
> Can we use different variable names for "inodes" here? One is the
> maximum allowed, the other is currently allocated.
Renamed to fs_icount.
> > +/* Scrub all of a quota type's items. */
> > +int
> > +xfs_scrub_quota(
> > + struct xfs_scrub_context *sc)
> > +{
> > + struct xfs_bmbt_irec irec = { 0 };
> > + struct xfs_mount *mp = sc->mp;
> > + struct xfs_inode *ip;
> > + struct xfs_quotainfo *qi = mp->m_quotainfo;
> > + struct xfs_dquot *dq;
> > + xfs_fileoff_t max_dqid_off;
> > + xfs_fileoff_t off = 0;
> > + xfs_dqid_t id = 0;
> > + uint dqtype;
> > + int nimaps;
> > + int error;
> > +
> > + if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
> > + return -ENOENT;
> > +
> > + mutex_lock(&qi->qi_quotaofflock);
> > + dqtype = xfs_scrub_quota_to_dqtype(sc);
> > + if (!xfs_this_quota_on(sc->mp, dqtype)) {
> > + error = -ENOENT;
> > + goto out;
>
> goto out_unlock_quota
>
> > + }
> > +
> > + /* Attach to the quota inode and set sc->ip so that reporting works. */
> > + ip = xfs_quota_inode(sc->mp, dqtype);
> > + sc->ip = ip;
> > +
> > + /* Look for problem extents. */
> > + xfs_ilock(ip, XFS_ILOCK_EXCL);
> > + max_dqid_off = ((xfs_dqid_t)-1) / qi->qi_dqperchunk;
> > + while (1) {
> > + if (xfs_scrub_should_terminate(sc, &error))
> > + break;
>
> goto out_unlock_inode
>
> > +
> > + off = irec.br_startoff + irec.br_blockcount;
> > + nimaps = 1;
> > + error = xfs_bmapi_read(ip, off, -1, &irec, &nimaps,
> > + XFS_BMAPI_ENTIRE);
> > + if (!xfs_scrub_fblock_op_ok(sc, XFS_DATA_FORK, off, &error))
> > + goto out_unlock;
>
> out_unlock_inode
>
> > + if (!nimaps)
> > + break;
> > + if (irec.br_startblock == HOLESTARTBLOCK)
> > + continue;
> > +
> > + /*
> > + * Unwritten extents or blocks mapped above the highest
> > + * quota id shouldn't happen.
> > + */
> > + if (isnullstartblock(irec.br_startblock) ||
> > + irec.br_startoff > max_dqid_off ||
> > + irec.br_startoff + irec.br_blockcount > max_dqid_off + 1)
> > + xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, off);
> > + }
> > + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > +
> > + /* Check all the quota items. */
> > + while (id < ((xfs_dqid_t)-1ULL)) {
> > + if (xfs_scrub_should_terminate(sc, &error))
> > + break;
> > +
> > + error = xfs_qm_dqget(mp, NULL, id, dqtype, XFS_QMOPT_DQNEXT,
> > + &dq);
> > + if (error == -ENOENT)
> > + break;
> > + if (!xfs_scrub_fblock_op_ok(sc, XFS_DATA_FORK,
> > + id * qi->qi_dqperchunk, &error))
> > + goto out;
>
> break
>
> > +
> > + xfs_scrub_quota_item(sc, dqtype, dq, id);
> > +
> > + id = be32_to_cpu(dq->q_core.d_id) + 1;
> > + xfs_qm_dqput(dq);
> > + if (!id)
> > + break;
> > + }
>
> out_unlock_quota:
> sc->ip = NULL;
> mutex_unlock(&qi->qi_quotaofflock);
> return error;
>
> out_unlock_inode:
> xfs_iunlock(ip, XFS_ILOCK_EXCL);
> goto out_unlock_quota;
Fixed.
> }
>
> > diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> > index 348e3c3..0849b3f 100644
> > --- a/fs/xfs/scrub/scrub.c
> > +++ b/fs/xfs/scrub/scrub.c
> > @@ -256,6 +256,24 @@ static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
> > { NULL },
> > { NULL },
> > #endif
> > +#ifdef CONFIG_XFS_QUOTA
> > + { /* user quota */
> > + .setup = xfs_scrub_setup_quota,
> > + .scrub = xfs_scrub_quota,
> > + },
> > + { /* group quota */
> > + .setup = xfs_scrub_setup_quota,
> > + .scrub = xfs_scrub_quota,
> > + },
> > + { /* project quota */
> > + .setup = xfs_scrub_setup_quota,
> > + .scrub = xfs_scrub_quota,
> > + },
> > +#else
> > + { NULL },
> > + { NULL },
> > + { NULL },
> > +#endif
> > };
>
> Again, I think stub functions are in order here.
Ok, will do.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2017-10-09 20:03 UTC|newest]
Thread overview: 91+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-03 20:40 [PATCH v11 00/25] xfs: online scrub support Darrick J. Wong
2017-10-03 20:40 ` [PATCH 01/25] xfs: create an ioctl to scrub AG metadata Darrick J. Wong
2017-10-03 20:41 ` [PATCH 02/25] xfs: dispatch metadata scrub subcommands Darrick J. Wong
2017-10-03 20:41 ` [PATCH 03/25] xfs: probe the scrub ioctl Darrick J. Wong
2017-10-03 23:32 ` Dave Chinner
2017-10-04 0:02 ` Darrick J. Wong
2017-10-04 1:56 ` Dave Chinner
2017-10-04 3:14 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 04/25] xfs: create helpers to record and deal with scrub problems Darrick J. Wong
2017-10-03 23:44 ` Dave Chinner
2017-10-04 0:56 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 05/25] xfs: create helpers to scrub a metadata btree Darrick J. Wong
2017-10-03 23:49 ` Dave Chinner
2017-10-04 0:13 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 06/25] xfs: scrub the shape of " Darrick J. Wong
2017-10-04 0:15 ` Dave Chinner
2017-10-04 3:51 ` Darrick J. Wong
2017-10-04 5:48 ` Dave Chinner
2017-10-04 17:48 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 07/25] xfs: scrub btree keys and records Darrick J. Wong
2017-10-04 20:52 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 08/25] xfs: create helpers to scan an allocation group Darrick J. Wong
2017-10-04 0:46 ` Dave Chinner
2017-10-04 3:58 ` Darrick J. Wong
2017-10-04 5:59 ` Dave Chinner
2017-10-04 17:51 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 09/25] xfs: scrub the backup superblocks Darrick J. Wong
2017-10-04 0:57 ` Dave Chinner
2017-10-04 4:06 ` Darrick J. Wong
2017-10-04 6:13 ` Dave Chinner
2017-10-04 17:56 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 10/25] xfs: scrub AGF and AGFL Darrick J. Wong
2017-10-04 1:31 ` Dave Chinner
2017-10-04 4:21 ` Darrick J. Wong
2017-10-04 6:28 ` Dave Chinner
2017-10-04 17:57 ` Darrick J. Wong
2017-10-03 20:41 ` [PATCH 11/25] xfs: scrub the AGI Darrick J. Wong
2017-10-04 1:43 ` Dave Chinner
2017-10-04 4:25 ` Darrick J. Wong
2017-10-04 6:43 ` Dave Chinner
2017-10-04 18:02 ` Darrick J. Wong
2017-10-04 22:16 ` Dave Chinner
2017-10-04 23:12 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 12/25] xfs: scrub free space btrees Darrick J. Wong
2017-10-05 0:59 ` Dave Chinner
2017-10-05 1:13 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 13/25] xfs: scrub inode btrees Darrick J. Wong
2017-10-05 2:08 ` Dave Chinner
2017-10-05 5:47 ` Darrick J. Wong
2017-10-05 7:22 ` Dave Chinner
2017-10-05 18:26 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 14/25] xfs: scrub rmap btrees Darrick J. Wong
2017-10-05 2:56 ` Dave Chinner
2017-10-05 5:02 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 15/25] xfs: scrub refcount btrees Darrick J. Wong
2017-10-05 2:59 ` Dave Chinner
2017-10-05 5:02 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 16/25] xfs: scrub inodes Darrick J. Wong
2017-10-05 4:04 ` Dave Chinner
2017-10-05 5:22 ` Darrick J. Wong
2017-10-05 7:13 ` Dave Chinner
2017-10-05 19:56 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 17/25] xfs: scrub inode block mappings Darrick J. Wong
2017-10-06 2:51 ` Dave Chinner
2017-10-06 17:00 ` Darrick J. Wong
2017-10-07 23:10 ` Dave Chinner
2017-10-08 3:54 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 18/25] xfs: scrub directory/attribute btrees Darrick J. Wong
2017-10-06 5:07 ` Dave Chinner
2017-10-06 18:30 ` Darrick J. Wong
2017-10-03 20:42 ` [PATCH 19/25] xfs: scrub directory metadata Darrick J. Wong
2017-10-06 7:07 ` Dave Chinner
2017-10-06 19:45 ` Darrick J. Wong
2017-10-06 22:16 ` Dave Chinner
2017-10-03 20:42 ` [PATCH 20/25] xfs: scrub directory freespace Darrick J. Wong
2017-10-09 1:44 ` Dave Chinner
2017-10-09 22:54 ` Darrick J. Wong
2017-10-03 20:43 ` [PATCH 21/25] xfs: scrub extended attributes Darrick J. Wong
2017-10-09 2:13 ` Dave Chinner
2017-10-09 21:14 ` Darrick J. Wong
2017-10-03 20:43 ` [PATCH 22/25] xfs: scrub symbolic links Darrick J. Wong
2017-10-09 2:17 ` Dave Chinner
2017-10-03 20:43 ` [PATCH 23/25] xfs: scrub parent pointers Darrick J. Wong
2017-10-03 20:43 ` [PATCH 24/25] xfs: scrub realtime bitmap/summary Darrick J. Wong
2017-10-09 2:28 ` Dave Chinner
2017-10-09 20:24 ` Darrick J. Wong
2017-10-03 20:43 ` [PATCH 25/25] xfs: scrub quota information Darrick J. Wong
2017-10-09 2:51 ` Dave Chinner
2017-10-09 20:03 ` Darrick J. Wong [this message]
2017-10-09 22:17 ` Dave Chinner
2017-10-09 23:08 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171009200328.GT7122@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).