On Wed, Mar 06, 2024 at 10:33:16AM +0800, Wang Yugui wrote:
> Hi,
>
>
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Chandan reported a AGI/AGF lock order hang on xfs/168 during recent
> > testing. The cause of the problem was the task running xfs_growfs
> > to shrink the filesystem. A failure occurred trying to remove the
> > free space from the btrees that the shrink would make disappear,
> > and that meant it ran the error handling for a partial failure.
> >
> > This error path involves restoring the per-ag block reservations,
> > and that requires calculating the amount of space needed to be
> > reserved for the free inode btree. The growfs operation hung here:
> >
> > [18679.536829] down+0x71/0xa0
> > [18679.537657] xfs_buf_lock+0xa4/0x290 [xfs]
> > [18679.538731] xfs_buf_find_lock+0xf7/0x4d0 [xfs]
> > [18679.539920] xfs_buf_lookup.constprop.0+0x289/0x500 [xfs]
> > [18679.542628] xfs_buf_get_map+0x2b3/0xe40 [xfs]
> > [18679.547076] xfs_buf_read_map+0xbb/0x900 [xfs]
> > [18679.562616] xfs_trans_read_buf_map+0x449/0xb10 [xfs]
> > [18679.569778] xfs_read_agi+0x1cd/0x500 [xfs]
> > [18679.573126] xfs_ialloc_read_agi+0xc2/0x5b0 [xfs]
> > [18679.578708] xfs_finobt_calc_reserves+0xe7/0x4d0 [xfs]
> > [18679.582480] xfs_ag_resv_init+0x2c5/0x490 [xfs]
> > [18679.586023] xfs_ag_shrink_space+0x736/0xd30 [xfs]
> > [18679.590730] xfs_growfs_data_private.isra.0+0x55e/0x990 [xfs]
> > [18679.599764] xfs_growfs_data+0x2f1/0x410 [xfs]
> > [18679.602212] xfs_file_ioctl+0xd1e/0x1370 [xfs]
> >
> > trying to get the AGI lock. The AGI lock was held by a fstress task
> > trying to do an inode allocation, and it was waiting on the AGF
> > lock to allocate a new inode chunk on disk. Hence deadlock.
> >
> > The fix for this is for the growfs code to hold the AGI over the
> > transaction roll it does in the error path. It already holds the AGF
> > locked across this, and that is what causes the lock order inversion
> > in the xfs_ag_resv_init() call.
> >
> > Reported-by: Chandan Babu R <chandanbabu@xxxxxxxxxx>
> > Fixes: 46141dc891f7 ("xfs: introduce xfs_ag_shrink_space()")
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_ag.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index d728709054b2..dc1873f76bff 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -975,14 +975,23 @@ xfs_ag_shrink_space(
> >
> > if (error) {
> > /*
> > - * if extent allocation fails, need to roll the transaction to
> > + * If extent allocation fails, need to roll the transaction to
> > * ensure that the AGFL fixup has been committed anyway.
> > + *
> > + * We need to hold the AGF across the roll to ensure nothing can
> > + * access the AG for allocation until the shrink is fully
> > + * cleaned up. And due to the resetting of the AG block
> > + * reservation space needing to lock the AGI, we also have to
> > + * hold that so we don't get AGI/AGF lock order inversions in
> > + * the error handling path.
> > */
> > xfs_trans_bhold(*tpp, agfbp);
> > + xfs_trans_bhold(*tpp, agibp);
> > err2 = xfs_trans_roll(tpp);
> > if (err2)
> > return err2;
> > xfs_trans_bjoin(*tpp, agfbp);
> > + xfs_trans_bjoin(*tpp, agibp);
> > goto resv_init_out;
>
> Should ‘xfs_trans_bjoin(*tpp, agibp)’ be done
> before ‘xfs_trans_bjoin(*tpp, agfbp)’?
It doesn't matter.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
