Re: [PATCH] xfs: flush inode gc workqueue before clearing agi bucket

[Dave Chinner <david@fromorbit.com>]

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@huawei.com>
> > > ---
> > >  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@redhat.com>
> 
> I propose adding:
> Fixes: ab23a7768739 ("xfs: per-cpu deferred inode inactivation queues")
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> > 
> > 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@fromorbit.com/
> > 
> > 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....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com