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

[Dave Chinner <david@fromorbit.com>]

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?

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.

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()?

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