| Message ID | 160375514426.879169.1166063350727282652.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfsprogs: fixes for 5.10 | expand |
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 > */ >
On Mon, Oct 26, 2020 at 04:32:24PM -0700, 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. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
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 */