| Message ID | 158904183079.982941.15948246247495283555.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs_repair: catch things that xfs_check misses | expand |
On Sat, May 09, 2020 at 09:30:30AM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > The rmapbt record order checks here don't quite work properly. For > non-shared filesystems, we fail to check that the startblock of the nth > record comes entirely after the previous record. > > However, for filesystems with shared blocks (reflink) we correctly check > that the startblock/owner/offset of the nth record comes after the > previous one. > > Therefore, make the reflink fs checks use "laststartblock" to preserve > that functionality while making the non-reflink fs checks use > "lastblock" to fix the problem outlined above. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > repair/scan.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > > diff --git a/repair/scan.c b/repair/scan.c > index 7c46ab89..7508f7e8 100644 > --- a/repair/scan.c > +++ b/repair/scan.c > @@ -925,15 +925,15 @@ struct rmap_priv { > static bool > rmap_in_order( > xfs_agblock_t b, > - xfs_agblock_t lastblock, > + xfs_agblock_t laststartblock, > uint64_t owner, > uint64_t lastowner, > uint64_t offset, > uint64_t lastoffset) > { > - if (b > lastblock) > + if (b > laststartblock) > return true; > - else if (b < lastblock) > + else if (b < laststartblock) > return false; > > if (owner > lastowner) So this is just a variable rename and looks obviously fine. > @@ -964,6 +964,7 @@ scan_rmapbt( > int hdr_errors = 0; > int numrecs; > int state; > + xfs_agblock_t laststartblock = 0; > xfs_agblock_t lastblock = 0; > uint64_t lastowner = 0; > uint64_t lastoffset = 0; > @@ -1101,14 +1102,15 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"), > /* Check for out of order records. */ > if (i == 0) { > advance: > - lastblock = b; > + laststartblock = b; > + lastblock = end - 1; > lastowner = owner; > lastoffset = offset; > } else { > bool bad; > > if (xfs_sb_version_hasreflink(&mp->m_sb)) > - bad = !rmap_in_order(b, lastblock, > + bad = !rmap_in_order(b, laststartblock, > owner, lastowner, > offset, lastoffset); > else This looks correct, but really hard to read. I'll send a follow on cleanup. Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/repair/scan.c b/repair/scan.c index 7c46ab89..7508f7e8 100644 --- a/repair/scan.c +++ b/repair/scan.c @@ -925,15 +925,15 @@ struct rmap_priv { static bool rmap_in_order( xfs_agblock_t b, - xfs_agblock_t lastblock, + xfs_agblock_t laststartblock, uint64_t owner, uint64_t lastowner, uint64_t offset, uint64_t lastoffset) { - if (b > lastblock) + if (b > laststartblock) return true; - else if (b < lastblock) + else if (b < laststartblock) return false; if (owner > lastowner) @@ -964,6 +964,7 @@ scan_rmapbt( int hdr_errors = 0; int numrecs; int state; + xfs_agblock_t laststartblock = 0; xfs_agblock_t lastblock = 0; uint64_t lastowner = 0; uint64_t lastoffset = 0; @@ -1101,14 +1102,15 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"), /* Check for out of order records. */ if (i == 0) { advance: - lastblock = b; + laststartblock = b; + lastblock = end - 1; lastowner = owner; lastoffset = offset; } else { bool bad; if (xfs_sb_version_hasreflink(&mp->m_sb)) - bad = !rmap_in_order(b, lastblock, + bad = !rmap_in_order(b, laststartblock, owner, lastowner, offset, lastoffset); else