Ian Kent wrote:
On Mon, 23 Mar 2009, Jeff Layton wrote:
This may be a problem on other filesystems too, but the reproducer I
have involves NFS.
On NFS, the __mark_inode_dirty() call after writing back the inode is
done in the rpc_release handler for COMMIT calls. This call is done
asynchronously after the call completes.
Because there's no real coordination between __mark_inode_dirty() and
__sync_single_inode(), it's often the case that these two calls will
race and __mark_inode_dirty() will get called while I_SYNC is still set.
When this happens, __sync_single_inode() should detect that the inode
was redirtied while we were flushing it and call redirty_tail() to put
it back on the s_dirty list.
When redirty_tail() puts it back on the list, it only resets the
dirtied_when value if it's necessary to maintain the list order. Given
the right situation (the right I/O patterns and a lot of luck), this
could result in dirtied_when never getting updated on an inode that's
constantly being redirtied while pdflush is writing it back.
Since dirtied_when is based on jiffies, it's possible for it to persist
across 2 sign-bit flips of jiffies. When that happens, the time_after()
check in sync_sb_inodes no longer works correctly and writeouts by
pdflush of this inode and any inodes after it on the list stop.
This patch fixes this by resetting the dirtied_when value on an inode
when we're adding it back onto an empty s_dirty list. Since we generally
write inodes from oldest to newest dirtied_when values, this has the
effect of making it so that these inodes don't end up with dirtied_when
values that are frozen.
I've also taken the liberty of fixing up the comments a bit and changed
the !time_after_eq() check in redirty_tail to be time_before(). That
should be functionally equivalent but I think it's more readable.
I wish this were just a theoretical problem, but we've had a customer
hit a variant of it in an older kernel. Newer upstream kernels have a
number of changes that make this problem less likely. As best I can tell
though, there is nothing that really prevents it.
Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
Acked-by: Ian Kent <raven@xxxxxxxxxx>
The assumption is that all inodes heading for the s_dirty list will get
their by calling redirty_tail(). It looks like that's the case but, Andrew
do you agree the assumption holds?
Oh .. hang on, that's now quite right.
Or that dirtied_when has been set to jiffies at the time of the move
(aka a newly dirtied inode).
---
fs/fs-writeback.c | 22 +++++++++++++++++-----
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index e3fe991..bd2a7ff 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -184,19 +184,31 @@ static int write_inode(struct inode *inode, int sync)
* furthest end of its superblock's dirty-inode list.
*
* Before stamping the inode's ->dirtied_when, we check to see whether it is
- * already the most-recently-dirtied inode on the s_dirty list. If that is
- * the case then the inode must have been redirtied while it was being written
- * out and we don't reset its dirtied_when.
+ * "newer" or equal to that of the most-recently-dirtied inode on the s_dirty
+ * list. If that is the case then we don't need to restamp it to maintain the
+ * order of the list.
+ *
+ * If s_dirty is empty however, then we need to go ahead and update
+ * dirtied_when for the inode. Not doing so will mean that inodes that are
+ * constantly being redirtied can end up with "stuck" dirtied_when values if
+ * they happen to consistently be the first one to go back on the list.
+ *
+ * Since we're using jiffies values in that field, letting dirtied_when grow
+ * too old will be problematic if jiffies wraps. It may also be causing
+ * pdflush to flush the inode too often since it'll always look like it was
+ * dirtied a long time ago.
*/
static void redirty_tail(struct inode *inode)
{
struct super_block *sb = inode->i_sb;
- if (!list_empty(&sb->s_dirty)) {
+ if (list_empty(&sb->s_dirty)) {
+ inode->dirtied_when = jiffies;
+ } else {
struct inode *tail_inode;
tail_inode = list_entry(sb->s_dirty.next, struct inode, i_list);
- if (!time_after_eq(inode->dirtied_when,
+ if (time_before(inode->dirtied_when,
tail_inode->dirtied_when))
inode->dirtied_when = jiffies;
}
--
1.6.0.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
