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

Jian Wen
2024-01-11T12:52:59Z

On Thu, Jan 11, 2024 at 5:38 AM Dave Chinner <david@fromorbit.com> 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.
>
> Time for them to move to CONFIG_PREEMPT_DYNAMIC?
I had asked one of them to support CONFIG_PREEMPT_DYNAMIC before
sending the patch.
>
> Also there has been recent action towards removing
> CONFIG_PREEMPT_NONE/VOLUNTARY and cond_resched() altogether because
> the lazy preemption model coming present in the RTPREEMPT patchset
> solves the performance issues with full preemption that PREEMPT_NONE
> works around...
>
> https://lwn.net/Articles/944686/
> https://lwn.net/Articles/945422/
>
> Further, Thomas Gleixner has stated in those discussions that:
>
> "Though definitely I'm putting a permanent NAK in place for
> any attempts to duct tape the preempt=NONE model any
> further by sprinkling more cond*() and whatever warts
> around."
>
> https://lwn.net/ml/linux-kernel/87jzshhexi.ffs@tglx/
>
> > Explicitly call cond_resched in xfs_itruncate_extents_flags avoid
> > the below softlockup warning.
>
> IOWs, this is no longer considered an acceptible solution by core
> kernel maintainers.
Understood. I will only build a hotfix for our production kernel then.
>
> Regardless of these policy issues, the code change:
>
> > 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>
>
> Global includes like this go in fs/xfs/xfs_linux.h, but I don't
> think that's even necessary because we have cond_resched() calls
> elsewhere in XFS with the same include list as xfs_inode.c...
>
> > #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();
> > }
>
> Shouldn't this go in xfs_defer_finish() so that we capture all the
> cases where we loop indefinitely over a range continually rolling a
> permanent transaction via xfs_defer_finish()?
It seems xfs_collapse_file_space and xfs_insert_file_space also need
to yield CPU.
I don't have use cases for them yet.
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com