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