writeback: avoid redirtying when ->write_inode failed to clear I_DIRTY

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 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]>

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +6 -0

@@ -464,8 +464,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.

Christoph,

Quoted 10 lines · Christoph Hellwig

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 3 lines

Change the handling of this case to use requeue_io 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.

Yeah redirty_tail() does have the problem of possibly delay inodes for too long time. However, you know requeue_io() always has the danger of triggering busy wb_writeback() loops in corner cases.

For example, nfs_write_inode()/nfs_commit_unstable_pages() often redirty the inode without doing anything (hence no any progress, a prerequisite for busy loops) depending on the in flight writes, which unfortunately further depends on _external_ network/server states.. That means some stalled network/sever state could lead to busy loops in NFS clients.

The alternative solution may be to firstly apply the attached patch, and change this one to:

  -			redirty_tail(inode, wb);
  +			requeue_io_wait(inode, wb);

Quoted 16 lines

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

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +6 -0

@@ -464,8 +464,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.

fs/fs-writeback.c writeback_single_inode(struct inode *ino · line 481

Quoted 10 lines

fs/fs-writeback.c +5 -1

+			 *
+			 * 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(inode, wb);
 		} else {
 			/*
 			 * The inode is clean.  At this point we either have

Subject: writeback: introduce queue b_more_io_wait
Date: Sun Jul 31 18:44:44 CST 2011

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.

The new data flow after converting all redirty_tail() calls to requeue_io_wait():

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]>

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

fs/fs-writeback.c +10 -0

@@ -220,6 +220,15 @@ 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.
+ */
+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)
 {
 	/*

@@ -307,6 +316,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, older_than_this);
 	trace_writeback_queue_io(wb, older_than_this, 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 */
 };
 

@@ -129,9 +130,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
 

@@ -635,6 +639,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);
 }

@@ -701,6 +706,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);
 	}

Quoted 28 lines · Wu Fengguang

Christoph,

Quoted 10 lines · Christoph Hellwig

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 3 lines

Change the handling of this case to use requeue_io 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.

Yeah redirty_tail() does have the problem of possibly delay inodes for too long time. However, you know requeue_io() always has the danger of triggering busy wb_writeback() loops in corner cases.

For example, nfs_write_inode()/nfs_commit_unstable_pages() often redirty the inode without doing anything (hence no any progress, a prerequisite for busy loops) depending on the in flight writes, which unfortunately further depends on _external_ network/server states.. That means some stalled network/sever state could lead to busy loops in NFS clients.

Quoted 5 lines

The alternative solution may be to firstly apply the attached patch, and change this one to:

  -			redirty_tail(inode, wb);
  +			requeue_io_wait(inode, wb);

But your patch doesn't solve the busyloop when the problematic inodes are
the only ones under writeback, does it? Then b_more_io and b_more_io_wait are effectively the same if I understand it right.

I think that busylooping in cases like these could be fixed improving the busyloop prevention at the end of the loop in wb_writeback(). Maybe we could just take a short nap before continuting with writeback instead of / in addition to waiting for inode writeback. What do you think?

Quoted 29 lines · Jan Kara

Quoted 28 lines · Wu Fengguang

Christoph,

Quoted 10 lines · Christoph Hellwig

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 3 lines

Change the handling of this case to use requeue_io 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.

Yeah redirty_tail() does have the problem of possibly delay inodes for too long time. However, you know requeue_io() always has the danger of triggering busy wb_writeback() loops in corner cases.

For example, nfs_write_inode()/nfs_commit_unstable_pages() often redirty the inode without doing anything (hence no any progress, a prerequisite for busy loops) depending on the in flight writes, which unfortunately further depends on _external_ network/server states.. That means some stalled network/sever state could lead to busy loops in NFS clients.

Quoted 8 lines

Quoted 5 lines

The alternative solution may be to firstly apply the attached patch, and change this one to:

  -			redirty_tail(inode, wb);
  +			requeue_io_wait(inode, wb);

But your patch doesn't solve the busyloop when the problematic inodes are
the only ones under writeback, does it? Then b_more_io and b_more_io_wait are effectively the same if I understand it right.

The difference lies in the

                /*
                 * No more inodes for IO, bail
                 */
                if (list_empty(&wb->b_more_io))
                        break;

check in wb_writeback(). So when what's left are all b_more_io_wait inodes, the above check will take effect and break the loop. This is the tricky point of the patch: it relies on the code not touched by the patch to work. I've updated the changelog to explain this.

Quoted 4 lines

I think that busylooping in cases like these could be fixed improving the busyloop prevention at the end of the loop in wb_writeback(). Maybe we could just take a short nap before continuting with writeback instead of / in addition to waiting for inode writeback. What do you think?

That's a reasonable robust option, however at the cost of keeping the writeback code in some ambiguous state ;)

---
Subject: writeback: introduce queue b_more_io_wait
Date: Sun Jul 31 18:44:44 CST 2011

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.

The new data flow after converting all redirty_tail() calls to requeue_io_wait():

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]>

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

fs/fs-writeback.c +10 -0

@@ -220,6 +220,15 @@ 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.
+ */
+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)
 {
 	/*

@@ -307,6 +316,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, older_than_this);
 	trace_writeback_queue_io(wb, older_than_this, 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 */
 };
 

@@ -129,9 +130,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
 

@@ -635,6 +639,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);
 }

@@ -701,6 +706,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);
 	}

Quoted 47 lines · Wu Fengguang

Quoted 29 lines · Jan Kara

Quoted 28 lines · Wu Fengguang

Christoph,

Quoted 10 lines · Christoph Hellwig

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 3 lines

Change the handling of this case to use requeue_io 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.

Yeah redirty_tail() does have the problem of possibly delay inodes for too long time. However, you know requeue_io() always has the danger of triggering busy wb_writeback() loops in corner cases.

For example, nfs_write_inode()/nfs_commit_unstable_pages() often redirty the inode without doing anything (hence no any progress, a prerequisite for busy loops) depending on the in flight writes, which unfortunately further depends on _external_ network/server states.. That means some stalled network/sever state could lead to busy loops in NFS clients.

Quoted 8 lines

Quoted 5 lines

The alternative solution may be to firstly apply the attached patch, and change this one to:

  -			redirty_tail(inode, wb);
  +			requeue_io_wait(inode, wb);

But your patch doesn't solve the busyloop when the problematic inodes are
the only ones under writeback, does it? Then b_more_io and b_more_io_wait are effectively the same if I understand it right.

The difference lies in the

                /*
                 * No more inodes for IO, bail
                 */
                if (list_empty(&wb->b_more_io))
                        break;

Quoted 4 lines

check in wb_writeback(). So when what's left are all b_more_io_wait inodes, the above check will take effect and break the loop. This is the tricky point of the patch: it relies on the code not touched by the patch to work. I've updated the changelog to explain this.

Argh, that's really subtle! Two points here.

1) We will immediately retry inodes in b_more_io_wait list because of

	if (progress)
		continue;

check.

2) The time writeout will be delayed is only <=dirty_expire_centisecs but can be arbitrarily small if someone submits more work.

Also if wb_writeback() was called from wb_do_writeback(), we will retry b_more_io_wait inodes twice immediately because of wb_check_old_data_flush() and wb_check_background_flush() calls.

Quoted 7 lines

Quoted 4 lines

I think that busylooping in cases like these could be fixed improving the busyloop prevention at the end of the loop in wb_writeback(). Maybe we could just take a short nap before continuting with writeback instead of / in addition to waiting for inode writeback. What do you think?

That's a reasonable robust option, however at the cost of keeping the writeback code in some ambiguous state ;)

What do you exactly mean by ambiguous state?

I don't see anything ambiguous in waiting for a jiffie or so. Not that I'd be completely happy about "just wait for a while and see if things are better" but your solution does not seem ideal either...

Quoted 3 lines

Quoted 2 lines

That's a reasonable robust option, however at the cost of keeping the writeback code in some ambiguous state ;)

What do you exactly mean by ambiguous state?

I mean in Christoph's case, it will be calling requeue_io() and at the same time rely on your suggested unconditional sleep at the end of wb_writeback() loop to avoid busy loop. Or in other words, b_more_io will be holding both inodes that should be busy retried and the inodes to be opportunistically retried. However I admit it's not a big problem if we take b_more_io as general "to be retried ASAP".

Quoted 3 lines

I don't see anything ambiguous in waiting for a jiffie or so. Not that I'd be completely happy about "just wait for a while and see if things are better" but your solution does not seem ideal either...

There are no big differences (that matter) in terms of "how much exact time to wait" in this XFS case. What make me prefer b_more_io_wait is that it looks a more general solution to replace the majority redirty_tail() calls to avoid modifying dirtied_when.

Quoted 15 lines · Wu Fengguang

Quoted 3 lines

Quoted 2 lines

That's a reasonable robust option, however at the cost of keeping the writeback code in some ambiguous state ;)

What do you exactly mean by ambiguous state?

I mean in Christoph's case, it will be calling requeue_io() and at the same time rely on your suggested unconditional sleep at the end of wb_writeback() loop to avoid busy loop. Or in other words, b_more_io will be holding both inodes that should be busy retried and the inodes to be opportunistically retried. However I admit it's not a big problem if we take b_more_io as general "to be retried ASAP".

Quoted 3 lines

I don't see anything ambiguous in waiting for a jiffie or so. Not that I'd be completely happy about "just wait for a while and see if things are better" but your solution does not seem ideal either...

Quoted 4 lines

There are no big differences (that matter) in terms of "how much exact time to wait" in this XFS case. What make me prefer b_more_io_wait is that it looks a more general solution to replace the majority redirty_tail() calls to avoid modifying dirtied_when.

FYI, we had a few more users hit this issue recently. I'm not sure why, but we are seeing this fairly often now. I'd really like to get some sort of fix for this in ASAP as it causes data loss for users.

Quoted 21 lines · Christoph Hellwig

Quoted 19 lines · Wu Fengguang

Quoted 3 lines

Quoted 2 lines

That's a reasonable robust option, however at the cost of keeping the writeback code in some ambiguous state ;)

What do you exactly mean by ambiguous state?

I mean in Christoph's case, it will be calling requeue_io() and at the same time rely on your suggested unconditional sleep at the end of wb_writeback() loop to avoid busy loop. Or in other words, b_more_io will be holding both inodes that should be busy retried and the inodes to be opportunistically retried. However I admit it's not a big problem if we take b_more_io as general "to be retried ASAP".

Quoted 3 lines

I don't see anything ambiguous in waiting for a jiffie or so. Not that I'd be completely happy about "just wait for a while and see if things are better" but your solution does not seem ideal either...

There are no big differences (that matter) in terms of "how much exact time to wait" in this XFS case. What make me prefer b_more_io_wait is that it looks a more general solution to replace the majority redirty_tail() calls to avoid modifying dirtied_when.

Quoted 3 lines

FYI, we had a few more users hit this issue recently. I'm not sure why, but we are seeing this fairly often now. I'd really like to get some sort of fix for this in ASAP as it causes data loss for users.

Jan, do you agree to push the b_more_io_wait patch into linux-next?

If not, let's do a patch to do unconditional sleep at the end of the wb_writeback() loop?