Re: [PATCH 5/5] xfs_repair: correctly detect partially written extents

Allison Henderson
2020-10-27T05:52:26Z

On 10/26/20 4:32 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> Recently, I was able to create a realtime file with a 16b extent size
> and the following data fork mapping:
>
> data offset 0 startblock 144 (0/144) count 3 flag 0
> data offset 3 startblock 147 (0/147) count 3 flag 1
> data offset 6 startblock 150 (0/150) count 10 flag 0
>
> Notice how we have a written extent, then an unwritten extent, and then
> another written extent. The current code in process_rt_rec trips over
> that third extent, because repair only knows not to complain about inuse
> extents if the mapping was unwritten.
>
> This loop logic is confusing, because it tries to do too many things.
> Move the phase3 and phase4 code to separate helper functions, then
> isolate the code that handles a mapping that starts in the middle of an
> rt extent so that it's clearer what's going on.
>
Ok, seems reasonable, though I think I might have hoisted one helper
separately to make that a little easier to read through.

Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> repair/dinode.c | 180 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 112 insertions(+), 68 deletions(-)
>
>
> diff --git a/repair/dinode.c b/repair/dinode.c
> index c89f21e08373..028a23cd5c8c 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -176,76 +176,69 @@ verify_dfsbno_range(
>
> return XR_DFSBNORANGE_VALID;
> }
> +
> static int
> -process_rt_rec(
> +process_rt_rec_dups(
> struct xfs_mount *mp,
> - struct xfs_bmbt_irec *irec,
> xfs_ino_t ino,
> - xfs_rfsblock_t *tot,
> - int check_dups)
> + struct xfs_bmbt_irec *irec)
> {
> - xfs_fsblock_t b, lastb;
> + xfs_fsblock_t b;
> xfs_rtblock_t ext;
> - int state;
> - int pwe; /* partially-written extent */
>
> - /*
> - * check numeric validity of the extent
> - */
> - if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
> - do_warn(
> -_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
> - ino,
> - irec->br_startblock,
> - irec->br_startoff);
> - return 1;
> - }
> -
> - lastb = irec->br_startblock + irec->br_blockcount - 1;
> - if (!libxfs_verify_rtbno(mp, lastb)) {
> - do_warn(
> -_("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
> - ino,
> - lastb,
> - irec->br_startoff);
> - return 1;
> - }
> - if (lastb < irec->br_startblock) {
> - do_warn(
> -_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
> - "end %" PRIu64 ", offset %" PRIu64 "\n"),
> - ino,
> - irec->br_startblock,
> - lastb,
> - irec->br_startoff);
> - return 1;
> - }
> -
> - /*
> - * set the appropriate number of extents
> - * this iterates block by block, this can be optimised using extents
> - */
> - for (b = irec->br_startblock; b < irec->br_startblock +
> - irec->br_blockcount; b += mp->m_sb.sb_rextsize) {
> + for (b = rounddown(irec->br_startblock, mp->m_sb.sb_rextsize);
> + b < irec->br_startblock + irec->br_blockcount;
> + b += mp->m_sb.sb_rextsize) {
> ext = (xfs_rtblock_t) b / mp->m_sb.sb_rextsize;
> - pwe = irec->br_state == XFS_EXT_UNWRITTEN &&
> - (b % mp->m_sb.sb_rextsize != 0);
> -
> - if (check_dups == 1) {
> - if (search_rt_dup_extent(mp, ext) && !pwe) {
> - do_warn(
> + if (search_rt_dup_extent(mp, ext)) {
> + do_warn(
> _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
> - "off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
> - ino,
> - irec->br_startoff,
> - irec->br_startblock,
> - irec->br_blockcount);
> - return 1;
> - }
> - continue;
> +"off - %" PRIu64 ", start - %" PRIu64 ", count %" PRIu64 "\n"),
> + ino,
> + irec->br_startoff,
> + irec->br_startblock,
> + irec->br_blockcount);
> + return 1;
> }
> + }
>
> + return 0;
> +}
> +
> +static int
> +process_rt_rec_state(
> + struct xfs_mount *mp,
> + xfs_ino_t ino,
> + struct xfs_bmbt_irec *irec)
> +{
> + xfs_fsblock_t b = irec->br_startblock;
> + xfs_rtblock_t ext;
> + int state;
> +
> + do {
> + ext = (xfs_rtblock_t)b / mp->m_sb.sb_rextsize;
> state = get_rtbmap(ext);
> +
> + if ((b % mp->m_sb.sb_rextsize) != 0) {
> + /*
> + * We are midway through a partially written extent.
> + * If we don't find the state that gets set in the
> + * other clause of this loop body, then we have a
> + * partially *mapped* rt extent and should complain.
> + */
> + if (state != XR_E_INUSE)
> + do_error(
> +_("data fork in rt inode %" PRIu64 " found invalid rt extent %"PRIu64" state %d at rt block %"PRIu64"\n"),
> + ino, ext, state, b);
> + b = roundup(b, mp->m_sb.sb_rextsize);
> + continue;
> + }
> +
> + /*
> + * This is the start of an rt extent. Set the extent state if
> + * nobody else has claimed the extent, or complain if there are
> + * conflicting states.
> + */
> switch (state) {
> case XR_E_FREE:
> case XR_E_UNKNOWN:
> @@ -253,32 +246,83 @@ _("data fork in rt ino %" PRIu64 " claims dup rt extent,"
> break;
> case XR_E_BAD_STATE:
> do_error(
> -_("bad state in rt block map %" PRIu64 "\n"),
> +_("bad state in rt extent map %" PRIu64 "\n"),
> ext);
> case XR_E_FS_MAP:
> case XR_E_INO:
> case XR_E_INUSE_FS:
> do_error(
> -_("data fork in rt inode %" PRIu64 " found metadata block %" PRIu64 " in rt bmap\n"),
> +_("data fork in rt inode %" PRIu64 " found rt metadata extent %" PRIu64 " in rt bmap\n"),
> ino, ext);
> case XR_E_INUSE:
> - if (pwe)
> - break;
> - /* fall through */
> case XR_E_MULT:
> set_rtbmap(ext, XR_E_MULT);
> do_warn(
> -_("data fork in rt inode %" PRIu64 " claims used rt block %" PRIu64 "\n"),
> - ino, ext);
> +_("data fork in rt inode %" PRIu64 " claims used rt extent %" PRIu64 "\n"),
> + ino, b);
> return 1;
> case XR_E_FREE1:
> default:
> do_error(
> -_("illegal state %d in rt block map %" PRIu64 "\n"),
> - state, b);
> +_("illegal state %d in rt extent %" PRIu64 "\n"),
> + state, ext);
> }
> + b += mp->m_sb.sb_rextsize;
> + } while (b < irec->br_startblock + irec->br_blockcount);
> +
> + return 0;
> +}
> +
> +static int
> +process_rt_rec(
> + struct xfs_mount *mp,
> + struct xfs_bmbt_irec *irec,
> + xfs_ino_t ino,
> + xfs_rfsblock_t *tot,
> + int check_dups)
> +{
> + xfs_fsblock_t lastb;
> + int bad;
> +
> + /*
> + * check numeric validity of the extent
> + */
> + if (!libxfs_verify_rtbno(mp, irec->br_startblock)) {
> + do_warn(
> +_("inode %" PRIu64 " - bad rt extent start block number %" PRIu64 ", offset %" PRIu64 "\n"),
> + ino,
> + irec->br_startblock,
> + irec->br_startoff);
> + return 1;
> }
>
> + lastb = irec->br_startblock + irec->br_blockcount - 1;
> + if (!libxfs_verify_rtbno(mp, lastb)) {
> + do_warn(
> +_("inode %" PRIu64 " - bad rt extent last block number %" PRIu64 ", offset %" PRIu64 "\n"),
> + ino,
> + lastb,
> + irec->br_startoff);
> + return 1;
> + }
> + if (lastb < irec->br_startblock) {
> + do_warn(
> +_("inode %" PRIu64 " - bad rt extent overflows - start %" PRIu64 ", "
> + "end %" PRIu64 ", offset %" PRIu64 "\n"),
> + ino,
> + irec->br_startblock,
> + lastb,
> + irec->br_startoff);
> + return 1;
> + }
> +
> + if (check_dups)
> + bad = process_rt_rec_dups(mp, ino, irec);
> + else
> + bad = process_rt_rec_state(mp, ino, irec);
> + if (bad)
> + return bad;
> +
> /*
> * bump up the block counter
> */
>