| Message ID | 20201208171651.GA1943235@magnolia (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: fix the forward progress assertion in xfs_iwalk_run_callbacks | expand |
On Tue, Dec 08, 2020 at 09:16:51AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > In commit 27c14b5daa82 we started tracking the last inode seen during an > inode walk to avoid infinite loops if a corrupt inobt record happens to > have a lower ir_startino than the record preceeding it. Unfortunately, > the assertion trips over the case where there are completely empty inobt > records (which can happen quite easily on 64k page filesystems) because > we advance the tracking cursor without actually putting the empty record > into the processing buffer. Fix the assert to allow for this case. > > Reported-by: zlang@redhat.com > Fixes: 27c14b5daa82 ("xfs: ensure inobt record walks always make forward progress") > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Looks good to me, and I just gave it a test on the same P9 machine which triggered this bug, test passed. Reviewed-by: Zorro Lang <zlang@redhat.com> > fs/xfs/xfs_iwalk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c > index 2a45138831e3..eae3aff9bc97 100644 > --- a/fs/xfs/xfs_iwalk.c > +++ b/fs/xfs/xfs_iwalk.c > @@ -363,7 +363,7 @@ xfs_iwalk_run_callbacks( > /* Delete cursor but remember the last record we cached... */ > xfs_iwalk_del_inobt(tp, curpp, agi_bpp, 0); > irec = &iwag->recs[iwag->nr_recs - 1]; > - ASSERT(next_agino == irec->ir_startino + XFS_INODES_PER_CHUNK); > + ASSERT(next_agino >= irec->ir_startino + XFS_INODES_PER_CHUNK); > > error = xfs_iwalk_ag_recs(iwag); > if (error) >
On Tue, Dec 08, 2020 at 09:16:51AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > In commit 27c14b5daa82 we started tracking the last inode seen during an > inode walk to avoid infinite loops if a corrupt inobt record happens to > have a lower ir_startino than the record preceeding it. Unfortunately, > the assertion trips over the case where there are completely empty inobt > records (which can happen quite easily on 64k page filesystems) because > we advance the tracking cursor without actually putting the empty record > into the processing buffer. Fix the assert to allow for this case. > > Reported-by: zlang@redhat.com > Fixes: 27c14b5daa82 ("xfs: ensure inobt record walks always make forward progress") > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_iwalk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c > index 2a45138831e3..eae3aff9bc97 100644 > --- a/fs/xfs/xfs_iwalk.c > +++ b/fs/xfs/xfs_iwalk.c > @@ -363,7 +363,7 @@ xfs_iwalk_run_callbacks( > /* Delete cursor but remember the last record we cached... */ > xfs_iwalk_del_inobt(tp, curpp, agi_bpp, 0); > irec = &iwag->recs[iwag->nr_recs - 1]; > - ASSERT(next_agino == irec->ir_startino + XFS_INODES_PER_CHUNK); > + ASSERT(next_agino >= irec->ir_startino + XFS_INODES_PER_CHUNK); Looks sensible. Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c index 2a45138831e3..eae3aff9bc97 100644 --- a/fs/xfs/xfs_iwalk.c +++ b/fs/xfs/xfs_iwalk.c @@ -363,7 +363,7 @@ xfs_iwalk_run_callbacks( /* Delete cursor but remember the last record we cached... */ xfs_iwalk_del_inobt(tp, curpp, agi_bpp, 0); irec = &iwag->recs[iwag->nr_recs - 1]; - ASSERT(next_agino == irec->ir_startino + XFS_INODES_PER_CHUNK); + ASSERT(next_agino >= irec->ir_startino + XFS_INODES_PER_CHUNK); error = xfs_iwalk_ag_recs(iwag); if (error)