From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 10/25] xfs: scrub AGF and AGFL
Date: Tue, 3 Oct 2017 21:21:40 -0700 [thread overview]
Message-ID: <20171004042140.GN6503@magnolia> (raw)
In-Reply-To: <20171004013148.GW3666@dastard>
On Wed, Oct 04, 2017 at 12:31:48PM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 01:41:52PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Check the block references in the AGF and AGFL headers to make sure
> > they make sense.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > fs/xfs/libxfs/xfs_fs.h | 4 +
> > fs/xfs/scrub/agheader.c | 220 +++++++++++++++++++++++++++++++++++++++++++++++
> > fs/xfs/scrub/common.c | 60 +++++++++++++
> > fs/xfs/scrub/common.h | 6 +
> > fs/xfs/scrub/scrub.c | 8 ++
> > fs/xfs/scrub/scrub.h | 2
> > 6 files changed, 299 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index 8543cbb..aeb2a66 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -485,9 +485,11 @@ struct xfs_scrub_metadata {
> > /* Scrub subcommands. */
> > #define XFS_SCRUB_TYPE_PROBE 0 /* presence test ioctl */
> > #define XFS_SCRUB_TYPE_SB 1 /* superblock */
> > +#define XFS_SCRUB_TYPE_AGF 2 /* AG free header */
> > +#define XFS_SCRUB_TYPE_AGFL 3 /* AG free list */
> >
> > /* Number of scrub subcommands. */
> > -#define XFS_SCRUB_TYPE_NR 2
> > +#define XFS_SCRUB_TYPE_NR 4
> >
> > /* i: Repair this metadata. */
> > #define XFS_SCRUB_IFLAG_REPAIR (1 << 0)
> > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> > index 487c4f4..7fe6630 100644
> > --- a/fs/xfs/scrub/agheader.c
> > +++ b/fs/xfs/scrub/agheader.c
> > @@ -49,6 +49,72 @@ xfs_scrub_setup_ag_header(
> > return xfs_scrub_setup_fs(sc, ip);
> > }
> >
> > +/* Find the size of the AG, in blocks. */
> > +static inline xfs_agblock_t
> > +xfs_scrub_ag_blocks(
> > + struct xfs_mount *mp,
> > + xfs_agnumber_t agno)
> > +{
> > + ASSERT(agno < mp->m_sb.sb_agcount);
> > +
> > + if (agno < mp->m_sb.sb_agcount - 1)
> > + return mp->m_sb.sb_agblocks;
> > + return mp->m_sb.sb_dblocks - (agno * mp->m_sb.sb_agblocks);
> > +}
>
> Can you make this a generic libxfs function, say xfs_get_ag_blocks()?
> This same calculation is repeated in quite a few places, especially
> in userspace...
Ok.
> > +
> > +/* Walk all the blocks in the AGFL. */
> > +int
> > +xfs_scrub_walk_agfl(
> > + struct xfs_scrub_context *sc,
> > + int (*fn)(struct xfs_scrub_context *,
> > + xfs_agblock_t bno, void *),
> > + void *priv)
> > +{
> > + struct xfs_agf *agf;
> > + __be32 *agfl_bno;
> > + struct xfs_mount *mp = sc->mp;
> > + unsigned int flfirst;
> > + unsigned int fllast;
> > + int i;
> > + int error;
> > +
> > + agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> > + agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, sc->sa.agfl_bp);
> > + flfirst = be32_to_cpu(agf->agf_flfirst);
> > + fllast = be32_to_cpu(agf->agf_fllast);
> > +
> > + /* Skip an empty AGFL. */
> > + if (agf->agf_flcount == cpu_to_be32(0))
> > + return 0;
>
> Check flfirst -> fllast == flcount.
<nod>
> ....
>
> > +/* Scrub the AGF. */
> > +int
> > +xfs_scrub_agf(
> > + struct xfs_scrub_context *sc)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + struct xfs_agf *agf;
> > + xfs_daddr_t daddr;
> > + xfs_daddr_t eofs;
> > + xfs_agnumber_t agno;
> > + xfs_agblock_t agbno;
> > + xfs_agblock_t eoag;
> > + xfs_agblock_t agfl_first;
> > + xfs_agblock_t agfl_last;
> > + xfs_agblock_t agfl_count;
> > + xfs_agblock_t fl_count;
> > + int level;
> > + int error = 0;
> > +
> > + agno = sc->sm->sm_agno;
> > + error = xfs_scrub_load_ag_headers(sc, agno, XFS_SCRUB_TYPE_AGF);
> > + if (!xfs_scrub_op_ok(sc, agno, XFS_AGF_BLOCK(sc->mp), &error))
> > + goto out;
> > +
> > + agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> > + eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
> > +
> > + /* Check the AG length */
> > + eoag = be32_to_cpu(agf->agf_length);
> > + if (eoag != xfs_scrub_ag_blocks(mp, agno))
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + /* Check the AGF btree roots and levels */
> > + agbno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
> > + daddr = XFS_AGB_TO_DADDR(mp, agno, agbno);
> > + if (agbno <= XFS_AGI_BLOCK(mp) || agbno >= mp->m_sb.sb_agblocks ||
>
> I'm assuming that you are checking that the block isn't part of the
> static metadata range with this XFS_AGI_BLOCK() check? Shouldn't it
> actually be agbno <= XFS_AGFL_BLOCK(mp) i.e. the AGFL block address?
D'oh! Yes.
> I think we need a generic "verify agbno" function. These checks seem
> to be open coded throughout the code instead calling a single
> function that does all the checks. The short btree pointers can use
> it as well...
>
> > + agbno >= eoag || daddr >= eofs)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + agbno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_CNT]);
> > + daddr = XFS_AGB_TO_DADDR(mp, agno, agbno);
> > + if (agbno <= XFS_AGI_BLOCK(mp) || agbno >= mp->m_sb.sb_agblocks ||
> > + agbno >= eoag || daddr >= eofs)
>
> There's another.
>
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + level = be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]);
> > + if (level <= 0 || level > XFS_BTREE_MAXLEVELS)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + level = be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]);
> > + if (level <= 0 || level > XFS_BTREE_MAXLEVELS)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + if (xfs_sb_version_hasrmapbt(&mp->m_sb)) {
> > + agbno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
> > + daddr = XFS_AGB_TO_DADDR(mp, agno, agbno);
> > + if (agbno <= XFS_AGI_BLOCK(mp) ||
> > + agbno >= mp->m_sb.sb_agblocks ||
> > + agbno >= eoag ||
> > + daddr >= eofs)
>
> And another.
>
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + level = be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]);
> > + if (level <= 0 || level > XFS_BTREE_MAXLEVELS)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > + }
> > +
> > + if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> > + agbno = be32_to_cpu(agf->agf_refcount_root);
> > + daddr = XFS_AGB_TO_DADDR(mp, agno, agbno);
> > + if (agbno <= XFS_AGI_BLOCK(mp) ||
> > + agbno >= mp->m_sb.sb_agblocks ||
> > + agbno >= eoag ||
> > + daddr >= eofs)
>
> And another.
Yes I see your point, I'll add some helpers to check that something
hasn't gone off the end of the AG or the FS.
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > +
> > + level = be32_to_cpu(agf->agf_refcount_level);
> > + if (level <= 0 || level > XFS_BTREE_MAXLEVELS)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
> > + }
> > +
> > + /* Check the AGFL counters */
> > + agfl_first = be32_to_cpu(agf->agf_flfirst);
> > + agfl_last = be32_to_cpu(agf->agf_fllast);
> > + agfl_count = be32_to_cpu(agf->agf_flcount);
> > + if (agfl_last > agfl_first)
> > + fl_count = agfl_last - agfl_first + 1;
> > + else
> > + fl_count = XFS_AGFL_SIZE(mp) - agfl_first + agfl_last + 1;
> > + if (agfl_count != 0 && fl_count != agfl_count)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
>
> Oh, the agfl counts are checked here. Maybe put a comment in
> xfs_scrub_walk_agfl() to mention this?
Ok.
>
> .....
>
> > +struct xfs_scrub_agfl {
> > + xfs_agblock_t eoag;
> > + xfs_daddr_t eofs;
> > +};
> > +
> > +/* Scrub an AGFL block. */
> > +STATIC int
> > +xfs_scrub_agfl_block(
> > + struct xfs_scrub_context *sc,
> > + xfs_agblock_t agbno,
> > + void *priv)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + xfs_agnumber_t agno = sc->sa.agno;
> > + struct xfs_scrub_agfl *sagfl = priv;
> > + int error = 0;
> > +
> > + if (agbno <= XFS_AGI_BLOCK(mp) ||
> > + agbno >= mp->m_sb.sb_agblocks ||
> > + agbno >= sagfl->eoag ||
> > + XFS_AGB_TO_DADDR(mp, agno, agbno) >= sagfl->eofs)
> > + xfs_scrub_block_set_corrupt(sc, sc->sa.agfl_bp);
> > +
> > + return error;
> > +}
>
> Oh, look, there's another xfs_agbno_verify() function call :P
>
> .....
> > diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> > index b056c9d..ee8e7be 100644
> > --- a/fs/xfs/scrub/common.c
> > +++ b/fs/xfs/scrub/common.c
> > @@ -471,6 +471,66 @@ xfs_scrub_ag_init(
> > return xfs_scrub_ag_btcur_init(sc, sa);
> > }
> >
> > +/*
> > + * Load and verify an AG header for further AG header examination.
> > + * If this header is not the target of the examination, don't return
> > + * the buffer if a runtime or verifier error occurs.
> > + */
> > +STATIC int
> > +xfs_scrub_load_ag_header(
> > + struct xfs_scrub_context *sc,
> > + xfs_daddr_t daddr,
> > + struct xfs_buf **bpp,
> > + const struct xfs_buf_ops *ops,
> > + bool is_target)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + int error;
> > +
> > + *bpp = NULL;
> > + error = xfs_trans_read_buf(mp, sc->tp, mp->m_ddev_targp,
> > + XFS_AG_DADDR(mp, sc->sa.agno, daddr),
> > + XFS_FSS_TO_BB(mp, 1), 0, bpp, ops);
> > + return is_target ? error : 0;
> > +}
> > +
> > +/*
> > + * Load as many of the AG headers and btree cursors as we can for an
> > + * examination and cross-reference of an AG header.
> > + */
> > +int
> > +xfs_scrub_load_ag_headers(
> > + struct xfs_scrub_context *sc,
> > + xfs_agnumber_t agno,
> > + unsigned int type)
> > +{
> > + struct xfs_mount *mp = sc->mp;
> > + int error;
> > +
> > + ASSERT(type == XFS_SCRUB_TYPE_AGF || type == XFS_SCRUB_TYPE_AGFL);
> > + memset(&sc->sa, 0, sizeof(sc->sa));
> > + sc->sa.agno = agno;
> > +
> > + error = xfs_scrub_load_ag_header(sc, XFS_AGI_DADDR(mp),
> > + &sc->sa.agi_bp, &xfs_agi_buf_ops, false);
> > + if (error)
> > + return error;
> > +
> > + error = xfs_scrub_load_ag_header(sc, XFS_AGF_DADDR(mp),
> > + &sc->sa.agf_bp, &xfs_agf_buf_ops,
> > + type == XFS_SCRUB_TYPE_AGF);
> > + if (error)
> > + return error;
> > +
> > + error = xfs_scrub_load_ag_header(sc, XFS_AGFL_DADDR(mp),
> > + &sc->sa.agfl_bp, &xfs_agfl_buf_ops,
> > + type == XFS_SCRUB_TYPE_AGFL);
> > + if (error)
> > + return error;
> > +
> > + return 0;
> > +}
>
> This should probably be combined with xfs_scrub_ag_read_headers().
> They essentially do the same thing, the only difference is the
> "target" error reporting.
It's quite different -- this function ignores verifier errors for
the two headers that don't match 'type' In other words, if we're
checking the AGF (for example) we'll try to grab the AGI and the AGFL.
Verifier errors on the AGI/AGFL don't matter, but we /do/ want to hear
the results if the AGF verifier fails.
xfs_scrub_ag_read_headers on the other hand will fail if /any/ of the
three verifiers fail. We want this behavior for the btree scrubbers so
that we can bail out with an operational error if the headers are bad,
but we don't want this behavior for the header scrubbers because an AGI
verifier error can cause the AGF verifier to report corruption.
Later on, repair will want the perag stuff loaded (which
xfs_scrub_load_ag_headers doesn't do), fwiw.
The two functions /could/ be combined, though the 'type' test becomes
trickier. Maybe it'd be better just to enhance the comments for the two
header loader functions to spell out how they differ in usage.
--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-04 4:21 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 [this message]
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
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=20171004042140.GN6503@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.