Message-ID: <ZaBPM14r5vGrQ9mc@dread.disaster.area>
Date: Fri, 12 Jan 2024 07:27:31 +1100
From: Dave Chinner <david@...morbit.com>
To: Jian Wen <wenjianhn@...il.com>
Cc: linux-xfs@...r.kernel.org, djwong@...nel.org, hch@....de,
	dchinner@...hat.com, Jian Wen <wenjian1@...omi.com>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: explicitly call cond_resched in
 xfs_itruncate_extents_flags

[cc Thomas, lkml]

On Thu, Jan 11, 2024 at 08:52:22PM +0800, Jian Wen wrote:
> On Thu, Jan 11, 2024 at 5:38 AM Dave Chinner <david@...morbit.com> wrote:
> >
> > On Wed, Jan 10, 2024 at 03:13:47PM +0800, Jian Wen wrote:
> > > From: Jian Wen <wenjianhn@...il.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.

OK.

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

Yeah, that may be your best short term fix. We'll need to clarify
what the current policy is on adding cond_resched points before we
go any further in this direction.

Thomas, any update on what is happening with cond_resched() - is
there an ETA on it going away/being unnecessary?

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

Yup, they do, but they also call xfs_defer_finish(), so having the
cond_resched() in that function will capture them as well.

Also, the current upstream tree has moved this code from
xfs_itruncate_extents_flags() to xfs_bunmapi_range(), so the
cond_resched() has to be moved, anyway. We may as well put it in
xfs_defer_finish() if we end up doing this.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists