writeback: avoid touching dirtied_when on blocked inodes

---
 fs/fs-writeback.c           |   16 ++++++++++++++++
 include/linux/backing-dev.h |    8 +++++---
 mm/backing-dev.c            |   10 ++++++++--
 3 files changed, 29 insertions(+), 5 deletions(-)

fs/fs-writeback.c +16 -0

@@ -234,6 +234,21 @@ static void requeue_io(struct inode *ino
 	list_move(&inode->i_wb_list, &wb->b_more_io);
 }
 
+/*
+ * The inode should be retried in an opportunistic way.
+ *
+ * The only difference between b_more_io and b_more_io_wait is:
+ * wb_writeback() won't quit as long as b_more_io is not empty.  When
+ * wb_writeback() quit on empty b_more_io and non-empty b_more_io_wait,
+ * the kupdate work will wakeup more frequently to retry the inodes in
+ * b_more_io_wait.
+ */
+static void requeue_io_wait(struct inode *inode, struct bdi_writeback *wb)
+{
+	assert_spin_locked(&wb->list_lock);
+	list_move(&inode->i_wb_list, &wb->b_more_io_wait);
+}
+
 static void inode_sync_complete(struct inode *inode)
 {
 	/*

@@ -321,6 +336,7 @@ static void queue_io(struct bdi_writebac
 	int moved;
 	assert_spin_locked(&wb->list_lock);
 	list_splice_init(&wb->b_more_io, &wb->b_io);
+	list_splice_init(&wb->b_more_io_wait, &wb->b_io);
 	moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, work);
 	trace_writeback_queue_io(wb, work, moved);
 }

include/linux/backing-dev.h +5 -3

@@ -59,6 +59,7 @@ struct bdi_writeback {
 	struct list_head b_dirty;	/* dirty inodes */
 	struct list_head b_io;		/* parked for writeback */
 	struct list_head b_more_io;	/* parked for more writeback */
+	struct list_head b_more_io_wait;/* opportunistic retry io */
 	spinlock_t list_lock;		/* protects the b_* lists */
 };

@@ -133,9 +134,10 @@ extern struct list_head bdi_pending_list
 
 static inline int wb_has_dirty_io(struct bdi_writeback *wb)
 {
-	return !list_empty(&wb->b_dirty) ||
-	       !list_empty(&wb->b_io) ||
-	       !list_empty(&wb->b_more_io);
+	return !list_empty(&wb->b_dirty)	||
+	       !list_empty(&wb->b_io)		||
+	       !list_empty(&wb->b_more_io)	||
+	       !list_empty(&wb->b_more_io_wait);
 }
 
 static inline void __add_bdi_stat(struct backing_dev_info *bdi,

mm/backing-dev.c +8 -2

@@ -74,10 +74,10 @@ static int bdi_debug_stats_show(struct s
 	unsigned long background_thresh;
 	unsigned long dirty_thresh;
 	unsigned long bdi_thresh;
-	unsigned long nr_dirty, nr_io, nr_more_io;
+	unsigned long nr_dirty, nr_io, nr_more_io, nr_more_io_wait;
 	struct inode *inode;
 
-	nr_dirty = nr_io = nr_more_io = 0;
+	nr_dirty = nr_io = nr_more_io = nr_more_io_wait = 0;
 	spin_lock(&wb->list_lock);
 	list_for_each_entry(inode, &wb->b_dirty, i_wb_list)
 		nr_dirty++;

@@ -85,6 +85,8 @@ static int bdi_debug_stats_show(struct s
 		nr_io++;
 	list_for_each_entry(inode, &wb->b_more_io, i_wb_list)
 		nr_more_io++;
+	list_for_each_entry(inode, &wb->b_more_io_wait, i_wb_list)
+		nr_more_io_wait++;
 	spin_unlock(&wb->list_lock);
 
 	global_dirty_limits(&background_thresh, &dirty_thresh);

@@ -103,6 +105,7 @@ static int bdi_debug_stats_show(struct s
 		   "b_dirty:            %10lu\n"
 		   "b_io:               %10lu\n"
 		   "b_more_io:          %10lu\n"
+		   "b_more_io_wait:     %10lu\n"
 		   "bdi_list:           %10u\n"
 		   "state:              %10lx\n",
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITEBACK)),

@@ -116,6 +119,7 @@ static int bdi_debug_stats_show(struct s
 		   nr_dirty,
 		   nr_io,
 		   nr_more_io,
+		   nr_more_io_wait,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -651,6 +655,7 @@ static void bdi_wb_init(struct bdi_write
 	INIT_LIST_HEAD(&wb->b_dirty);
 	INIT_LIST_HEAD(&wb->b_io);
 	INIT_LIST_HEAD(&wb->b_more_io);
+	INIT_LIST_HEAD(&wb->b_more_io_wait);
 	spin_lock_init(&wb->list_lock);
 	setup_timer(&wb->wakeup_timer, wakeup_timer_fn, (unsigned long)bdi);
 }

@@ -718,6 +723,7 @@ void bdi_destroy(struct backing_dev_info
 		list_splice(&bdi->wb.b_dirty, &dst->b_dirty);
 		list_splice(&bdi->wb.b_io, &dst->b_io);
 		list_splice(&bdi->wb.b_more_io, &dst->b_more_io);
+		list_splice(&bdi->wb.b_more_io_wait, &dst->b_more_io_wait);
 		spin_unlock(&bdi->wb.list_lock);
 		spin_unlock(&dst->list_lock);
 	}

Jan Kara

reviewer Acked-by on v1 1/7

2011-10-20 23:23 UTC #2 lore raw

Re: [PATCH 1/7] writeback: introduce queue b_more_io_wait

Quoted 13 lines · Wu Fengguang

The problem is, redirty_tail() may update i_dirtied_when and result in 30s max delay. If redirty_tail() is called often enough, some inode may even be delayed for ever.
So introduce the b_more_io_wait queue to park inodes that for some reason cannot be synced immediately. The inodes will be sent to b_io at the next b_io refill time, however won't be busy retried as b_more_io:
when the redirtied inodes are all in b_more_io_wait, wb_writeback() will see empty b_more_io and hence break out of the loop.
This would be the new data flow after converting all redirty_tail() calls to requeue_io_wait():

Quoted 8 lines

b_dirty --> b_io --> b_more_io/b_more_io_wait --+
             ^                                  |
             |                                  |
             +----------------------------------+
Cc Jan Kara <[email protected]>
Cc Dave Chinner <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>

You can add:

Honza

Quoted 120 lines

---
 fs/fs-writeback.c           |   16 ++++++++++++++++
 include/linux/backing-dev.h |    8 +++++---
 mm/backing-dev.c            |   10 ++++++++--
 3 files changed, 29 insertions(+), 5 deletions(-)

fs/fs-writeback.c +16 -0

@@ -234,6 +234,21 @@ static void requeue_io(struct inode *ino
 	list_move(&inode->i_wb_list, &wb->b_more_io);
 }
 
+/*
+ * The inode should be retried in an opportunistic way.
+ *
+ * The only difference between b_more_io and b_more_io_wait is:
+ * wb_writeback() won't quit as long as b_more_io is not empty.  When
+ * wb_writeback() quit on empty b_more_io and non-empty b_more_io_wait,
+ * the kupdate work will wakeup more frequently to retry the inodes in
+ * b_more_io_wait.
+ */
+static void requeue_io_wait(struct inode *inode, struct bdi_writeback *wb)
+{
+	assert_spin_locked(&wb->list_lock);
+	list_move(&inode->i_wb_list, &wb->b_more_io_wait);
+}
+
 static void inode_sync_complete(struct inode *inode)
 {
 	/*

@@ -321,6 +336,7 @@ static void queue_io(struct bdi_writebac
 	int moved;
 	assert_spin_locked(&wb->list_lock);
 	list_splice_init(&wb->b_more_io, &wb->b_io);
+	list_splice_init(&wb->b_more_io_wait, &wb->b_io);
 	moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, work);
 	trace_writeback_queue_io(wb, work, moved);
 }

include/linux/backing-dev.h +5 -3

@@ -59,6 +59,7 @@ struct bdi_writeback {
 	struct list_head b_dirty;	/* dirty inodes */
 	struct list_head b_io;		/* parked for writeback */
 	struct list_head b_more_io;	/* parked for more writeback */
+	struct list_head b_more_io_wait;/* opportunistic retry io */
 	spinlock_t list_lock;		/* protects the b_* lists */
 };

@@ -133,9 +134,10 @@ extern struct list_head bdi_pending_list
 
 static inline int wb_has_dirty_io(struct bdi_writeback *wb)
 {
-	return !list_empty(&wb->b_dirty) ||
-	       !list_empty(&wb->b_io) ||
-	       !list_empty(&wb->b_more_io);
+	return !list_empty(&wb->b_dirty)	||
+	       !list_empty(&wb->b_io)		||
+	       !list_empty(&wb->b_more_io)	||
+	       !list_empty(&wb->b_more_io_wait);
 }
 
 static inline void __add_bdi_stat(struct backing_dev_info *bdi,

mm/backing-dev.c +8 -2

@@ -74,10 +74,10 @@ static int bdi_debug_stats_show(struct s
 	unsigned long background_thresh;
 	unsigned long dirty_thresh;
 	unsigned long bdi_thresh;
-	unsigned long nr_dirty, nr_io, nr_more_io;
+	unsigned long nr_dirty, nr_io, nr_more_io, nr_more_io_wait;
 	struct inode *inode;
 
-	nr_dirty = nr_io = nr_more_io = 0;
+	nr_dirty = nr_io = nr_more_io = nr_more_io_wait = 0;
 	spin_lock(&wb->list_lock);
 	list_for_each_entry(inode, &wb->b_dirty, i_wb_list)
 		nr_dirty++;

@@ -85,6 +85,8 @@ static int bdi_debug_stats_show(struct s
 		nr_io++;
 	list_for_each_entry(inode, &wb->b_more_io, i_wb_list)
 		nr_more_io++;
+	list_for_each_entry(inode, &wb->b_more_io_wait, i_wb_list)
+		nr_more_io_wait++;
 	spin_unlock(&wb->list_lock);
 
 	global_dirty_limits(&background_thresh, &dirty_thresh);

@@ -103,6 +105,7 @@ static int bdi_debug_stats_show(struct s
 		   "b_dirty:            %10lu\n"
 		   "b_io:               %10lu\n"
 		   "b_more_io:          %10lu\n"
+		   "b_more_io_wait:     %10lu\n"
 		   "bdi_list:           %10u\n"
 		   "state:              %10lx\n",
 		   (unsigned long) K(bdi_stat(bdi, BDI_WRITEBACK)),

@@ -116,6 +119,7 @@ static int bdi_debug_stats_show(struct s
 		   nr_dirty,
 		   nr_io,
 		   nr_more_io,
+		   nr_more_io_wait,
 		   !list_empty(&bdi->bdi_list), bdi->state);
 #undef K

@@ -651,6 +655,7 @@ static void bdi_wb_init(struct bdi_write
 	INIT_LIST_HEAD(&wb->b_dirty);
 	INIT_LIST_HEAD(&wb->b_io);
 	INIT_LIST_HEAD(&wb->b_more_io);
+	INIT_LIST_HEAD(&wb->b_more_io_wait);
 	spin_lock_init(&wb->list_lock);
 	setup_timer(&wb->wakeup_timer, wakeup_timer_fn, (unsigned long)bdi);
 }

@@ -718,6 +723,7 @@ void bdi_destroy(struct backing_dev_info
 		list_splice(&bdi->wb.b_dirty, &dst->b_dirty);
 		list_splice(&bdi->wb.b_io, &dst->b_io);
 		list_splice(&bdi->wb.b_more_io, &dst->b_more_io);
+		list_splice(&bdi->wb.b_more_io_wait, &dst->b_more_io_wait);
 		spin_unlock(&bdi->wb.list_lock);
 		spin_unlock(&dst->list_lock);
 	}

Signature · 2 lines

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

Wu Fengguang

author

2011-10-20 15:22 UTC #3 lore raw

PATCH 2/7 writeback: avoid redirtying when ->write_inode failed to clear I_DIRTY

From: Christoph Hellwig <[email protected]>

Right now ->write_inode has no way to safely return a EAGAIN without explicitly redirtying the inode, as we would lose the dirty state otherwise. Most filesystems get this wrong, but XFS makes heavy use of it to avoid blocking the flusher thread when ->write_inode hits contentended inode locks. A contended ilock is something XFS can hit very easibly when extending files, as the data I/O completion handler takes the lock to update the size, and the ->write_inode call can race with it fairly easily if writing enough data in one go so that the completion for the first write come in just before we call ->write_inode.

Change the handling of this case to use requeue_io_wait for a quick retry instead of redirty_tail, which keeps moving out the dirtied_when data and thus keeps delaying the writeout more and more with every failed attempt to get the lock.

Signed-off-by Christoph Hellwig <[email protected]>

---
 fs/fs-writeback.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

fs/fs-writeback.c +11 -1

@@ -488,8 +488,18 @@ writeback_single_inode(struct inode *ino
 			 * operations, such as delayed allocation during
 			 * submission or metadata updates after data IO
 			 * completion.
+			 *
+			 * For the latter case it is very important to give
+			 * the inode another turn on b_more_io instead of
+			 * redirtying it.  Constantly moving dirtied_when
+			 * forward will prevent us from ever writing out
+			 * the metadata dirtied in the I/O completion handler.
+			 *
+			 * For files on XFS that constantly get appended to
+			 * calling redirty_tail means they will never get
+			 * their updated i_size written out.
 			 */
-			redirty_tail(inode, wb);
+			requeue_io_wait(inode, wb);
 		} else {
 			/*
 			 * The inode is clean.  At this point we either have

Jan Kara

reviewer Acked-by on v1 2/7

2011-10-20 23:24 UTC #4 lore raw

Re: [PATCH 2/7] writeback: avoid redirtying when ->write_inode failed to clear I_DIRTY

Quoted 12 lines · Wu Fengguang

From: Christoph Hellwig <[email protected]>
Right now ->write_inode has no way to safely return a EAGAIN without explicitly redirtying the inode, as we would lose the dirty state otherwise. Most filesystems get this wrong, but XFS makes heavy use of it to avoid blocking the flusher thread when ->write_inode hits contentended inode locks. A contended ilock is something XFS can hit very easibly when extending files, as the data I/O completion handler takes the lock to update the size, and the ->write_inode call can race with it fairly easily if writing enough data in one go so that the completion for the first write come in just before we call ->write_inode.

Quoted 6 lines

Change the handling of this case to use requeue_io_wait for a quick retry instead of redirty_tail, which keeps moving out the dirtied_when data and thus keeps delaying the writeout more and more with every failed attempt to get the lock.
Signed-off-by Christoph Hellwig <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>

You can add:

Honza

Quoted 28 lines

---
 fs/fs-writeback.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

fs/fs-writeback.c +11 -1

@@ -488,8 +488,18 @@ writeback_single_inode(struct inode *ino
 			 * operations, such as delayed allocation during
 			 * submission or metadata updates after data IO
 			 * completion.
+			 *
+			 * For the latter case it is very important to give
+			 * the inode another turn on b_more_io instead of
+			 * redirtying it.  Constantly moving dirtied_when
+			 * forward will prevent us from ever writing out
+			 * the metadata dirtied in the I/O completion handler.
+			 *
+			 * For files on XFS that constantly get appended to
+			 * calling redirty_tail means they will never get
+			 * their updated i_size written out.
 			 */
-			redirty_tail(inode, wb);
+			requeue_io_wait(inode, wb);
 		} else {
 			/*
 			 * The inode is clean.  At this point we either have

Signature · 2 lines

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

Wu Fengguang

author

2011-10-20 15:22 UTC #5 lore raw

PATCH 3/7 writeback: update wb->last_active on written pages/inodes

From: Jan Kara <[email protected]>

The writeback last_active value should be updated on not only written pages, but also cleaned inodes.

Signed-off-by Jan Kara <[email protected]>

---
 fs/fs-writeback.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

fs/fs-writeback.c +7 -6

@@ -744,6 +744,7 @@ static long wb_writeback(struct bdi_writ
 	unsigned long oldest_jif;
 	struct inode *inode;
 	long progress;
+	long total_progress = 0;
 
 	oldest_jif = jiffies;
 	work->older_than_this = &oldest_jif;

@@ -787,6 +788,7 @@ static long wb_writeback(struct bdi_writ
 		else
 			progress = __writeback_inodes_wb(wb, work);
 		trace_writeback_written(wb->bdi, work);
+		total_progress += progress;
 
 		wb_update_bandwidth(wb, wb_start);

@@ -820,7 +822,8 @@ static long wb_writeback(struct bdi_writ
 	}
 	spin_unlock(&wb->list_lock);
 
-	return nr_pages - work->nr_pages;
+	trace_writeback_pages_written(nr_pages - work->nr_pages);
+	return total_progress;
 }
 
 /*

@@ -954,7 +957,7 @@ int bdi_writeback_thread(void *data)
 {
 	struct bdi_writeback *wb = data;
 	struct backing_dev_info *bdi = wb->bdi;
-	long pages_written;
+	long progress;
 
 	current->flags |= PF_SWAPWRITE;
 	set_freezable();

@@ -974,11 +977,9 @@ int bdi_writeback_thread(void *data)
 		 */
 		del_timer(&wb->wakeup_timer);
 
-		pages_written = wb_do_writeback(wb, 0);
-
-		trace_writeback_pages_written(pages_written);
+		progress = wb_do_writeback(wb, 0);
 
-		if (pages_written)
+		if (progress)
 			wb->last_active = jiffies;
 
 		set_current_state(TASK_INTERRUPTIBLE);

Wu Fengguang

author

2011-10-20 15:22 UTC #6 lore raw

PATCH 4/7 writeback: Retry kupdate work early if we need to retry some inode writeback

From: Jan Kara <[email protected]>

In case we could not do any writeback for some inodes, trigger next kupdate work early so that writeback on these inodes is not delayed for the whole dirty_writeback_interval.

Signed-off-by Jan Kara <[email protected]>

---
 fs/fs-writeback.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

fs/fs-writeback.c +13 -4

@@ -886,7 +886,7 @@ static long wb_check_old_data_flush(stru
 
 	expired = wb->last_old_flush +
 			msecs_to_jiffies(dirty_writeback_interval * 10);
-	if (time_before(jiffies, expired))
+	if (time_before(jiffies, expired) && list_empty(&wb->b_more_io_wait))
 		return 0;
 
 	wb->last_old_flush = jiffies;

@@ -958,6 +958,10 @@ int bdi_writeback_thread(void *data)
 	struct bdi_writeback *wb = data;
 	struct backing_dev_info *bdi = wb->bdi;
 	long progress;
+	unsigned int pause = 1;
+	unsigned int max_pause = dirty_writeback_interval ?
+			msecs_to_jiffies(dirty_writeback_interval * 10) :
+			HZ;
 
 	current->flags |= PF_SWAPWRITE;
 	set_freezable();

@@ -979,8 +983,10 @@ int bdi_writeback_thread(void *data)
 
 		progress = wb_do_writeback(wb, 0);
 
-		if (progress)
+		if (progress) {
 			wb->last_active = jiffies;
+			pause = 1;
+		}
 
 		set_current_state(TASK_INTERRUPTIBLE);
 		if (!list_empty(&bdi->work_list) || kthread_should_stop()) {

@@ -988,8 +994,11 @@ int bdi_writeback_thread(void *data)
 			continue;
 		}
 
-		if (wb_has_dirty_io(wb) && dirty_writeback_interval)
-			schedule_timeout(msecs_to_jiffies(dirty_writeback_interval * 10));
+		if (!list_empty(&wb->b_more_io_wait) && pause < max_pause) {
+			schedule_timeout(pause);
+			pause <<= 1;
+		} else if (wb_has_dirty_io(wb) && dirty_writeback_interval)
+			schedule_timeout(max_pause);
 		else {
 			/*
 			 * We have nothing to do, so can go sleep without any

Wu Fengguang

author

2011-10-20 15:22 UTC #7 lore raw

PATCH 5/7 writeback: requeue_io_wait() on pages_skipped inode

Use requeue_io_wait() if some pages were skipped due to locked buffers.

Cc Dave Chinner <[email protected]>

Cc Michael Rubin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

---
 fs/fs-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

@@ -618,11 +618,11 @@ static long writeback_sb_inodes(struct s
 		if (wbc.pages_skipped) {
 			/*
 			 * writeback is not making progress due to locked
 			 * buffers.  Skip this inode for now.
 			 */
-			redirty_tail(inode, wb);
+			requeue_io_wait(inode, wb);
 		}
 		spin_unlock(&inode->i_lock);
 		spin_unlock(&wb->list_lock);
 		iput(inode);
 		cond_resched();

Jan Kara

reviewer Acked-by on v1 5/7

2011-10-20 23:25 UTC #8 lore raw

Re: [PATCH 5/7] writeback: requeue_io_wait() on pages_skipped inode

Quoted 7 lines · Wu Fengguang

Use requeue_io_wait() if some pages were skipped due to locked buffers.
Cc Dave Chinner <[email protected]>
Cc Michael Rubin <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Fengguang Wu <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>

Two signed-off-by?

Otherwise

Honza

Quoted 21 lines

---
 fs/fs-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

@@ -618,11 +618,11 @@ static long writeback_sb_inodes(struct s
 		if (wbc.pages_skipped) {
 			/*
 			 * writeback is not making progress due to locked
 			 * buffers.  Skip this inode for now.
 			 */
-			redirty_tail(inode, wb);
+			requeue_io_wait(inode, wb);
 		}
 		spin_unlock(&inode->i_lock);
 		spin_unlock(&wb->list_lock);
 		iput(inode);
 		cond_resched();

Signature · 2 lines

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

Wu Fengguang

author

2011-10-20 15:22 UTC #9 lore raw

PATCH 6/7 writeback: requeue_io_wait() on blocked inode

Use requeue_io_wait() if inode is somehow blocked.

CC Jan Kara <[email protected]>

CC Dave Chinner <[email protected]>

---
 fs/fs-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

@@ -471,25 +471,25 @@ writeback_single_inode(struct inode *ino
 				/*
 				 * slice used up: queue for next turn
 				 */
 				requeue_io(inode, wb);
 			} else {
 				/*
 				 * Writeback blocked by something other than
 				 * congestion. Delay the inode for some time to
 				 * avoid spinning on the CPU (100% iowait)
 				 * retrying writeback of the dirty page/inode
 				 * that cannot be performed immediately.
 				 */
-				redirty_tail(inode, wb);
+				requeue_io_wait(inode, wb);
 			}
 		} else if (inode->i_state & I_DIRTY) {
 			/*
 			 * Filesystems can dirty the inode during writeback
 			 * operations, such as delayed allocation during
 			 * submission or metadata updates after data IO
 			 * completion.
 			 *
 			 * For the latter case it is very important to give
 			 * the inode another turn on b_more_io instead of
 			 * redirtying it.  Constantly moving dirtied_when
 			 * forward will prevent us from ever writing out

Jan Kara

reviewer Acked-by on v1 6/7

2011-10-20 23:31 UTC #10 lore raw

Re: [PATCH 6/7] writeback: requeue_io_wait() on blocked inode

Quoted 5 lines · Wu Fengguang

Use requeue_io_wait() if inode is somehow blocked.
CC Jan Kara <[email protected]>
CC Dave Chinner <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>

Honza

Quoted 35 lines

---
 fs/fs-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

@@ -471,25 +471,25 @@ writeback_single_inode(struct inode *ino
 				/*
 				 * slice used up: queue for next turn
 				 */
 				requeue_io(inode, wb);
 			} else {
 				/*
 				 * Writeback blocked by something other than
 				 * congestion. Delay the inode for some time to
 				 * avoid spinning on the CPU (100% iowait)
 				 * retrying writeback of the dirty page/inode
 				 * that cannot be performed immediately.
 				 */
-				redirty_tail(inode, wb);
+				requeue_io_wait(inode, wb);
 			}
 		} else if (inode->i_state & I_DIRTY) {
 			/*
 			 * Filesystems can dirty the inode during writeback
 			 * operations, such as delayed allocation during
 			 * submission or metadata updates after data IO
 			 * completion.
 			 *
 			 * For the latter case it is very important to give
 			 * the inode another turn on b_more_io instead of
 			 * redirtying it.  Constantly moving dirtied_when
 			 * forward will prevent us from ever writing out

Signature · 2 lines

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

Wu Fengguang

author

2011-10-20 15:22 UTC #11 lore raw

PATCH 7/7 writeback: requeue_io_wait() when failed to grab superblock

It's some block condition that's not really related to the inode, but still need to move it to b_more_io_wait to prevent possible busy looping.

CC Jan Kara <[email protected]>

---
 fs/fs-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

@@ -652,17 +652,17 @@ static long __writeback_inodes_wb(struct
 		struct super_block *sb = inode->i_sb;
 
 		if (!grab_super_passive(sb)) {
 			/*
 			 * grab_super_passive() may fail consistently due to
 			 * s_umount being grabbed by someone else. Don't use
 			 * requeue_io() to avoid busy retrying the inode/sb.
 			 */
-			redirty_tail(inode, wb);
+			requeue_io_wait(inode, wb);
 			continue;
 		}
 		wrote += writeback_sb_inodes(sb, wb, work);
 		drop_super(sb);
 
 		/* refer to the same tests at the end of writeback_sb_inodes */
 		if (wrote) {
 			if (time_is_before_jiffies(start_time + HZ / 10UL))

Jan Kara

reviewer Acked-by on v1 7/7

2011-10-20 23:25 UTC #12 lore raw

Re: [PATCH 7/7] writeback: requeue_io_wait() when failed to grab superblock

Quoted 5 lines · Wu Fengguang

It's some block condition that's not really related to the inode, but still need to move it to b_more_io_wait to prevent possible busy looping.
CC Jan Kara <[email protected]>
Signed-off-by Wu Fengguang <[email protected]>

Honza

Quoted 27 lines

---
 fs/fs-writeback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/fs-writeback.c +1 -1

@@ -652,17 +652,17 @@ static long __writeback_inodes_wb(struct
 		struct super_block *sb = inode->i_sb;
 
 		if (!grab_super_passive(sb)) {
 			/*
 			 * grab_super_passive() may fail consistently due to
 			 * s_umount being grabbed by someone else. Don't use
 			 * requeue_io() to avoid busy retrying the inode/sb.
 			 */
-			redirty_tail(inode, wb);
+			requeue_io_wait(inode, wb);
 			continue;
 		}
 		wrote += writeback_sb_inodes(sb, wb, work);
 		drop_super(sb);
 
 		/* refer to the same tests at the end of writeback_sb_inodes */
 		if (wrote) {
 			if (time_is_before_jiffies(start_time + HZ / 10UL))

Signature · 2 lines

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

Jan Kara

reviewer

2011-10-20 23:21 UTC #13 lore raw

Hello,

Quoted 14 lines · Wu Fengguang

This tries to keep dirtied_when for blocked inodes by converting some redirty_tail() calls to requeue_io_wait(). The possibly blocked inodes will be moved to b_more_io_wait. The b_more_io_wait inodes will now be retried as dillegent as b_more_io inodes, except when the latter goes empty, b_more_io_wait will be retried by the kupdate work on increasing intervals until exceeding dirty_writeback_interval.
Christoph Hellwig (1):
      writeback: avoid redirtying when ->write_inode failed to clear I_DIRTY
Jan Kara (2):
writeback: update wb->last_active on written pages/inodes
writeback: Retry kupdate work early if we need to retry some inode writeback

Quoted 5 lines

Wu Fengguang (4):
writeback: introduce queue b_more_io_wait
writeback: requeue_io_wait() on pages_skipped inode
writeback: requeue_io_wait() on blocked inode
writeback: requeue_io_wait() when failed to grab superblock

How about adding the attached patch to the series? With it applied we
would have all busyloop prevention done in the same way.

Honza

Signature · 2 lines

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

Quoted 1 line

From 15f3c6f15891121ff8ca06e214f75c4bac9f2f80 Mon Sep 17 00:00:00 2001

From: Jan Kara <[email protected]>
Date: Fri, 21 Oct 2011 01:15:43 +0200
Subject: [PATCH] writeback: requeue_io_wait() when I_SYNC is set

If writeback skips inode because it is already under writeback (I_SYNC flag is set), we can now use requeue_io_wait() instead of requeue_io() and remove special busyloop prevention in wb_writeback().

Signed-off-by Jan Kara <[email protected]>

---
 fs/fs-writeback.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

fs/fs-writeback.c +9 ~~-12~~

index 89b9b70..4e5cae0 100644
@@ -14,6 +14,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/ratelimit.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>

@@ -391,7 +392,7 @@ writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,
 		 * completed a full scan of b_io.
 		 */
 		if (wbc->sync_mode != WB_SYNC_ALL) {
-			requeue_io(inode, wb);
+			requeue_io_wait(inode, wb);
 			trace_writeback_single_inode_requeue(inode, wbc,
 							     nr_to_write);
 			return 0;

@@ -725,7 +726,6 @@ static long wb_writeback(struct bdi_writeback *wb,
 	unsigned long wb_start = jiffies;
 	long nr_pages = work->nr_pages;
 	unsigned long oldest_jif;
-	struct inode *inode;
 	long progress;
 	long total_progress = 0;

@@ -791,17 +791,14 @@ static long wb_writeback(struct bdi_writeback *wb,
 		if (list_empty(&wb->b_more_io))
 			break;
 		/*
-		 * Nothing written. Wait for some inode to
-		 * become available for writeback. Otherwise
-		 * we'll just busyloop.
+		 * Nothing written but some inodes were moved to b_more_io.
+		 * This should not happen as we can easily busyloop.
 		 */
-		if (!list_empty(&wb->b_more_io))  {
-			trace_writeback_wait(wb->bdi, work);
-			inode = wb_inode(wb->b_more_io.prev);
-			spin_lock(&inode->i_lock);
-			inode_wait_for_writeback(inode, wb);
-			spin_unlock(&inode->i_lock);
-		}
+		pr_warn_ratelimited("mm: Possible busyloop in data writeback "
+			"(bdi %s nr_pages %ld sync_mode %d kupdate %d "
+			"background %d)\n",
+			wb->bdi->name, work->nr_pages, work->sync_mode,
+			work->for_kupdate, work->for_background);
 	}
 	spin_unlock(&wb->list_lock);

Signature · 1 line

1.7.1

Wu Fengguang

author

2011-10-21 10:40 UTC #14 lore raw

Hi Jan,

Quoted 2 lines

How about adding the attached patch to the series? With it applied we
would have all busyloop prevention done in the same way.

Sure. It looks good.

Review

Quoted 5 lines

(quoted diff) +5 -0

+		pr_warn_ratelimited("mm: Possible busyloop in data writeback "
+			"(bdi %s nr_pages %ld sync_mode %d kupdate %d "
+			"background %d)\n",
+			wb->bdi->name, work->nr_pages, work->sync_mode,
+			work->for_kupdate, work->for_background);

I'll change the last two fields to the newly introduced writeback "reason":

pr_warn_ratelimited("mm: Possible busyloop in data writeback "
"(bdi %s nr_pages %ld sync_mode %d reason %s)\n", wb->bdi->name, work->nr_pages, work->sync_mode, wb_reason_name[work->reason]);

btw, with the I_SYNC case converted, it's actually no longer necessary to keep a standalone b_more_io_wait. It should still be better to keep the list and the above error check for catching possible errors and the flexibility of adding policies like "don't retry possible blocked inodes in N seconds as long as there are other inodes to work with".

The below diff only intends to show the _possibility_ to remove b_more_io_wait:

fs/fs-writeback.c +2 ~~-28~~

@@ -235,20 +235,7 @@ static void requeue_io(struct inode *ino
 	list_move(&inode->i_wb_list, &wb->b_more_io);
 }
 
-/*
- * The inode should be retried in an opportunistic way.
- *
- * The only difference between b_more_io and b_more_io_wait is:
- * wb_writeback() won't quit as long as b_more_io is not empty.  When
- * wb_writeback() quit on empty b_more_io and non-empty b_more_io_wait,
- * the kupdate work will wakeup more frequently to retry the inodes in
- * b_more_io_wait.
- */
-static void requeue_io_wait(struct inode *inode, struct bdi_writeback *wb)
-{
-	assert_spin_locked(&wb->list_lock);
-	list_move(&inode->i_wb_list, &wb->b_more_io_wait);
-}
+#define requeue_io_wait(inode, wb) requeue_io(inode, wb)
 
 static void inode_sync_complete(struct inode *inode)
 {

@@ -798,21 +785,8 @@ static long wb_writeback(struct bdi_writ
 		 * mean the overall work is done. So we keep looping as long
 		 * as made some progress on cleaning pages or inodes.
 		 */
-		if (progress)
-			continue;
-		/*
-		 * No more inodes for IO, bail
-		 */
-		if (list_empty(&wb->b_more_io))
+		if (!progress)
 			break;
-		/*
-		 * Nothing written but some inodes were moved to b_more_io.
-		 * This should not happen as we can easily busyloop.
-		 */
-		pr_warn_ratelimited("mm: Possible busyloop in data writeback "
-			"(bdi %s nr_pages %ld sync_mode %d reason %s)\n",
-			wb->bdi->name, work->nr_pages, work->sync_mode,
-			wb_reason_name[work->reason]);
 	}
 	spin_unlock(&wb->list_lock);

Thanks,
Fengguang

Jan Kara

reviewer

2011-10-21 19:54 UTC #15 lore raw

Quoted 6 lines · Wu Fengguang

Hi Jan,
Quoted 2 lines
How about adding the attached patch to the series? With it applied we
would have all busyloop prevention done in the same way.
Sure. It looks good.

Thanks.

Quoted 8 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+		pr_warn_ratelimited("mm: Possible busyloop in data writeback "
+			"(bdi %s nr_pages %ld sync_mode %d kupdate %d "
+			"background %d)\n",
+			wb->bdi->name, work->nr_pages, work->sync_mode,
+			work->for_kupdate, work->for_background);
I'll change the last two fields to the newly introduced writeback "reason":

Quoted 4 lines

pr_warn_ratelimited("mm: Possible busyloop in data writeback "
"(bdi %s nr_pages %ld sync_mode %d reason %s)\n", wb->bdi->name, work->nr_pages, work->sync_mode, wb_reason_name[work->reason]);

Makes sense. Thanks.

Quoted 8 lines

btw, with the I_SYNC case converted, it's actually no longer necessary to keep a standalone b_more_io_wait. It should still be better to keep the list and the above error check for catching possible errors and the flexibility of adding policies like "don't retry possible blocked inodes in N seconds as long as there are other inodes to work with".
The below diff only intends to show the _possibility_ to remove b_more_io_wait:

Good observation. So should we introduce b_more_io_wait in the end? We
could always introduce it when the need for some more complicated policy comes...

Honza

Quoted 50 lines

fs/fs-writeback.c +2 ~~-28~~

@@ -235,20 +235,7 @@ static void requeue_io(struct inode *ino
 	list_move(&inode->i_wb_list, &wb->b_more_io);
 }
 
-/*
- * The inode should be retried in an opportunistic way.
- *
- * The only difference between b_more_io and b_more_io_wait is:
- * wb_writeback() won't quit as long as b_more_io is not empty.  When
- * wb_writeback() quit on empty b_more_io and non-empty b_more_io_wait,
- * the kupdate work will wakeup more frequently to retry the inodes in
- * b_more_io_wait.
- */
-static void requeue_io_wait(struct inode *inode, struct bdi_writeback *wb)
-{
-	assert_spin_locked(&wb->list_lock);
-	list_move(&inode->i_wb_list, &wb->b_more_io_wait);
-}
+#define requeue_io_wait(inode, wb) requeue_io(inode, wb)
 
 static void inode_sync_complete(struct inode *inode)
 {

@@ -798,21 +785,8 @@ static long wb_writeback(struct bdi_writ
 		 * mean the overall work is done. So we keep looping as long
 		 * as made some progress on cleaning pages or inodes.
 		 */
-		if (progress)
-			continue;
-		/*
-		 * No more inodes for IO, bail
-		 */
-		if (list_empty(&wb->b_more_io))
+		if (!progress)
 			break;
-		/*
-		 * Nothing written but some inodes were moved to b_more_io.
-		 * This should not happen as we can easily busyloop.
-		 */
-		pr_warn_ratelimited("mm: Possible busyloop in data writeback "
-			"(bdi %s nr_pages %ld sync_mode %d reason %s)\n",
-			wb->bdi->name, work->nr_pages, work->sync_mode,
-			wb_reason_name[work->reason]);
 	}
 	spin_unlock(&wb->list_lock);

Thanks,
Fengguang

Wu Fengguang

author

2011-10-22 03:11 UTC #16 lore raw

Quoted 6 lines

Quoted 6 lines
btw, with the I_SYNC case converted, it's actually no longer necessary to keep a standalone b_more_io_wait. It should still be better to keep the list and the above error check for catching possible errors and the flexibility of adding policies like "don't retry possible blocked inodes in N seconds as long as there are other inodes to work with".

Quoted 6 lines

Quoted 2 lines
The below diff only intends to show the _possibility_ to remove b_more_io_wait:
Good observation. So should we introduce b_more_io_wait in the end? We
could always introduce it when the need for some more complicated policy comes...

I have no problem removing it if you liked it more. Anyway, let me test the idea out first (just kicked off the tests).

Thanks,
Fengguang

Wu Fengguang

author

2011-10-22 05:39 UTC #17 lore raw

Quoted 8 lines · Wu Fengguang

Quoted 8 lines
Quoted 8 lines
btw, with the I_SYNC case converted, it's actually no longer necessary to keep a standalone b_more_io_wait. It should still be better to keep the list and the above error check for catching possible errors and the flexibility of adding policies like "don't retry possible blocked inodes in N seconds as long as there are other inodes to work with".
The below diff only intends to show the _possibility_ to remove b_more_io_wait:

Quoted 7 lines

Quoted 4 lines
Good observation. So should we introduce b_more_io_wait in the end? We
could always introduce it when the need for some more complicated policy comes...
I have no problem removing it if you liked it more. Anyway, let me test the idea out first (just kicked off the tests).

When removing b_more_io_wait, performance is slightly dropped comparing to the full more_io_wait patchset.

3.1.0-rc9-ioless-full-more_io_wait-next-20111014+ 3.1.0-rc9-ioless-full-more_io_wait-x-next-20111014+ ------------------------ ------------------------

                   45.30        +6.3%        48.14  thresh=1G/ext3-1dd-4k-8p-4096M-1024M:10-X
                   48.23        -2.0%        47.27  thresh=1G/ext4-100dd-4k-8p-4096M-1024M:10-X
                   54.21        -2.6%        52.80  thresh=1G/ext4-10dd-4k-8p-4096M-1024M:10-X
                   56.07        -0.3%        55.91  thresh=1G/ext4-1dd-4k-8p-4096M-1024M:10-X
                   45.12        -5.8%        42.49  thresh=1G/xfs-100dd-4k-8p-4096M-1024M:10-X
                   53.94        -1.2%        53.27  thresh=1G/xfs-10dd-4k-8p-4096M-1024M:10-X
                   55.66        -0.1%        55.63  thresh=1G/xfs-1dd-4k-8p-4096M-1024M:10-X
                  358.53        -0.8%       355.51  TOTAL write_bw

I'll try to reduce the changes and retest.

In general it looks better we first root case the "decreasing wrote pages by writeback_single_inode() over time" problem before looking into further steps..

Thanks,
Fengguang

Wu Fengguang

author

2011-10-22 06:59 UTC #18 lore raw

Quoted 3 lines

In general it looks better we first root case the "decreasing wrote pages by writeback_single_inode() over time" problem before looking into further steps..

The problem shows up in many of the regression cases. For example, the thresh=1G/ext4-100dd case with patches 1-4 applied will not only see nr_writeback occasionally dropped low (bdi_dirty_state-8:0.png), but also see repeated pattern of nr_wrote by writeback_single_inode() slowly decreasing to 0 during some period of dozens of seconds (writeback_single_inode.png).

Thanks,
Fengguang

Wu Fengguang

author

2011-10-22 07:07 UTC #19 lore raw

Quoted 10 lines · Wu Fengguang

Quoted 3 lines
In general it looks better we first root case the "decreasing wrote pages by writeback_single_inode() over time" problem before looking into further steps..
The problem shows up in many of the regression cases. For example, the thresh=1G/ext4-100dd case with patches 1-4 applied will not only see nr_writeback occasionally dropped low (bdi_dirty_state-8:0.png), but also see repeated pattern of nr_wrote by writeback_single_inode() slowly decreasing to 0 during some period of dozens of seconds (writeback_single_inode.png).

Another example is the thresh=1G/xfs-100dd case with patches 1-4 and Jan's requeue_io_wait-on-I_SYNC patch. The writeback_single_inode.png graph shows steadily decreasing wrote pages in some period of 400 seconds. It's accompanied by decrease of nr_writeback, IO size and impacts write bandwidth a lot.

Thanks,
Fengguang

Wu Fengguang

author

2011-10-22 07:46 UTC #20 lore raw

Quoted 31 lines · Wu Fengguang

Quoted 8 lines · Wu Fengguang
Quoted 8 lines
Quoted 8 lines
btw, with the I_SYNC case converted, it's actually no longer necessary to keep a standalone b_more_io_wait. It should still be better to keep the list and the above error check for catching possible errors and the flexibility of adding policies like "don't retry possible blocked inodes in N seconds as long as there are other inodes to work with".
The below diff only intends to show the _possibility_ to remove b_more_io_wait:
Quoted 7 lines
Quoted 4 lines
Good observation. So should we introduce b_more_io_wait in the end? We
could always introduce it when the need for some more complicated policy comes...
I have no problem removing it if you liked it more. Anyway, let me test the idea out first (just kicked off the tests).
When removing b_more_io_wait, performance is slightly dropped comparing to the full more_io_wait patchset.
3.1.0-rc9-ioless-full-more_io_wait-next-20111014+ 3.1.0-rc9-ioless-full-more_io_wait-x-next-20111014+ ------------------------ ------------------------
                   45.30        +6.3%        48.14  thresh=1G/ext3-1dd-4k-8p-4096M-1024M:10-X
                   48.23        -2.0%        47.27  thresh=1G/ext4-100dd-4k-8p-4096M-1024M:10-X
                   54.21        -2.6%        52.80  thresh=1G/ext4-10dd-4k-8p-4096M-1024M:10-X
                   56.07        -0.3%        55.91  thresh=1G/ext4-1dd-4k-8p-4096M-1024M:10-X
                   45.12        -5.8%        42.49  thresh=1G/xfs-100dd-4k-8p-4096M-1024M:10-X
                   53.94        -1.2%        53.27  thresh=1G/xfs-10dd-4k-8p-4096M-1024M:10-X
                   55.66        -0.1%        55.63  thresh=1G/xfs-1dd-4k-8p-4096M-1024M:10-X
                  358.53        -0.8%       355.51  TOTAL write_bw

Quoted 1 line

I'll try to reduce the changes and retest.

Unfortunately, the reduced patches 1-4 + I_SYNC change + remove requeue_more_io_wait combination still performances noticeably worse:

3.1.0-rc9-ioless-full-next-20111014+ 3.1.0-rc9-ioless-full-more_io_wait-x2-next-20111014+ ------------------------ ------------------------

                   49.84        -7.9%        45.91  thresh=1G/ext4-100dd-4k-8p-4096M-1024M:10-X
                   56.03        -7.2%        52.01  thresh=1G/ext4-10dd-4k-8p-4096M-1024M:10-X
                   57.42        -1.7%        56.45  thresh=1G/ext4-1dd-4k-8p-4096M-1024M:10-X
                   45.74        -2.8%        44.48  thresh=1G/xfs-100dd-4k-8p-4096M-1024M:10-X
                   54.19        -4.8%        51.57  thresh=1G/xfs-10dd-4k-8p-4096M-1024M:10-X
                   55.93        -2.2%        54.70  thresh=1G/xfs-1dd-4k-8p-4096M-1024M:10-X
                  319.14        -4.4%       305.12  TOTAL write_bw

Thanks,
Fengguang

Wu Fengguang

author

2011-10-22 04:47 UTC #21 lore raw