Re: [PATCH] xfs: explicitly call cond_resched in xfs_itruncate_extents_flags

Dave Chinner
2024-01-11T20:17:01Z

On Wed, Jan 10, 2024 at 09:45:01AM -0800, Darrick J. Wong wrote:
> On Wed, Jan 10, 2024 at 03:13:47PM +0800, Jian Wen wrote:
> > From: Jian Wen <wenjianhn@gmail.com>
> >
> > Deleting a file with lots of extents may cause a soft lockup if the
> > preemption model is none(CONFIG_PREEMPT_NONE=y or preempt=none is set
> > in the kernel cmdline). Alibaba cloud kernel and Oracle UEK container
> > kernel are affected by the issue, since they select CONFIG_PREEMPT_NONE=y.
> >
> > Explicitly call cond_resched in xfs_itruncate_extents_flags avoid
> > the below softlockup warning.
> > watchdog: BUG: soft lockup - CPU#0 stuck for 23s! [kworker/0:13:139]
>
> Wowwee, how many extents does this file have, anyway?

IIRC, extent removal runs at about 50,000 extents/s on a typical
modern CPU on a production kernel build when everything is cached.
23s then equates to a bit above a million extents....

> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c0f1c89786c2..194381e10472 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -4,6 +4,7 @@
> > * All Rights Reserved.
> > */
> > #include <linux/iversion.h>
> > +#include <linux/sched.h>
> >
> > #include "xfs.h"
> > #include "xfs_fs.h"
> > @@ -1383,6 +1384,8 @@ xfs_itruncate_extents_flags(
> > error = xfs_defer_finish(&tp);
> > if (error)
> > goto out;
> > +
> > + cond_resched();
>
> Is there a particular reason why truncation uses a giant chained
> transaction for all extents (with xfs_defer_finish) instead of, say, one
> chain per extent?

IIUC the question correctly, extent removal for a given range ibeing
truncated has to been seen externally as an atomic change so the
ILOCK has to held across the entire removal operation.

This means it has to be executed as a single rolling transaction as
transaction reservation cannot be done with the ILOCK held and the
atomicity of extent removal prevents the ILOCK from being dropped.

This atomicity only really matters for active operations
like truncate, hole punching, etc when other operations may also be
trying to act on the extent list. However, in inodegc, we are
essentially guaranteed that nobody else will need to access the
extent list whilst we are truncating them away, so it could be done
as a bunch of fine grained transactions.

However, this doesn't solve the problem the CPU never being yeilded
if everything is cached and no locks or transaction reservations are
ever contended. It will still hold the CPU for the same length of
time. Hence I'm not sure changing the way the transactions are run
will really change anything material w.r.t. the issue at hand.

-Dave.
--
Dave Chinner
david@fromorbit.com