Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1333949 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a workqueue job Jeff Layton <jlayton@poochiereds.net> - 2016-02-15 01:20 +0100
    Re: [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a  workqueue job Eryu Guan <guaneryu@gmail.com> - 2016-02-15 07:10 +0100
      Re: [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a  workqueue job Jeff Layton <jlayton@poochiereds.net> - 2016-02-15 19:00 +0100
    Re: [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a  workqueue job Jan Kara <jack@suse.cz> - 2016-02-17 21:10 +0100

#1333949 — [PATCH 2/2] fsnotify: turn fsnotify reaper thread into a workqueue job

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@suse.com>
Cc: Eric Paris <eparis@parisplace.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eryu Guan <guaneryu@gmail.com>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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

[toc] | [next] | [standalone]

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

On Sun, Feb 14, 2016 at 07:15:23PM -0500, 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@suse.com>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Eryu Guan <guaneryu@gmail.com>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>

With these two patches applied on top of r.5-rc3, the same test passed
2-hour stress run, also survived stress test that forks 5 processes
running the same test program for 30 minutes.

Thanks for looking into this!

Eryu

[toc] | [prev] | [next] | [standalone]

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

On Mon, 15 Feb 2016 14:03:10 +0800
Eryu Guan <guaneryu@gmail.com> wrote:

> On Sun, Feb 14, 2016 at 07:15:23PM -0500, 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@suse.com>
> > Cc: Eric Paris <eparis@parisplace.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Eryu Guan <guaneryu@gmail.com>
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>  
> 
> With these two patches applied on top of r.5-rc3, the same test passed
> 2-hour stress run, also survived stress test that forks 5 processes
> running the same test program for 30 minutes.
> 
> Thanks for looking into this!
> 
> Eryu

Thanks for reporting and testing the patches. Yes, I tested them too
while running your reproducer and watched the size of the
inotify_inode_mark slab. It seemed to stay pretty stable with these
patches as well.

Andrew, would you mind picking up these patches since you merged the
original one?

Thanks,
-- 
Jeff Layton <jlayton@poochiereds.net>

[toc] | [prev] | [next] | [standalone]

#1336721 — 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@suse.com>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Eryu Guan <guaneryu@gmail.com>
> Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>

The patch looks correct to me. You can add:

Reviewed-by: Jan Kara <jack@suse.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@suse.com>
SUSE Labs, CR

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web