On Fri, Dec 22, 2017 at 04:44:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> When we're scrubbing various btrees, cross-reference the records with
> the bnobt to ensure that we don't also think the space is free.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/agheader.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++
> fs/xfs/scrub/alloc.c | 19 +++++++++++
> fs/xfs/scrub/bmap.c | 21 +++++++++++-
> fs/xfs/scrub/btree.c | 12 +++++++
> fs/xfs/scrub/ialloc.c | 1 +
> fs/xfs/scrub/inode.c | 15 ++++++++
> fs/xfs/scrub/refcount.c | 1 +
> fs/xfs/scrub/rmap.c | 4 ++
> fs/xfs/scrub/scrub.h | 5 +++
> 9 files changed, 161 insertions(+), 1 deletion(-)
>
>
> diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> index 5be9059..3bb0f96 100644
> --- a/fs/xfs/scrub/agheader.c
> +++ b/fs/xfs/scrub/agheader.c
> @@ -107,6 +107,20 @@ xfs_scrub_superblock_xref(
> struct xfs_scrub_context *sc,
> struct xfs_buf *bp)
> {
> + struct xfs_mount *mp = sc->mp;
> + xfs_agnumber_t agno = sc->sm->sm_agno;
> + xfs_agblock_t bno;
> + int error;
> +
> + bno = XFS_SB_BLOCK(mp);
agbno
> +
> + error = xfs_scrub_ag_init(sc, agno, &sc->sa);
> + if (!xfs_scrub_xref_process_error(sc, agno, bno, &error))
> + return;
> +
> + xfs_scrub_xref_not_free(sc, &sc->sa.bno_cur, bno, 1);
"xref_not_free". That doesn't tell me what it's actually checking
is not free, or what it's checking against. Seeing as we're checking
these all against the bnobt, wouldn't something like this be more
obvious:
xfs_scrub_is_used_space(sc, agbno, 1);
> + /* scrub teardown will take care of sc->sa for us */
> }
>
> /*
> @@ -407,11 +421,51 @@ xfs_scrub_superblock(
>
> /* AGF */
>
> +/* Tally freespace record lengths. */
> +STATIC int
> +xfs_scrub_agf_record_bno_lengths(
> + struct xfs_btree_cur *cur,
> + struct xfs_alloc_rec_incore *rec,
> + void *priv)
> +{
> + xfs_extlen_t *blocks = priv;
> +
> + (*blocks) += rec->ar_blockcount;
> + return 0;
> +}
> +
> /* Cross-reference with the other btrees. */
> STATIC void
> xfs_scrub_agf_xref(
> struct xfs_scrub_context *sc)
> {
> + struct xfs_mount *mp = sc->mp;
> + struct xfs_agf *agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
> + struct xfs_btree_cur **pcur;
> + xfs_agblock_t bno;
> + xfs_extlen_t blocks;
> + int error;
> +
> + bno = XFS_AGF_BLOCK(mp);
agbno. (and for all the others)
> +
> + error = xfs_scrub_ag_btcur_init(sc, &sc->sa);
> + if (error)
> + return;
> +
> + xfs_scrub_xref_not_free(sc, &sc->sa.bno_cur, bno, 1);
> +
> + /* Check agf_freeblks */
> + pcur = &sc->sa.bno_cur;
> + if (*pcur) {
> + blocks = 0;
> + error = xfs_alloc_query_all(*pcur,
> + xfs_scrub_agf_record_bno_lengths, &blocks);
> + if (xfs_scrub_should_xref(sc, &error, pcur) &&
> + blocks != be32_to_cpu(agf->agf_freeblks))
> + xfs_scrub_block_xref_set_corrupt(sc, sc->sa.agf_bp);
> + }
I have no idea what xfs_scrub_should_xref() means in this context.
We're doing a xref scrub, so why are we asking if we should be
running a xref?
> diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c
> index 0d95b84..3d6f8cc 100644
> --- a/fs/xfs/scrub/alloc.c
> +++ b/fs/xfs/scrub/alloc.c
> @@ -114,3 +114,22 @@ xfs_scrub_cntbt(
> {
> return xfs_scrub_allocbt(sc, XFS_BTNUM_CNT);
> }
> +
> +/* xref check that the extent is not free */
> +void
> +xfs_scrub_xref_not_free(
> + struct xfs_scrub_context *sc,
> + struct xfs_btree_cur **pcur,
> + xfs_agblock_t bno,
> + xfs_extlen_t len)
> +{
> + bool is_freesp;
> + int error;
> +
> + if (!(*pcur))
> + return;
> +
> + error = xfs_alloc_has_record(*pcur, bno, len, &is_freesp);
> + if (xfs_scrub_should_xref(sc, &error, pcur) && is_freesp)
> + xfs_scrub_btree_xref_set_corrupt(sc, *pcur, 0);
> +}
Again, "should_xref" doesn't actually tell me anything about what
this function is doing...
> @@ -117,6 +117,25 @@ xfs_scrub_bmap_extent_xref(
> struct xfs_btree_cur *cur,
> struct xfs_bmbt_irec *irec)
> {
> + struct xfs_scrub_ag sa = { 0 };
> + struct xfs_mount *mp = info->sc->mp;
> + xfs_agnumber_t agno;
> + xfs_agblock_t agbno;
> + xfs_extlen_t len;
> + int error;
> +
> + agno = XFS_FSB_TO_AGNO(mp, irec->br_startblock);
> + agbno = XFS_FSB_TO_AGBNO(mp, irec->br_startblock);
> + len = irec->br_blockcount;
> +
> + error = xfs_scrub_ag_init(info->sc, agno, &sa);
> + if (!xfs_scrub_fblock_process_error(info->sc, info->whichfork,
> + irec->br_startoff, &error))
> + return;
> +
> + xfs_scrub_xref_not_free(info->sc, &sa.bno_cur, agbno, len);
> +
> + xfs_scrub_ag_free(info->sc, &sa);
Why do we use an on-stack struct xfs_scrub_ag here, and not the one
embedded into the scrub context like all the other functions in this
patch?
> }
>
> /* Scrub a single extent record. */
> diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
> index 9151499..ae58fcc 100644
> --- a/fs/xfs/scrub/btree.c
> +++ b/fs/xfs/scrub/btree.c
> @@ -381,9 +381,12 @@ xfs_scrub_btree_check_block_owner(
> struct xfs_scrub_ag sa = { 0 };
> struct xfs_scrub_ag *psa;
> xfs_agnumber_t agno;
> + xfs_agblock_t bno;
> + bool is_freesp;
> int error = 0;
>
> agno = xfs_daddr_to_agno(bs->cur->bc_mp, daddr);
> + bno = xfs_daddr_to_agbno(bs->cur->bc_mp, daddr);
agbno.
> @@ -584,6 +584,21 @@ xfs_scrub_inode_xref(
> xfs_ino_t ino,
> struct xfs_dinode *dip)
> {
> + struct xfs_scrub_ag sa = { 0 };
> + xfs_agnumber_t agno;
> + xfs_agblock_t agbno;
> + int error;
> +
> + agno = XFS_INO_TO_AGNO(sc->mp, ino);
> + agbno = XFS_INO_TO_AGBNO(sc->mp, ino);
> +
> + error = xfs_scrub_ag_init(sc, agno, &sa);
> + if (!xfs_scrub_xref_process_error(sc, agno, agbno, &error))
> + return;
> +
> + xfs_scrub_xref_not_free(sc, &sa.bno_cur, agbno, 1);
> +
> + xfs_scrub_ag_free(sc, &sa);
Same question here - on-stack vs embedded....
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
