On Wed, Jul 13, 2022 at 07:49:40AM +1000, Dave Chinner wrote:
> On Tue, Jul 12, 2022 at 11:32:19AM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 12, 2022 at 08:06:42AM +1000, Dave Chinner wrote:
> > > On Mon, Jul 11, 2022 at 10:41:34PM +0800, Zhang Yi wrote:
> > > > In the procedure of recover AGI unlinked lists, if something bad
> > > > happenes on one of the unlinked inode in the bucket list, we would call
> > > > xlog_recover_clear_agi_bucket() to clear the whole unlinked bucket list,
> > > > not the unlinked inodes after the bad one. If we have already added some
> > > > inodes to the gc workqueue before the bad inode in the list, we could
> > > > get below error when freeing those inodes, and finaly fail to complete
> > > > the log recover procedure.
> > > >
> > > > XFS (ram0): Internal error xfs_iunlink_remove at line 2456 of file
> > > > fs/xfs/xfs_inode.c. Caller xfs_ifree+0xb0/0x360 [xfs]
> > > >
> > > > The problem is xlog_recover_clear_agi_bucket() clear the bucket list, so
> > > > the gc worker fail to check the agino in xfs_verify_agino(). Fix this by
> > > > flush workqueue before clearing the bucket.
> > > >
> > > > Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>
> > > > ---
> > > > fs/xfs/xfs_log_recover.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > > index 5f7e4e6e33ce..2f655ef4364e 100644
> > > > --- a/fs/xfs/xfs_log_recover.c
> > > > +++ b/fs/xfs/xfs_log_recover.c
> > > > @@ -2714,6 +2714,7 @@ xlog_recover_process_one_iunlink(
> > > > * Call xlog_recover_clear_agi_bucket() to perform a transaction to
> > > > * clear the inode pointer in the bucket.
> > > > */
> > > > + xfs_inodegc_flush(mp);
> > > > xlog_recover_clear_agi_bucket(mp, agno, bucket);
> > > > return NULLAGINO;
> > > > }
> > >
> > > Looks good.
> > >
> > > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > I propose adding:
> > Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues")
> >
> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > >
> > > Darrick, FYI, I actually tripped over this and fixed it in the inode
> > > iunlink series as part of double linking the unlinked inode list in
> > > this patch:
> > >
> > > https://lore.kernel.org/linux-xfs/20220707234345.1097095-6-david@xxxxxxxxxxxxx/
> > >
> > > I didn't realise at the time I was forward porting this code that it
> > > was a pre-existing bug.....
> >
> > Yep. I'll merge this into the tree for easier porting with 5.15, and
> > fix up whatever merge conflicts result, if you're still interested in
> > merging the incore iunlinks for 5.20.
>
> Yes, I'll send you a pull request for it soon now that all reviews
> have been done. If you want, I'll include this fix first and rebase
> the commits that is causes conflicts with on top of it cleanly....
Yes, please include it in the rebase. :)
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
