Received-SPF: pass (google.com: domain of linux-nfs+bounces-115-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 27 Nov 2023 17:59:03 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: NeilBrown <neilb@suse.de>
Cc: Al Viro <viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>,
        Jens Axboe <axboe@kernel.dk>, Oleg Nesterov <oleg@redhat.com>,
        Jeff Layton <jlayton@kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH/RFC] core/nfsd: allow kernel threads to use task_work.
Message-ID: <ZWUfNyO6OG/+aFuo@tissot.1015granger.net>
References: <170112272125.7109.6245462722883333440@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <170112272125.7109.6245462722883333440@noble.neil.brown.name>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?w0soJZ+QWqLGygvGiYJ0S9N+H3T+tBUaFkvnmuUX7MCS4Jb4qqa3vjfE+19k?=
 =?us-ascii?Q?1EuG+sCibze2aDEY93hPu6vQd9KGQ+XzZgiPRbIdnxTZ9x1i0LntThV96fMx?=
 =?us-ascii?Q?x0x8wDzFn1l008ZWwChsf120EFfjrPFPorB67KdCb3+1VX2WbtwlF7wByU6w?=
 =?us-ascii?Q?jEgHqhtsxRxyh7nb5Yo4NDXtGmEBwy5ljE2LSkOZQKvWbNfv+PuLZpSNdXQS?=
 =?us-ascii?Q?SrvWkXggfHXkMJB91AO/3iqv1iHYww/THHsNDliZM6e+6iUCntcuU53NvDT9?=
 =?us-ascii?Q?MFNWXnADorI/oGqbQxejKvTy68+M/VHOotgl2Gbe4Orr4+ZOpA5BivEKEbFc?=
 =?us-ascii?Q?mMuiEUm8jYbuAHbgthItWvaa69Udpz32H07IcKt0jUc8Y1BTrWtgralkjlfr?=
 =?us-ascii?Q?YJnVn9wHbrlmAMhfKm2Tl7/Jhp9wWpvPdR+jfaSsDJlVPhuaalPdAypszq1w?=
 =?us-ascii?Q?JWeB7RplZLc60whP+Ybt6U2VSq49sWhI7vGf6dirQwmyCaCJcSZ0siePPWR5?=
 =?us-ascii?Q?nzU06hUGD/4h+2Zjrfh+01B2qkWM+OyEF6ynnU2iTtfqUuRdsG9KkmemtZSl?=
 =?us-ascii?Q?BkYcZcCDZar+4KI2izeXeRbKij2MrYro2pncHBJRrDk47CJpTEtS4Fe26Ucm?=
 =?us-ascii?Q?yQcc/DyhnbqKIdP7LcnkgQGTYZI9b5QcBXXC87gHDWH1h4jC2cMJo3O+3GHL?=
 =?us-ascii?Q?h4iaVG60BcNVlyswK8vxyj+XPgKfgEDDzRNHLwfj9k30Y1wjgSQDgXIqoGCK?=
 =?us-ascii?Q?sqCyekPiA9nVEeKV4Up+suI2Shzb/EAUgEeTxFFzseYPYVmPbuAIp9co1os8?=
 =?us-ascii?Q?Ctail1N3w96hk/bXTBFCet/OBoUhVhvH+tilzbt34+PzHigxiXcKGv6WdINS?=
 =?us-ascii?Q?sOdR/Uhd/DbDGKwRbqZKHB1XniTIQxv5+r6Uh9a+RgUr2iVVsGIEPvPdSlUj?=
 =?us-ascii?Q?dbu+ud5U8FENblnelG3akMMhDyJ5Z09y07A923AzlJNNNAfFfntv3PXc+4FV?=
 =?us-ascii?Q?ITR8crNdD1FcAX/XNWA3AnuUQJvMh+IwLnlD80aaxW66VT3Sck4Vvr15x/nA?=
 =?us-ascii?Q?Bucu0RAOT9uAjWZyajFxlk9QTKgiSjmlO6TDUs6SVVHjbceiJWd8Fd5yE6Y1?=
 =?us-ascii?Q?KXHE65AOrvJMww4DLGN9sSAwIDvfdVNDJW8reIcQ/3mXVvuf/Er9+GMgp877?=
 =?us-ascii?Q?X+ukVhZHqOptu/f3F4/sSkzsdAgrRXWZTMzdlK5Zib0rivHO+rTlL0QMI+os?=
 =?us-ascii?Q?ZE8LRSiKdjehRv1XyaVH2aOymPVNU8bzAv4ITarMdJkWhPELVyRtjZn5GGl/?=
 =?us-ascii?Q?k1V/Sf+4z1G0sb6W6/x54KgmhTMKZ9p6lS3q+bYzARL6TLSr74MkmwTvc3r1?=
 =?us-ascii?Q?UzZuvrpw7s+4M/cUCyNdd82TexzgZOJ8cfCrkuh1K3URDU2YYcHFCX/PcG/Z?=
 =?us-ascii?Q?6GdQ+zF05WfLTEjq2ZaxFere0z+6QZFbwPLKBb4oDR/00p2YqnUlOevTLz2p?=
 =?us-ascii?Q?TV7lwn/w12yoAIz7e3wNtiu4eL7LngwJre3pytD96Kk31WFD543XRxTycQ+z?=
 =?us-ascii?Q?QoqAFGCM6eX6+wf2VVvnH+ExfD3qac49/bYghBEPm+g5JVOnheYfggedFFy8?=
 =?us-ascii?Q?pw=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	=?us-ascii?Q?Va3FTr3HmAJ/+gT7anRo3bCl+suDftBkl8s3ATcDSJ9/6HFR+cSMlX+9IZ8O?=
 =?us-ascii?Q?uk2yqd205qVg6XbAW/1YQH41zanWdTUv5Sz0JQW3Q8Wnu3NJUpaU12fBcCvN?=
 =?us-ascii?Q?pXov94vQjchjyS124qIDwYo49vXH5J7MUh91IXQDXYqwzSOnPLZzLPxiApYN?=
 =?us-ascii?Q?5VWsa42LcRcOyYiddBCejMx4amr17J7s5VNx1dkW5xftJetxQYUrt9iIhtYh?=
 =?us-ascii?Q?CfUr5H0MO6dUleMhvjAArnJXCsyj/9LFRRvNjZgQ+bc0O2WhgKYtdBoTzbur?=
 =?us-ascii?Q?eJ6wwd6habC4oiOWvhPSc1qA+2kX+sGiY9DvaQuxCAfwNWfGzWkoRPitpJ7m?=
 =?us-ascii?Q?swMaSlEAZU6O8VHZab6wka21a91rjFIZPP9r6sp9TpCYae1Yio3cMJpenO9q?=
 =?us-ascii?Q?YDfr/RMurG74uJLXHJryJU/jXB7UKdEBRD9ZexUTxKA2pqkZTkrewFGzsw88?=
 =?us-ascii?Q?4EvD0ffLMDv7Q51aD6Lop3glxrKTTFIJPPGLq8FgvjMWX+bOHOPKTcbrgp/B?=
 =?us-ascii?Q?gT6KXvXsUacNQaWEKmo4K0Zp6PiNToC7N3fzev9z3DgSwpycTloW30mCrpef?=
 =?us-ascii?Q?aC5KaBcUJEic/TIqMyrI/92GAPWTxA0Nxks4J7MUTegFsCHoKY7W57YKVBwx?=
 =?us-ascii?Q?fJ5TKFu+b4k4qA35CZJHhIRz/vexXlIX3HTXUKTPLtiqYbXhc8U8PEe9ZxKz?=
 =?us-ascii?Q?W+Nak1+Vq0maKVdvEMUw2ZoumYtBMYNCuMpoWCf+QdbF4uMeV4JcHdi8Nq1u?=
 =?us-ascii?Q?b5tSwRuZcWnxCZPYqwLdcFoiLEXc7C+eFJIntiOzLLtIprAaR8mQZF5sQ7Mb?=
 =?us-ascii?Q?G9D8VNN2qiDZTwTL0HvUpVy4SdwTvtJDXRG0yyVC4oB4yv+4zizi8hxZ2Dd6?=
 =?us-ascii?Q?85MBTHOtZpBpkBgpulKP6FHWdfdMyMIw9QnN4l2zPmMl0uuCgHGHb1wKNoga?=
 =?us-ascii?Q?qcWgwWvK4bXBEaVKyT52uECZ+alu1ei1r/IycRvYSL4=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a6b4765-e0bf-4c1c-3c39-08dbef9c7546
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Nov 2023 22:59:06.7050
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NUEK+kIS+XWBvDmWK5poXehUkM0llw5GJgUkcPVMY3fPVB1hxhB3mGQJgivcdz+ybCde905x5PH4J69CZYvnbg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR10MB7653
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-11-27_19,2023-11-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=467 spamscore=0
 adultscore=0 suspectscore=0 phishscore=0 bulkscore=0 malwarescore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311060000 definitions=main-2311270160
X-Proofpoint-ORIG-GUID: dm-I-2u4crHpjHR6ZuvsjKwoefiMXwZn
X-Proofpoint-GUID: dm-I-2u4crHpjHR6ZuvsjKwoefiMXwZn

On Tue, Nov 28, 2023 at 09:05:21AM +1100, NeilBrown wrote:
> 
> I have evidence from a customer site of 256 nfsd threads adding files to
> delayed_fput_lists nearly twice as fast they are retired by a single
> work-queue thread running delayed_fput().  As you might imagine this
> does not end well (20 million files in the queue at the time a snapshot
> was taken for analysis).
> 
> While this might point to a problem with the filesystem not handling the
> final close efficiently, such problems should only hurt throughput, not
> lead to memory exhaustion.

I have this patch queued for v6.8:

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/?h=nfsd-next&id=c42661ffa58acfeaf73b932dec1e6f04ce8a98c0


> For normal threads, the thread that closes the file also calls the
> final fput so there is natural rate limiting preventing excessive growth
> in the list of delayed fputs.  For kernel threads, and particularly for
> nfsd, delayed in the final fput do not impose any throttling to prevent
> the thread from closing more files.

I don't think we want to block nfsd threads waiting for files to
close. Won't that be a potential denial of service?


> A simple way to fix this is to treat nfsd threads like normal processes
> for task_work.  Thus the pending files are queued for the thread, and
> the same thread finishes the work.
> 
> Currently KTHREADs are assumed never to call task_work_run().  With this
> patch that it still the default but it is implemented by storing the
> magic value TASK_WORKS_DISABLED in ->task_works.  If a kthread, such as
> nfsd, will call task_work_run() periodically, it sets ->task_works
> to NULL to indicate this.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> 
> I wonder which tree this should go through assuming everyone likes it.
> VFS maybe??
> 
> Thanks.
> 
>  fs/file_table.c           | 2 +-
>  fs/nfsd/nfssvc.c          | 4 ++++
>  include/linux/sched.h     | 1 +
>  include/linux/task_work.h | 4 +++-
>  kernel/fork.c             | 2 +-
>  kernel/task_work.c        | 7 ++++---
>  6 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index de4a2915bfd4..e79351df22be 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -445,7 +445,7 @@ void fput(struct file *file)
>  	if (atomic_long_dec_and_test(&file->f_count)) {
>  		struct task_struct *task = current;
>  
> -		if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) {
> +		if (likely(!in_interrupt())) {
>  			init_task_work(&file->f_rcuhead, ____fput);
>  			if (!task_work_add(task, &file->f_rcuhead, TWA_RESUME))
>  				return;
> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
> index 66ca50b38b27..c047961262ca 100644
> --- a/fs/nfsd/nfssvc.c
> +++ b/fs/nfsd/nfssvc.c
> @@ -13,6 +13,7 @@
>  #include <linux/fs_struct.h>
>  #include <linux/swap.h>
>  #include <linux/siphash.h>
> +#include <linux/task_work.h>
>  
>  #include <linux/sunrpc/stats.h>
>  #include <linux/sunrpc/svcsock.h>
> @@ -941,6 +942,7 @@ nfsd(void *vrqstp)
>  	}
>  
>  	current->fs->umask = 0;
> +	current->task_works = NULL; /* Declare that I will call task_work_run() */
>  
>  	atomic_inc(&nfsdstats.th_cnt);
>  
> @@ -955,6 +957,8 @@ nfsd(void *vrqstp)
>  
>  		svc_recv(rqstp);
>  		validate_process_creds();
> +		if (task_work_pending(current))
> +			task_work_run();
>  	}
>  
>  	atomic_dec(&nfsdstats.th_cnt);
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 292c31697248..c63c2bedbf71 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1117,6 +1117,7 @@ struct task_struct {
>  	unsigned int			sas_ss_flags;
>  
>  	struct callback_head		*task_works;
> +#define	TASK_WORKS_DISABLED	((void*)1)
>  
>  #ifdef CONFIG_AUDIT
>  #ifdef CONFIG_AUDITSYSCALL
> diff --git a/include/linux/task_work.h b/include/linux/task_work.h
> index 795ef5a68429..3c74e3de81ed 100644
> --- a/include/linux/task_work.h
> +++ b/include/linux/task_work.h
> @@ -22,7 +22,9 @@ enum task_work_notify_mode {
>  
>  static inline bool task_work_pending(struct task_struct *task)
>  {
> -	return READ_ONCE(task->task_works);
> +	struct callback_head *works = READ_ONCE(task->task_works);
> +
> +	return works && works != TASK_WORKS_DISABLED;
>  }
>  
>  int task_work_add(struct task_struct *task, struct callback_head *twork,
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 10917c3e1f03..903b29804fe1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2577,7 +2577,7 @@ __latent_entropy struct task_struct *copy_process(
>  	p->dirty_paused_when = 0;
>  
>  	p->pdeath_signal = 0;
> -	p->task_works = NULL;
> +	p->task_works = args->kthread ? TASK_WORKS_DISABLED : NULL;
>  	clear_posix_cputimers_work(p);
>  
>  #ifdef CONFIG_KRETPROBES
> diff --git a/kernel/task_work.c b/kernel/task_work.c
> index 95a7e1b7f1da..ffdf4b0d7a0e 100644
> --- a/kernel/task_work.c
> +++ b/kernel/task_work.c
> @@ -49,7 +49,8 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
>  
>  	head = READ_ONCE(task->task_works);
>  	do {
> -		if (unlikely(head == &work_exited))
> +		if (unlikely(head == &work_exited ||
> +			     head == TASK_WORKS_DISABLED))
>  			return -ESRCH;
>  		work->next = head;
>  	} while (!try_cmpxchg(&task->task_works, &head, work));
> @@ -157,7 +158,7 @@ void task_work_run(void)
>  		work = READ_ONCE(task->task_works);
>  		do {
>  			head = NULL;
> -			if (!work) {
> +			if (!work || work == TASK_WORKS_DISABLED) {
>  				if (task->flags & PF_EXITING)
>  					head = &work_exited;
>  				else
> @@ -165,7 +166,7 @@ void task_work_run(void)
>  			}
>  		} while (!try_cmpxchg(&task->task_works, &work, head));
>  
> -		if (!work)
> +		if (!work || work == TASK_WORKS_DISABLED)
>  			break;
>  		/*
>  		 * Synchronize with task_work_cancel(). It can not remove
> -- 
> 2.42.1
> 

-- 
Chuck Lever