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

["Darrick J. Wong" <djwong@kernel.org>]

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@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....

Yes, please include it in the rebase. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com