| Message ID | 20240715170915.776487-1-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | repair: btree blocks are never on the RT subvolume | expand |
On Mon, Jul 15, 2024 at 07:09:15PM +0200, Christoph Hellwig wrote: > scan_bmapbt tries to track btree blocks in the RT duplicate extent > AVL tree if the inode has the realtime flag set. Given that the > RT subvolume is only ever used for file data this is incorrect. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Did you actually hit this, or did you find it through code inspection? AFAICT for attr forks, the (whichfork != XFS_DATA_FORK) check should have been saving us this whole time? And the (type != XR_INO_RTDATA) check did the job for the data fork? IOWs, is this merely cleaning out a logic bomb, or is it resolving some false positive/customer complaint? Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > repair/scan.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/repair/scan.c b/repair/scan.c > index 338308ef8..8352b3ccf 100644 > --- a/repair/scan.c > +++ b/repair/scan.c > @@ -390,22 +390,11 @@ _("bad state %d, inode %" PRIu64 " bmap block 0x%" PRIx64 "\n"), > break; > } > pthread_mutex_unlock(&ag_locks[agno].lock); > - } else { > - /* > - * attribute fork for realtime files is in the regular > - * filesystem > - */ > - if (type != XR_INO_RTDATA || whichfork != XFS_DATA_FORK) { > - if (search_dup_extent(XFS_FSB_TO_AGNO(mp, bno), > - XFS_FSB_TO_AGBNO(mp, bno), > - XFS_FSB_TO_AGBNO(mp, bno) + 1)) > - return(1); > - } else { > - xfs_rtxnum_t ext = xfs_rtb_to_rtx(mp, bno); > - > - if (search_rt_dup_extent(mp, ext)) > - return 1; > - } > + } else { > + if (search_dup_extent(XFS_FSB_TO_AGNO(mp, bno), > + XFS_FSB_TO_AGBNO(mp, bno), > + XFS_FSB_TO_AGBNO(mp, bno) + 1)) > + return 1; > } > (*tot)++; > numrecs = be16_to_cpu(block->bb_numrecs); > -- > 2.43.0 > >
On Mon, Jul 15, 2024 at 10:32:04AM -0700, Darrick J. Wong wrote: > Did you actually hit this, or did you find it through code inspection? I hit this, but only with the per-rtg rt bitmap, with which this caused an out of bounds access in the new, non-upstream array of RT AVL trees. > AFAICT for attr forks, the (whichfork != XFS_DATA_FORK) check should > have been saving us this whole time? And the (type != XR_INO_RTDATA) > check did the job for the data fork? > > IOWs, is this merely cleaning out a logic bomb, or is it resolving some > false positive/customer complaint? As far as I can tell this is a real bug upstream. If you have a bmap btree block number that when interpreted as a RT extent is also otherwise used you'll get repair finding an incorrect duplicate block. It just seems hard enough to trigger so that no one found it despite being there since day 1 of public xfsprogs releases.
diff --git a/repair/scan.c b/repair/scan.c index 338308ef8..8352b3ccf 100644 --- a/repair/scan.c +++ b/repair/scan.c @@ -390,22 +390,11 @@ _("bad state %d, inode %" PRIu64 " bmap block 0x%" PRIx64 "\n"), break; } pthread_mutex_unlock(&ag_locks[agno].lock); - } else { - /* - * attribute fork for realtime files is in the regular - * filesystem - */ - if (type != XR_INO_RTDATA || whichfork != XFS_DATA_FORK) { - if (search_dup_extent(XFS_FSB_TO_AGNO(mp, bno), - XFS_FSB_TO_AGBNO(mp, bno), - XFS_FSB_TO_AGBNO(mp, bno) + 1)) - return(1); - } else { - xfs_rtxnum_t ext = xfs_rtb_to_rtx(mp, bno); - - if (search_rt_dup_extent(mp, ext)) - return 1; - } + } else { + if (search_dup_extent(XFS_FSB_TO_AGNO(mp, bno), + XFS_FSB_TO_AGBNO(mp, bno), + XFS_FSB_TO_AGBNO(mp, bno) + 1)) + return 1; } (*tot)++; numrecs = be16_to_cpu(block->bb_numrecs);
scan_bmapbt tries to track btree blocks in the RT duplicate extent AVL tree if the inode has the realtime flag set. Given that the RT subvolume is only ever used for file data this is incorrect. Signed-off-by: Christoph Hellwig <hch@lst.de> --- repair/scan.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)