On Sat, May 09, 2020 at 09:30:30AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxx>
