| Message ID | 1455495323-29605-2-git-send-email-jeff.layton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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 > >
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);
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(-)