| Message ID | 150706331250.19351.3004733994525860282.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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... > + > +/* 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. .... > +/* 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? 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. > + 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? ..... > +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. Cheers, Dave.
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 -- 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
On Tue, Oct 03, 2017 at 09:21:40PM -0700, Darrick J. Wong wrote: > 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: > > > +/* > > > + * 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. What they do is quite different. The implementation is /almost/ identical. type is just an error masking variable and .... > xfs_scrub_ag_read_headers on the other hand will fail if /any/ of the > three verifiers fail. .... if no type is set, then we don't mask any errors at all and we bail if any of the three verifiers fail. i.e.: error = xfs_ialloc_read_agi(mp, sc->tp, agno, agi); if (error && (!type || type == XFS_SCRUB_TYPE_AGI) return error; error = xfs_alloc_read_agf(mp, sc->tp, agno, 0, agf); if (error && (!type || type == XFS_SCRUB_TYPE_AGF) return error; error = xfs_alloc_read_agfl(mp, sc->tp, agno, agfl); if (error && (!type || type == XFS_SCRUB_TYPE_AGFL) return error; It's also much simpler to understand because we are using the proper functions for reading these headers.... > 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. Again, I'd much prefer similar functionality is combined into common helpers if it's simple enough to do... Cheers, Dave.
On Wed, Oct 04, 2017 at 05:28:40PM +1100, Dave Chinner wrote: > On Tue, Oct 03, 2017 at 09:21:40PM -0700, Darrick J. Wong wrote: > > 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: > > > > +/* > > > > + * 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. > > What they do is quite different. The implementation is /almost/ > identical. type is just an error masking variable and .... > > > xfs_scrub_ag_read_headers on the other hand will fail if /any/ of the > > three verifiers fail. > > .... if no type is set, then we don't mask any errors at all and > we bail if any of the three verifiers fail. i.e.: > > error = xfs_ialloc_read_agi(mp, sc->tp, agno, agi); > if (error && (!type || type == XFS_SCRUB_TYPE_AGI) > return error; > > error = xfs_alloc_read_agf(mp, sc->tp, agno, 0, agf); > if (error && (!type || type == XFS_SCRUB_TYPE_AGF) > return error; > > error = xfs_alloc_read_agfl(mp, sc->tp, agno, agfl); > if (error && (!type || type == XFS_SCRUB_TYPE_AGFL) > return error; > > It's also much simpler to understand because we are using the proper > functions for reading these headers.... Ok, sounds good to me. > > 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. > > Again, I'd much prefer similar functionality is combined into > common helpers if it's simple enough to do... <nod> --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 -- 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
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); +} + +/* 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; + + /* first to last is a consecutive list. */ + if (fllast >= flfirst) { + for (i = flfirst; i <= fllast; i++) { + error = fn(sc, be32_to_cpu(agfl_bno[i]), priv); + if (error) + return error; + } + + return 0; + } + + /* first to the end */ + for (i = flfirst; i < XFS_AGFL_SIZE(mp); i++) { + error = fn(sc, be32_to_cpu(agfl_bno[i]), priv); + if (error) + return error; + } + + /* the start to last. */ + for (i = 0; i <= fllast; i++) { + error = fn(sc, be32_to_cpu(agfl_bno[i]), priv); + if (error) + return error; + } + + return 0; +} + /* Superblock */ /* Scrub the filesystem superblock. */ @@ -315,3 +381,157 @@ xfs_scrub_superblock( return error; } + +/* AGF */ + +/* 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 || + 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) + 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) + 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) + 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); + +out: + return error; +} + +/* AGFL */ + +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; +} + +/* Scrub the AGFL. */ +int +xfs_scrub_agfl( + struct xfs_scrub_context *sc) +{ + struct xfs_scrub_agfl sagfl; + struct xfs_mount *mp = sc->mp; + struct xfs_agf *agf; + xfs_agnumber_t agno; + int error; + + agno = sc->sm->sm_agno; + error = xfs_scrub_load_ag_headers(sc, agno, XFS_SCRUB_TYPE_AGFL); + if (!xfs_scrub_op_ok(sc, agno, XFS_AGFL_BLOCK(sc->mp), &error)) + goto out; + if (!sc->sa.agf_bp) + return -EFSCORRUPTED; + + agf = XFS_BUF_TO_AGF(sc->sa.agf_bp); + sagfl.eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks); + sagfl.eoag = be32_to_cpu(agf->agf_length); + + /* Check the blocks in the AGFL. */ + return xfs_scrub_walk_agfl(sc, xfs_scrub_agfl_block, &sagfl); +out: + return error; +} 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; +} + /* Per-scrubber setup functions */ /* Set us up with a transaction and an empty context. */ diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h index 390f772..4d8bb72 100644 --- a/fs/xfs/scrub/common.h +++ b/fs/xfs/scrub/common.h @@ -88,5 +88,11 @@ int xfs_scrub_ag_read_headers(struct xfs_scrub_context *sc, xfs_agnumber_t agno, void xfs_scrub_ag_btcur_free(struct xfs_scrub_ag *sa); int xfs_scrub_ag_btcur_init(struct xfs_scrub_context *sc, struct xfs_scrub_ag *sa); +int xfs_scrub_load_ag_headers(struct xfs_scrub_context *sc, xfs_agnumber_t agno, + unsigned int type); +int xfs_scrub_walk_agfl(struct xfs_scrub_context *sc, + int (*fn)(struct xfs_scrub_context *, xfs_agblock_t bno, + void *), + void *priv); #endif /* __XFS_SCRUB_COMMON_H__ */ diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c index 7f6f997..6f3c4f0 100644 --- a/fs/xfs/scrub/scrub.c +++ b/fs/xfs/scrub/scrub.c @@ -157,6 +157,14 @@ static const struct xfs_scrub_meta_ops meta_scrub_ops[] = { .setup = xfs_scrub_setup_ag_header, .scrub = xfs_scrub_superblock, }, + { /* agf */ + .setup = xfs_scrub_setup_ag_header, + .scrub = xfs_scrub_agf, + }, + { /* agfl */ + .setup = xfs_scrub_setup_ag_header, + .scrub = xfs_scrub_agfl, + }, }; /* This isn't a stable feature, warn once per day. */ diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h index 13e3f9b..50f8641 100644 --- a/fs/xfs/scrub/scrub.h +++ b/fs/xfs/scrub/scrub.h @@ -68,5 +68,7 @@ struct xfs_scrub_context { /* Metadata scrubbers */ int xfs_scrub_tester(struct xfs_scrub_context *sc); int xfs_scrub_superblock(struct xfs_scrub_context *sc); +int xfs_scrub_agf(struct xfs_scrub_context *sc); +int xfs_scrub_agfl(struct xfs_scrub_context *sc); #endif /* __XFS_SCRUB_SCRUB_H__ */