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

[Jan Kara]

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@xxxxxxxx>
> Cc: Eric Paris <eparis@xxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Eryu Guan <guaneryu@xxxxxxxxx>
> Signed-off-by: Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>

The patch looks correct to me. You can add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								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@xxxxxxxx>
SUSE Labs, CR