transfer ASYNC vmscan writeback IO to the flusher threads

The sync() is performed in two stages: the WB_SYNC_NONE sync and the WB_SYNC_ALL sync. It is necessary to tag both stages with wbc.for_sync, so as to prevent either of them being livelocked.

The basic livelock scheme will be based on the sync_after timestamp. Inodes dirtied after that won't be queued for IO. The timestamp could be recorded as early as the sync() time, this patch lazily sets it in writeback_inodes_sb()/sync_inodes_sb(). This will stop livelock, but may do more work than necessary.

Note that writeback_inodes_sb() is called by not only sync(), they are treated the same because the other callers need the same livelock prevention.

CC Jan Kara <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c         |   21 ++++++++++++---------
 include/linux/writeback.h |    1 +
 2 files changed, 13 insertions(+), 9 deletions(-)

fs/fs-writeback.c +6 -5

@@ -36,6 +36,8 @@ struct wb_writeback_work {
 	long nr_pages;
 	struct super_block *sb;
 	enum writeback_sync_modes sync_mode;
+	unsigned long sync_after;
+	unsigned int for_sync:1;
 	unsigned int for_kupdate:1;
 	unsigned int range_cyclic:1;
 	unsigned int for_background:1;

@@ -1086,20 +1090,17 @@ static void wait_sb_inodes(struct super_
  */
 void writeback_inodes_sb(struct super_block *sb)
 {
-	unsigned long nr_dirty = global_page_state(NR_FILE_DIRTY);
-	unsigned long nr_unstable = global_page_state(NR_UNSTABLE_NFS);
 	DECLARE_COMPLETION_ONSTACK(done);
 	struct wb_writeback_work work = {
 		.sb		= sb,
 		.sync_mode	= WB_SYNC_NONE,
+		.for_sync	= 1,
+		.sync_after	= jiffies,
 		.done		= &done,
 	};
 
 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
 
-	work.nr_pages = nr_dirty + nr_unstable +
-			(inodes_stat.nr_inodes - inodes_stat.nr_unused);
-
 	bdi_queue_work(sb->s_bdi, &work);
 	wait_for_completion(&done);
 }

@@ -1137,6 +1138,8 @@ void sync_inodes_sb(struct super_block *
 	struct wb_writeback_work work = {
 		.sb		= sb,
 		.sync_mode	= WB_SYNC_ALL,
+		.for_sync	= 1,
+		.sync_after	= jiffies,
 		.nr_pages	= LONG_MAX,
 		.range_cyclic	= 0,
 		.done		= &done,

include/linux/writeback.h +1 -0

@@ -48,6 +48,7 @@ struct writeback_control {
 	unsigned encountered_congestion:1; /* An output: a queue is full */
 	unsigned for_kupdate:1;		/* A kupdate writeback */
 	unsigned for_background:1;	/* A background writeback */
+	unsigned for_sync:1;		/* A writeback for sync */
 	unsigned for_reclaim:1;		/* Invoked from the page allocator */
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 };

Quoted 10 lines · Wu Fengguang

The sync() is performed in two stages: the WB_SYNC_NONE sync and the WB_SYNC_ALL sync. It is necessary to tag both stages with wbc.for_sync, so as to prevent either of them being livelocked.

The basic livelock scheme will be based on the sync_after timestamp. Inodes dirtied after that won't be queued for IO. The timestamp could be recorded as early as the sync() time, this patch lazily sets it in writeback_inodes_sb()/sync_inodes_sb(). This will stop livelock, but may do more work than necessary.

Quoted 3 lines

Note that writeback_inodes_sb() is called by not only sync(), they are treated the same because the other callers need the same livelock prevention.

OK, but the patch does nothing, doesn't it? I'd prefer if the fields
you introduce were actually used in this patch.

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c           |   13 +++++++++++++
 include/linux/backing-dev.h |    6 ++++++
 mm/backing-dev.c            |    1 +
 3 files changed, 20 insertions(+)

fs/fs-writeback.c +13 -0

@@ -80,6 +80,8 @@ static void bdi_queue_work(struct backin
 
 	spin_lock(&bdi->wb_lock);
 	list_add_tail(&work->list, &bdi->work_list);
+	if (work->for_sync)
+		atomic_inc(&bdi->wb.sync_works);
 	spin_unlock(&bdi->wb_lock);
 
 	/*

@@ -633,6 +635,14 @@ static long wb_writeback(struct bdi_writ
 			break;
 
 		/*
+		 * background/periodic works can run forever, need to abort
+		 * on seeing any pending sync work, to prevent livelock it.
+		 */
+		if (atomic_read(&wb->sync_works) &&
+		    (work->for_background || work->for_kupdate))
+			break;
+
+		/*
 		 * For background writeout, stop when we are below the
 		 * background dirty threshold
 		 */

@@ -765,6 +775,9 @@ long wb_do_writeback(struct bdi_writebac
 
 		wrote += wb_writeback(wb, work);
 
+		if (work->for_sync)
+			atomic_dec(&wb->sync_works);
+
 		/*
 		 * Notify the caller of completion if this is a synchronous
 		 * work item, otherwise just free it.

include/linux/backing-dev.h +6 -0

@@ -50,6 +50,12 @@ struct bdi_writeback {
 
 	unsigned long last_old_flush;		/* last old data flush */
 
+	/*
+	 * sync works queued, background works shall abort on seeing this,
+	 * to prevent livelocking the sync works
+	 */
+	atomic_t sync_works;
+
 	struct task_struct	*task;		/* writeback task */
 	struct list_head	b_dirty;	/* dirty inodes */
 	struct list_head	b_io;		/* parked for writeback */

mm/backing-dev.c +1 -1

@@ -257,6 +257,7 @@ static void bdi_wb_init(struct bdi_write
 
 	wb->bdi = bdi;
 	wb->last_old_flush = jiffies;
+	atomic_set(&wb->sync_works, 0);
 	INIT_LIST_HEAD(&wb->b_dirty);
 	INIT_LIST_HEAD(&wb->b_io);
 	INIT_LIST_HEAD(&wb->b_more_io);
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

I even had a patch for this but it's already outdated by now. But I
can refresh it if we decide this is the way to go.

Quoted 70 lines

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/fs-writeback.c           |   13 +++++++++++++
 include/linux/backing-dev.h |    6 ++++++
 mm/backing-dev.c            |    1 +
 3 files changed, 20 insertions(+)

fs/fs-writeback.c +13 -0

@@ -80,6 +80,8 @@ static void bdi_queue_work(struct backin
 
 	spin_lock(&bdi->wb_lock);
 	list_add_tail(&work->list, &bdi->work_list);
+	if (work->for_sync)
+		atomic_inc(&bdi->wb.sync_works);
 	spin_unlock(&bdi->wb_lock);
 
 	/*

@@ -633,6 +635,14 @@ static long wb_writeback(struct bdi_writ
 			break;
 
 		/*
+		 * background/periodic works can run forever, need to abort
+		 * on seeing any pending sync work, to prevent livelock it.
+		 */
+		if (atomic_read(&wb->sync_works) &&
+		    (work->for_background || work->for_kupdate))
+			break;
+
+		/*
 		 * For background writeout, stop when we are below the
 		 * background dirty threshold
 		 */

@@ -765,6 +775,9 @@ long wb_do_writeback(struct bdi_writebac
 
 		wrote += wb_writeback(wb, work);
 
+		if (work->for_sync)
+			atomic_dec(&wb->sync_works);
+
 		/*
 		 * Notify the caller of completion if this is a synchronous
 		 * work item, otherwise just free it.

include/linux/backing-dev.h +6 -0

@@ -50,6 +50,12 @@ struct bdi_writeback {
 
 	unsigned long last_old_flush;		/* last old data flush */
 
+	/*
+	 * sync works queued, background works shall abort on seeing this,
+	 * to prevent livelocking the sync works
+	 */
+	atomic_t sync_works;
+
 	struct task_struct	*task;		/* writeback task */
 	struct list_head	b_dirty;	/* dirty inodes */
 	struct list_head	b_io;		/* parked for writeback */

mm/backing-dev.c +1 -0

@@ -257,6 +257,7 @@ static void bdi_wb_init(struct bdi_write
 
 	wb->bdi = bdi;
 	wb->last_old_flush = jiffies;
+	atomic_set(&wb->sync_works, 0);
 	INIT_LIST_HEAD(&wb->b_dirty);
 	INIT_LIST_HEAD(&wb->b_io);
 	INIT_LIST_HEAD(&wb->b_more_io);

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Quoted 2 lines

I even had a patch for this but it's already outdated by now. But I
can refresh it if we decide this is the way to go.

I'm very interested in your old patch, would you post it? Let's see which one is easier to work with :)

Quoted 17 lines · Wu Fengguang

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Quoted 2 lines

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Well, even after writing all those MAX_WRITEBACK_PAGES. Because what you
want to do in a non-livelockable work is: take inode, write it, never look at it again for this work. Because if you later return to the inode, it can have newer dirty pages and thus you cannot really avoid livelock. Of course, this all assumes .nr_to_write isn't set to something small. That avoids the livelock as well.

Quoted 5 lines

Quoted 2 lines

I even had a patch for this but it's already outdated by now. But I
can refresh it if we decide this is the way to go.

I'm very interested in your old patch, would you post it? Let's see which one is easier to work with :)

OK, attached is the patch. I've rebased it against 2.6.35.
Honza

Signature · 19 lines

Jan Kara <[email protected]>
SUSE Labs, CR

From a6df0d4db148f983fe756df4791409db28dff459 Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Mon, 2 Aug 2010 22:30:25 +0200
Subject: [PATCH] mm: Stop background writeback if there is other work queued for the thread

Background writeback and kupdate-style writeback are easily livelockable
(from a definition of their target). This is inconvenient because it can
make sync(1) stall forever waiting on its queued work to be finished.
Fix the problem by interrupting background and kupdate writeback if there
is some other work to do. We can return to them after completing all the
queued work.

Signed-off-by: Jan Kara <[email protected]>
---
 fs/fs-writeback.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

fs/fs-writeback.c +0 -0

index d5be169..542471e 100644
@@ -633,6 +633,14 @@ static long wb_writeback(struct bdi_writeback *wb,
 			break;

Quoted 18 lines · Jan Kara

Quoted 17 lines · Wu Fengguang

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Quoted 8 lines

Quoted 2 lines

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Well, even after writing all those MAX_WRITEBACK_PAGES. Because what you
want to do in a non-livelockable work is: take inode, write it, never look at it again for this work. Because if you later return to the inode, it can have newer dirty pages and thus you cannot really avoid livelock. Of course, this all assumes .nr_to_write isn't set to something small. That avoids the livelock as well.

I do have a poor man's solution that can handle this case.
https://kerneltrap.org/mailarchive/linux-fsdevel/2009/10/7/6476473/thread
It may do more extra works, but will stop livelock in theory.

A related question is, what if some for_reclaim works get enqueued? Shall we postpone the sync work as well? The global sync is not likely to hit the dirty pages in a small memcg, or may take long time. It seems not a high priority task though.

Quoted 34 lines

Quoted 5 lines

Quoted 2 lines

I even had a patch for this but it's already outdated by now. But I
can refresh it if we decide this is the way to go.

I'm very interested in your old patch, would you post it? Let's see which one is easier to work with :)

OK, attached is the patch. I've rebased it against 2.6.35.
Honza

Signature · 19 lines

Jan Kara <[email protected]>
SUSE Labs, CR

From a6df0d4db148f983fe756df4791409db28dff459 Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Mon, 2 Aug 2010 22:30:25 +0200
Subject: [PATCH] mm: Stop background writeback if there is other work queued for the thread

Background writeback and kupdate-style writeback are easily livelockable
(from a definition of their target). This is inconvenient because it can
make sync(1) stall forever waiting on its queued work to be finished.
Fix the problem by interrupting background and kupdate writeback if there
is some other work to do. We can return to them after completing all the
queued work.

Signed-off-by: Jan Kara <[email protected]>
---
 fs/fs-writeback.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

fs/fs-writeback.c +0 -0

index d5be169..542471e 100644
@@ -633,6 +633,14 @@ static long wb_writeback(struct bdi_writeback *wb,
 			break;

fs/fs-writeback.c static long wb_writeback(struct bdi_writeback *wb, · line 643

Quoted 10 lines

fs/fs-writeback.c +8 -0

 
 		/*
+		 * Background writeout and kupdate-style writeback are
+		 * easily livelockable. Stop them if there is other work
+		 * to do so that e.g. sync can proceed.
+		 */
+		if ((work->for_background || work->for_kupdate) &&
+		    !list_empty(&wb->bdi->work_list))
+			break;
+		/*

I like it. It's much simpler.

Reviewed-by Wu Fengguang <[email protected]>

Quoted 28 lines · Wu Fengguang

Quoted 18 lines · Jan Kara

Quoted 17 lines · Wu Fengguang

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Quoted 8 lines

Quoted 2 lines

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Well, even after writing all those MAX_WRITEBACK_PAGES. Because what you
want to do in a non-livelockable work is: take inode, write it, never look at it again for this work. Because if you later return to the inode, it can have newer dirty pages and thus you cannot really avoid livelock. Of course, this all assumes .nr_to_write isn't set to something small. That avoids the livelock as well.

Quoted 3 lines

I do have a poor man's solution that can handle this case.
https://kerneltrap.org/mailarchive/linux-fsdevel/2009/10/7/6476473/thread
It may do more extra works, but will stop livelock in theory.

So I don't think sync work on it's own is a problem. There we can just
give up any fairness and just go inode by inode. IMHO it's much simpler that way. The remaining types of work we have are "for_reclaim" and then ones triggered by filesystems to get rid of delayed allocated data. These cases can easily have well defined and low nr_to_write so they wouldn't be livelockable either. What do you think?

Quoted 32 lines · Jan Kara

Quoted 28 lines · Wu Fengguang

Quoted 18 lines · Jan Kara

Quoted 17 lines · Wu Fengguang

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Quoted 8 lines

Quoted 2 lines

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Well, even after writing all those MAX_WRITEBACK_PAGES. Because what you
want to do in a non-livelockable work is: take inode, write it, never look at it again for this work. Because if you later return to the inode, it can have newer dirty pages and thus you cannot really avoid livelock. Of course, this all assumes .nr_to_write isn't set to something small. That avoids the livelock as well.

Quoted 3 lines

I do have a poor man's solution that can handle this case.
https://kerneltrap.org/mailarchive/linux-fsdevel/2009/10/7/6476473/thread
It may do more extra works, but will stop livelock in theory.

Quoted 6 lines

So I don't think sync work on it's own is a problem. There we can just
give up any fairness and just go inode by inode. IMHO it's much simpler that way. The remaining types of work we have are "for_reclaim" and then ones triggered by filesystems to get rid of delayed allocated data. These cases can easily have well defined and low nr_to_write so they wouldn't be livelockable either. What do you think?

Fengguang, how about merging also the attached simple patch together with
my fix? With these two patches, I'm not able to trigger any sync livelock while without one of them I hit them quite easily...

Honza

Signature · 22 lines

Jan Kara <[email protected]>
SUSE Labs, CR

From e4b708115825bca6a1020eed2356e2aab0567e3a Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
Date: Tue, 3 Aug 2010 10:35:02 +0000
Subject: [PATCH] mm: Avoid resetting wb_start after each writeback round

WB_SYNC_NONE writeback is done in rounds of 1024 pages so that we don't write
out some huge inode for too long while starving writeout of other inodes. To
avoid livelocks, we record time we started writeback in wbc->wb_start and do
not write out inodes which were dirtied after this time. But currently,
writeback_inodes_wb() resets wb_start each time it is called thus effectively
invalidating this logic and making any WB_SYNC_NONE writeback prone to
livelocks.

This patch makes sure wb_start is set only once when we start writeback.

Signed-off-by: Jan Kara <[email protected]>
---
 fs/fs-writeback.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

fs/fs-writeback.c +3 -2

index 6bdc924..aa59394 100644
@@ -530,7 +530,8 @@ void writeback_inodes_wb(struct bdi_writeback *wb,
 {
 	int ret = 0;
 
-	wbc->wb_start = jiffies; /* livelock avoidance */
+	if (!wbc->wb_start)
+		wbc->wb_start = jiffies; /* livelock avoidance */
 	spin_lock(&inode_lock);
 	if (!wbc->for_kupdate || list_empty(&wb->b_io))
 		queue_io(wb, wbc->older_than_this);

@@ -559,7 +560,6 @@ static void __writeback_inodes_sb(struct super_block *sb,
 {
 	WARN_ON(!rwsem_is_locked(&sb->s_umount));
 
-	wbc->wb_start = jiffies; /* livelock avoidance */
 	spin_lock(&inode_lock);
 	if (!wbc->for_kupdate || list_empty(&wb->b_io))
 		queue_io(wb, wbc->older_than_this);

@@ -625,6 +625,7 @@ static long wb_writeback(struct bdi_writeback *wb,
 		wbc.range_end = LLONG_MAX;
 	}
 
+	wbc.wb_start = jiffies; /* livelock avoidance */
 	for (;;) {
 		/*
 		 * Stop writeback when nr_pages has been consumed

Signature · 1 line

1.6.0.2

Quoted 39 lines · Jan Kara

Quoted 32 lines · Jan Kara

Quoted 28 lines · Wu Fengguang

Quoted 18 lines · Jan Kara

Quoted 17 lines · Wu Fengguang

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Quoted 8 lines

Quoted 2 lines

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Well, even after writing all those MAX_WRITEBACK_PAGES. Because what you
want to do in a non-livelockable work is: take inode, write it, never look at it again for this work. Because if you later return to the inode, it can have newer dirty pages and thus you cannot really avoid livelock. Of course, this all assumes .nr_to_write isn't set to something small. That avoids the livelock as well.

Quoted 3 lines

I do have a poor man's solution that can handle this case.
https://kerneltrap.org/mailarchive/linux-fsdevel/2009/10/7/6476473/thread
It may do more extra works, but will stop livelock in theory.

Quoted 6 lines

So I don't think sync work on it's own is a problem. There we can just
give up any fairness and just go inode by inode. IMHO it's much simpler that way. The remaining types of work we have are "for_reclaim" and then ones triggered by filesystems to get rid of delayed allocated data. These cases can easily have well defined and low nr_to_write so they wouldn't be livelockable either. What do you think?

Quoted 3 lines

Fengguang, how about merging also the attached simple patch together with
my fix? With these two patches, I'm not able to trigger any sync livelock while without one of them I hit them quite easily...

This looks OK. However note that redirty_tail() can modify dirtied_when unexpectedly. So the more we rely on wb_start, the more possibility an inode is (wrongly) skipped by sync. I have a bunch of patches to remove redirty_tail(). However they may not be good candidates for 2.6.36..

Quoted 3 lines

Fengguang, how about merging also the attached simple patch together with
my fix? With these two patches, I'm not able to trigger any sync livelock while without one of them I hit them quite easily...

This looks OK. However note that redirty_tail() can modify dirtied_when unexpectedly. So the more we rely on wb_start, the more possibility an inode is (wrongly) skipped by sync. I have a bunch of patches to remove redirty_tail(). However they may not be good candidates for 2.6.36..

Quoted 4 lines

Quoted 3 lines

Fengguang, how about merging also the attached simple patch together with
my fix? With these two patches, I'm not able to trigger any sync livelock while without one of them I hit them quite easily...

Quoted 5 lines

This looks OK. However note that redirty_tail() can modify dirtied_when unexpectedly. So the more we rely on wb_start, the more possibility an inode is (wrongly) skipped by sync. I have a bunch of patches to remove redirty_tail(). However they may not be good candidates for 2.6.36..

It looks that setting wb_start at the beginning of writeback_inodes_wb() won't be easily affected by redirty_tail().

Quoted 10 lines · Wu Fengguang

Quoted 9 lines

Quoted 3 lines

Fengguang, how about merging also the attached simple patch together with
my fix? With these two patches, I'm not able to trigger any sync livelock while without one of them I hit them quite easily...

This looks OK. However note that redirty_tail() can modify dirtied_when unexpectedly. So the more we rely on wb_start, the more possibility an inode is (wrongly) skipped by sync. I have a bunch of patches to remove redirty_tail(). However they may not be good candidates for 2.6.36..

Quoted 2 lines

It looks that setting wb_start at the beginning of writeback_inodes_wb() won't be easily affected by redirty_tail().

Except for this redirty_tail(), which may mess up the dirtied_when ordering in b_dirty and later on break the assumption of inode_dirtied_after(inode, wbc->wb_start).

It can be replaced by a requeue_io() for now. Christoph mentioned a patchset to introduce sb->s_wb, which should be a better solution.

Thanks,
Fengguang

fs/fs-writeback.c +0 -0

index a178828..e56e68b 100644
@@ -457,12 +457,7 @@ int generic_writeback_sb_inodes(struct super_block *sb,
 
 		if (inode->i_sb != sb) {
 			if (only_this_sb) {

Quoted 28 lines · Wu Fengguang

Quoted 26 lines · Jan Kara

Quoted 11 lines · Wu Fengguang

Quoted 9 lines · Jan Kara

Quoted 4 lines · Wu Fengguang

The periodic/background writeback can run forever. So when any sync work is enqueued, increase bdi->sync_works to notify the active non-sync works to exit. Non-sync works queued after sync works won't be affected.

Hmm, wouldn't it be simpler logic to just make for_kupdate and
for_background work always yield when there's some other work to do (as they are livelockable from the definition of the target they have) and make sure any other work isn't livelockable?

Quoted 8 lines

Good idea!

Quoted 3 lines

The only downside is that
non-livelockable work cannot be "fair" in the sense that we cannot switch inodes after writing MAX_WRITEBACK_PAGES.

Cannot switch indoes _before_ finish with the current MAX_WRITEBACK_PAGES batch?

Well, even after writing all those MAX_WRITEBACK_PAGES. Because what you
want to do in a non-livelockable work is: take inode, write it, never look at it again for this work. Because if you later return to the inode, it can have newer dirty pages and thus you cannot really avoid livelock. Of course, this all assumes .nr_to_write isn't set to something small. That avoids the livelock as well.

Quoted 3 lines

I do have a poor man's solution that can handle this case.
https://kerneltrap.org/mailarchive/linux-fsdevel/2009/10/7/6476473/thread
It may do more extra works, but will stop livelock in theory.

So I don't think sync work on it's own is a problem. There we can just
give up any fairness and just go inode by inode. IMHO it's much simpler that way.

The remaining types of work we have are "for_reclaim" and then ones triggered by filesystems to get rid of delayed allocated data. These cases can easily have well defined and low nr_to_write so they wouldn't be livelockable either. What do you think?

Quoted 4 lines

A related question is, what if some for_reclaim works get enqueued? Shall we postpone the sync work as well? The global sync is not likely to hit the dirty pages in a small memcg, or may take long time. It seems not a high priority task though.

I see some incentive to do this but the simple thing with for_background
and for_kupdate work is that they are essentially state-less and so they can be easily (and automatically) restarted. It would be really hard to implement something like this for sync and still avoid livelocks.

The start time in writeback_inodes_wb() is not very useful because it slips at each invocation time. Preferrably one _constant_ time shall be used at the beginning to cover the whole sync() work.

The newly dirtied inodes are now guarded at the queue_io() time instead of the b_io walk time. This is more natural: non-empty b_io/b_more_io means "more work pending".

The timestamp is now grabbed the sync work submission time, and may be further optimized to the initial sync() call time.

Hi Fengguang,

Quoted 8 lines · Wu Fengguang

The start time in writeback_inodes_wb() is not very useful because it slips at each invocation time. Preferrably one _constant_ time shall be used at the beginning to cover the whole sync() work.

The newly dirtied inodes are now guarded at the queue_io() time instead of the b_io walk time. This is more natural: non-empty b_io/b_more_io means "more work pending".

Quoted 2 lines

The timestamp is now grabbed the sync work submission time, and may be further optimized to the initial sync() call time.

The patch seems to have some issues...

fs/fs-writeback.c static void move_expired_inodes(struct l · line 231

Quoted 1 line

fs/fs-writeback.c +1 -0

+	if (wbc->for_sync) {

For example this is never set. You only set wb->for_sync.

fs/fs-writeback.c static void move_expired_inodes(struct l · line 233

Quoted 2 lines

fs/fs-writeback.c +2 -0

+		expire_interval = 1;
+		older_than_this = wbc->sync_after;

And sync_after is never set either???

fs/fs-writeback.c void writeback_inodes_wb(struct bdi_writ · line 545

Quoted 3 lines

fs/fs-writeback.c +1 -1

-	if (!(wbc->for_kupdate || wbc->for_background) || list_empty(&wb->b_io))
+	if (list_empty(&wb->b_io))
 		queue_io(wb, wbc);

And what is the purpose of this? It looks as an unrelated change to me.

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

Quoted 7 lines · Wu Fengguang

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

Quoted 4 lines

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

Well, essentially any WB_SYNC_NONE writeback is still livelockable if you
just grow a file constantly. So your changes are a step in the right direction but won't fix the issue completely.

But what we could do to fix
the issue completely would be to just set wbc->nr_to_write to LONG_MAX before writing inode for sync use my livelock avoidance using page-tagging for this case (it wouldn't have the possible performance issue because we are going to write all the inode anyway).

I can write the patch but frankly there are so many patches floating
around that I'm not sure what I should base it on...

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

let the flusher threads do ASYNC writeback for pageout()
[PATCH 4/5] writeback: introduce bdi_start_inode_writeback() [PATCH 5/5] vmscan: transfer async file writeback to the flusher

Quoted 12 lines · Wu Fengguang

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

Quoted 3 lines

let the flusher threads do ASYNC writeback for pageout()
[PATCH 4/5] writeback: introduce bdi_start_inode_writeback() [PATCH 5/5] vmscan: transfer async file writeback to the flusher

I really do not like this - all it does is transfer random page writeback from vmscan to the flusher threads rather than avoiding random page writeback altogether. Random page writeback is nasty - just say no.

Quoted 17 lines · Wu Fengguang

Quoted 15 lines

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

let the flusher threads do ASYNC writeback for pageout()
[PATCH 4/5] writeback: introduce bdi_start_inode_writeback() [PATCH 5/5] vmscan: transfer async file writeback to the flusher

Quoted 3 lines

I really do not like this - all it does is transfer random page writeback from vmscan to the flusher threads rather than avoiding random page writeback altogether. Random page writeback is nasty - just say no.

There are cases we have to do pageout().

- a stressed memcg with lots of dirty pages
- a large NUMA system whose nodes have unbalanced vmscan rate and dirty pages

Quoted 4 lines

There are cases we have to do pageout().

- a stressed memcg with lots of dirty pages
- a large NUMA system whose nodes have unbalanced vmscan rate and dirty pages

- 32bit highmem system too

can you please see following commit? this describe current design.

commit c4e2d7ddde9693a4c05da7afd485db02c27a7a09

Author: akpm <akpm>
Date:   Sun Dec 22 01:07:33 2002 +0000

[PATCH] Give kswapd writeback higher priority than pdflush

The `low latency page reclaim' design works by preventing page allocators from blocking on request queues (and by preventing them from blocking against writeback of individual pages, but that is immaterial here).

This has a problem under some situations. pdflush (or a write(2) caller) could be saturating the queue with highmem pages. This prevents anyone from writing back ZONE_NORMAL pages. We end up doing enormous amounts of scenning.

A test case is to mmap(MAP_SHARED) almost all of a 4G machine's memory, then kill the mmapping applications. The machine instantly goes from 0% of memory dirty to 95% or more. pdflush kicks in and starts writing the least-recently-dirtied pages, which are all highmem. The queue is congested so nobody will write back ZONE_NORMAL pages. kswapd chews 50% of the CPU scanning past dirty ZONE_NORMAL pages and page reclaim efficiency (pages_reclaimed/pages_scanned) falls to 2%.

So this patch changes the policy for kswapd. kswapd may use all of a request queue, and is prepared to block on request queues.

What will now happen in the above scenario is:

1: The page alloctor scans some pages, fails to reclaim enough
memory and takes a nap in blk_congetion_wait().

2: kswapd() will scan the ZONE_NORMAL LRU and will start writing
back pages. (These pages will be rotated to the tail of the inactive list at IO-completion interrupt time).

This writeback will saturate the queue with ZONE_NORMAL pages. Conveniently, pdflush will avoid the congested queues. So we end up writing the correct pages.

In this test, kswapd CPU utilisation falls from 50% to 2%, page reclaim efficiency rises from 2% to 40% and things are generally a lot happier.

The downside is that kswapd may now do a lot less page reclaim, increasing page allocation latency, causing more direct reclaim, increasing lock contention in the VM, etc. But I have not been able to demonstrate that in testing.

The other problem is that there is only one kswapd, and there are lots of disks. That is a generic problem - without being able to co-opt user processes we don't have enough threads to keep lots of disks saturated.

One fix for this would be to add an additional "really congested" threshold in the request queues, so kswapd can still perform nonblocking writeout. This gives kswapd priority over pdflush while allowing kswapd to feed many disk queues. I doubt if this will be called for.

BKrev: 3e051055aitHp3bZBPSqmq21KGs5aQ

Quoted 13 lines · Dave Chinner

Quoted 12 lines · Wu Fengguang

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

Quoted 7 lines

Quoted 3 lines

let the flusher threads do ASYNC writeback for pageout()
[PATCH 4/5] writeback: introduce bdi_start_inode_writeback() [PATCH 5/5] vmscan: transfer async file writeback to the flusher

I really do not like this - all it does is transfer random page writeback from vmscan to the flusher threads rather than avoiding random page writeback altogether. Random page writeback is nasty - just say no.

There are cases we have to do pageout().

- a stressed memcg with lots of dirty pages
- a large NUMA system whose nodes have unbalanced vmscan rate and dirty pages

In the above cases, the whole system may not be that stressed, except for some local LRU list being busy scanned. If the local memory stress lead to lots of pageout(), it could bring down the whole system by congesting the disks with many small seeky IO.

It may be an overkill to push global writeback (ie. it's silly to sync 1GB dirty data because there is a small stressed 100MB LRU list).

The
obvious solution is to keep the pageout() calls and make them more IO wise by doing write-around at the same time. The write-around pages will likely be in the same stressed LRU list, hence will do good for page reclaim as well.

Transferring ASYNC work to the flushers helps the kswapd-vs-flusher priority problem too. Currently the kswapd/direct reclaim either have to skip dirty pages on congestion, or to risk being blocked in get_request_wait(), both are not good options. However the use of bdi_start_inode_writeback() do ask for a good vmscan throttling scheme to prevent it falsely OOM before the flusher is able to clean the transfered pages. This would be tricky.

Quoted 27 lines · Wu Fengguang

Quoted 13 lines · Dave Chinner

Quoted 12 lines · Wu Fengguang

Andrew,

It's possible to transfer ASYNC vmscan writeback IOs to the flusher threads. This simple patchset shows the basic idea. Since it's a big behavior change, there are inevitably lots of details to sort out. I don't know where it will go after tests and discussions, so the patches are intentionally kept simple.

sync livelock avoidance (need more to be complete, but this is minimal required for the last two patches)
[PATCH 1/5] writeback: introduce wbc.for_sync to cover the two sync stages [PATCH 2/5] writeback: stop periodic/background work on seeing sync works
[PATCH 3/5] writeback: prevent sync livelock with the sync_after timestamp

Quoted 7 lines

Quoted 3 lines

let the flusher threads do ASYNC writeback for pageout()
[PATCH 4/5] writeback: introduce bdi_start_inode_writeback() [PATCH 5/5] vmscan: transfer async file writeback to the flusher

I really do not like this - all it does is transfer random page writeback from vmscan to the flusher threads rather than avoiding random page writeback altogether. Random page writeback is nasty - just say no.

There are cases we have to do pageout().

- a stressed memcg with lots of dirty pages
- a large NUMA system whose nodes have unbalanced vmscan rate and dirty pages

Quoted 7 lines

In the above cases, the whole system may not be that stressed, except for some local LRU list being busy scanned. If the local memory stress lead to lots of pageout(), it could bring down the whole system by congesting the disks with many small seeky IO.

It may be an overkill to push global writeback (ie. it's silly to sync 1GB dirty data because there is a small stressed 100MB LRU list).

No it isn't. Dirty pages have to cleaned sometime and it reclaim has a need to clean pages, we may as well start cleaning them all. Kicking background writeback is effectively just starting work we have already delayed into the future a little bit earlier than we otherwise would have.

Doing this is only going to hurt performance if the same pages are being frequently dirtied, but the cahnges to flush expired inodes first in background writeback should avoid the worst of that behaviour. Further, the more clean pages we have, the faster susbequent memory reclaims are going to free up pages....

Quoted 5 lines

The
obvious solution is to keep the pageout() calls and make them more IO wise by doing write-around at the same time. The write-around pages will likely be in the same stressed LRU list, hence will do good for page reclaim as well.

You've kind of already done that by telling it to writeback 1024 pages starting with a specific page. However, the big problem with this is that it asusme that the inode has contiguous dirty pages in

the cache. That assumption fall down in many cases e.g. when you are writing lots of small files like kernel trees contain, and so you still end up with random IO patterns coming out of reclaim.

Quoted 8 lines

Transferring ASYNC work to the flushers helps the kswapd-vs-flusher priority problem too. Currently the kswapd/direct reclaim either have to skip dirty pages on congestion, or to risk being blocked in get_request_wait(), both are not good options. However the use of bdi_start_inode_writeback() do ask for a good vmscan throttling scheme to prevent it falsely OOM before the flusher is able to clean the transfered pages. This would be tricky.

I have no problem with that aspect ofthe patch - my issue is that it does nothing to prevent the problem that causes excessive congestion in the first place...