linux-kernel - Re: [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a workqueue job

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 17 Feb 2016 21:01:36 +0100
From:	Jan Kara <jack@...e.cz>
To:	Jeff Layton <jlayton@...chiereds.net>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Jan Kara <jack@...e.com>, Eric Paris <eparis@...isplace.org>,
	Eryu Guan <guaneryu@...il.com>
Subject: Re: [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a
 workqueue job

On Sun 14-02-16 19:15:23, Jeff Layton wrote:
> We don't require a dedicated thread for fsnotify cleanup. Switch it over
> to a workqueue job instead that runs on the system_unbound_wq.
> 
> In the interest of not thrashing the queued job too often when there are
> a lot of marks being removed, we delay the reaper job slightly when
> queueing it, to allow several to gather on the list.
> 
> Cc: Jan Kara <jack@...e.com>
> Cc: Eric Paris <eparis@...isplace.org>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Eryu Guan <guaneryu@...il.com>
> Signed-off-by: Jeff Layton <jeff.layton@...marydata.com>

The patch looks correct to me. You can add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza

> ---
>  fs/notify/mark.c | 49 ++++++++++++++++++-------------------------------
>  1 file changed, 18 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index fc0df4442f7b..7115c5d7d373 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -91,10 +91,14 @@
>  #include <linux/fsnotify_backend.h>
>  #include "fsnotify.h"
>  
> +#define FSNOTIFY_REAPER_DELAY	(1)	/* 1 jiffy */
> +
>  struct srcu_struct fsnotify_mark_srcu;
>  static DEFINE_SPINLOCK(destroy_lock);
>  static LIST_HEAD(destroy_list);
> -static DECLARE_WAIT_QUEUE_HEAD(destroy_waitq);
> +
> +static void fsnotify_mark_destroy(struct work_struct *work);
> +static DECLARE_DELAYED_WORK(reaper_work, fsnotify_mark_destroy);
>  
>  void fsnotify_get_mark(struct fsnotify_mark *mark)
>  {
> @@ -189,7 +193,8 @@ void fsnotify_free_mark(struct fsnotify_mark *mark)
>  	spin_lock(&destroy_lock);
>  	list_add(&mark->g_list, &destroy_list);
>  	spin_unlock(&destroy_lock);
> -	wake_up(&destroy_waitq);
> +	queue_delayed_work(system_unbound_wq, &reaper_work,
> +				FSNOTIFY_REAPER_DELAY);
>  
>  	/*
>  	 * Some groups like to know that marks are being freed.  This is a
> @@ -388,7 +393,8 @@ err:
>  	spin_lock(&destroy_lock);
>  	list_add(&mark->g_list, &destroy_list);
>  	spin_unlock(&destroy_lock);
> -	wake_up(&destroy_waitq);
> +	queue_delayed_work(system_unbound_wq, &reaper_work,
> +				FSNOTIFY_REAPER_DELAY);
>  
>  	return ret;
>  }
> @@ -493,39 +499,20 @@ void fsnotify_init_mark(struct fsnotify_mark *mark,
>  	mark->free_mark = free_mark;
>  }
>  
> -static int fsnotify_mark_destroy(void *ignored)
> +static void fsnotify_mark_destroy(struct work_struct *work)
>  {
>  	struct fsnotify_mark *mark, *next;
>  	struct list_head private_destroy_list;
>  
> -	for (;;) {
> -		spin_lock(&destroy_lock);
> -		/* exchange the list head */
> -		list_replace_init(&destroy_list, &private_destroy_list);
> -		spin_unlock(&destroy_lock);
> -
> -		synchronize_srcu(&fsnotify_mark_srcu);
> +	spin_lock(&destroy_lock);
> +	/* exchange the list head */
> +	list_replace_init(&destroy_list, &private_destroy_list);
> +	spin_unlock(&destroy_lock);
>  
> -		list_for_each_entry_safe(mark, next, &private_destroy_list, g_list) {
> -			list_del_init(&mark->g_list);
> -			fsnotify_put_mark(mark);
> -		}
> +	synchronize_srcu(&fsnotify_mark_srcu);
>  
> -		wait_event_interruptible(destroy_waitq, !list_empty(&destroy_list));
> +	list_for_each_entry_safe(mark, next, &private_destroy_list, g_list) {
> +		list_del_init(&mark->g_list);
> +		fsnotify_put_mark(mark);
>  	}
> -
> -	return 0;
> -}
> -
> -static int __init fsnotify_mark_init(void)
> -{
> -	struct task_struct *thread;
> -
> -	thread = kthread_run(fsnotify_mark_destroy, NULL,
> -			     "fsnotify_mark");
> -	if (IS_ERR(thread))
> -		panic("unable to start fsnotify mark destruction thread.");
> -
> -	return 0;
>  }
> -device_initcall(fsnotify_mark_init);
> -- 
> 2.5.0
> 
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR