[PATCHv4 00/17] kill unnecessary SB sync wake-ups

[PATCHv4 00/17] kill unnecessary SB sync wake-ups

[Artem Bityutskiy]

Hi,

last year I attempted to upstream these patches three times with no luck [1].
Here is the fourth attempt.

The problem: the 'sync_supers' kernel thread wakes up every 5 seconds (by
default, this is configurable via '/proc/sys/vm/dirty_writeback_centisecs')
walks all superblocks and synchronizes them. And even when there are no dirty
superblocks, the thread still keeps waking up.

This is a problem for small battery-powered embedded devices, like the Nokia
N900 phone, Android/MeeGo/etc stuff, etc. In these devices it is important
to have as few wake-ups as possible in order to make sure the CPU is sleeping
for as long time as possible, and the battery power is saved.

The following set of patches tries to address this problem by introducing
wrappers for the 's_dirt' flag manipulations, and then making the 'sync_supers'
task wake up only if there is are dirty superblocks.

I've made patches to be per-FS, and added all the FS maintainers to Cc. But if I
submit this series again, I will not Cc the FS maintainers, to lessen the mount
of spam in their mailboxes. Also, I can merge all the "do not use s_dirty flag
directly" patches, if this is preferred.

[1] the previous attempt: http://marc.info/?l=linux-kernel&m=124712270723226&w=2

[PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

This patch introduces 3 new VFS helpers: 'mark_sb_dirty()',
'mark_sb_clean()', and 'is_sb_dirty()'. The helpers simply
set 'sb->s_dirt' or test 'sb->s_dirt'. The plan is to make
every FS use these helpers instead of manipulating the
'sb->s_dirt' flag directly.

Ultimately, this change is a preparation for the periodic
superblock synchronization optimization which is about
preventing the "sync_supers" kernel thread from waking up
even if there is nothing to synchronize.

This patch also makes VFS use the new helpers.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Tigran A. Aivazian <tigran@aivazian.fsnet.co.uk>
Cc: Chris Mason <chris.mason@oracle.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: linux-ext4@vger.kernel.org
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: reiserfs-devel@vger.kernel.org
Cc: Jan Kara <jack@suse.cz>
Cc: Evgeniy Dushistov <dushistov@mail.ru>
---
 fs/super.c         |    4 ++--
 fs/sync.c          |    2 +-
 include/linux/fs.h |   17 +++++++++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 69688b1..2b418fb 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -368,12 +368,12 @@ void sync_supers(void)
 	list_for_each_entry_safe(sb, n, &super_blocks, s_list) {
 		if (list_empty(&sb->s_instances))
 			continue;
-		if (sb->s_op->write_super && sb->s_dirt) {
+		if (sb->s_op->write_super && is_sb_dirty(sb)) {
 			sb->s_count++;
 			spin_unlock(&sb_lock);
 
 			down_read(&sb->s_umount);
-			if (sb->s_root && sb->s_dirt)
+			if (sb->s_root && is_sb_dirty(sb))
 				sb->s_op->write_super(sb);
 			up_read(&sb->s_umount);
 
diff --git a/fs/sync.c b/fs/sync.c
index e8cbd41..782e466 100644
--- a/fs/sync.c
+++ b/fs/sync.c
@@ -144,7 +144,7 @@ int file_fsync(struct file *filp, struct dentry *dentry, int datasync)
 
 	/* sync the superblock to buffers */
 	sb = inode->i_sb;
-	if (sb->s_dirt && sb->s_op->write_super)
+	if (is_sb_dirty(sb) && sb->s_op->write_super)
 		sb->s_op->write_super(sb);
 
 	/* .. finally sync the buffers to disk */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b336cb9..21fe2b3 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1782,6 +1782,23 @@ extern int get_sb_pseudo(struct file_system_type *, char *,
 	struct vfsmount *mnt);
 extern void simple_set_mnt(struct vfsmount *mnt, struct super_block *sb);
 
+/*
+ * Note, VFS does not provide any serialization for the super block clean/dirty
+ * state changes, file-systems should take care of this.
+ */
+static inline void mark_sb_dirty(struct super_block *sb)
+{
+	sb->s_dirt = 1;
+}
+static inline void mark_sb_clean(struct super_block *sb)
+{
+	sb->s_dirt = 0;
+}
+static inline int is_sb_dirty(struct super_block *sb)
+{
+	return sb->s_dirt;
+}
+
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
 #define fops_get(fops) \
 	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
-- 
1.6.6.1

[PATCHv4 02/17] AFFS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Roman Zippel <zippel@linux-m68k.org>
---
 fs/affs/bitmap.c |    4 ++--
 fs/affs/super.c  |   14 +++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/affs/bitmap.c b/fs/affs/bitmap.c
index 3e26271..e55ecde 100644
--- a/fs/affs/bitmap.c
+++ b/fs/affs/bitmap.c
@@ -103,7 +103,7 @@ affs_free_block(struct super_block *sb, u32 block)
 	*(__be32 *)bh->b_data = cpu_to_be32(tmp - mask);
 
 	mark_buffer_dirty(bh);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	bm->bm_free++;
 
 	mutex_unlock(&sbi->s_bmlock);
@@ -248,7 +248,7 @@ find_bit:
 	*(__be32 *)bh->b_data = cpu_to_be32(tmp + mask);
 
 	mark_buffer_dirty(bh);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	mutex_unlock(&sbi->s_bmlock);
 
diff --git a/fs/affs/super.c b/fs/affs/super.c
index 16a3e47..ea8d0cd 100644
--- a/fs/affs/super.c
+++ b/fs/affs/super.c
@@ -69,9 +69,13 @@ affs_write_super(struct super_block *sb)
 		//		if (buffer_dirty(sbi->s_bitmap[i].bm_bh)) {
 		//			clean = 0;
 		affs_commit_super(sb, clean);
-		sb->s_dirt = !clean;	/* redo until bitmap synced */
+		/* redo until bitmap synced */
+		if (clean)
+			mark_sb_clean(sb);
+		else
+			mark_sb_dirty(sb);
 	} else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 	unlock_super(sb);
 
 	pr_debug("AFFS: write_super() at %lu, clean=%d\n", get_seconds(), clean);
@@ -82,7 +86,7 @@ affs_sync_fs(struct super_block *sb, int wait)
 {
 	lock_super(sb);
 	affs_commit_super(sb, 2);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 	unlock_super(sb);
 	return 0;
 }
@@ -554,8 +558,8 @@ affs_remount(struct super_block *sb, int *flags, char *data)
 		return 0;
 	}
 	if (*flags & MS_RDONLY) {
-		sb->s_dirt = 1;
-		while (sb->s_dirt)
+		mark_sb_dirty(sb);
+		while (is_sb_dirty(sb))
 			affs_write_super(sb);
 		affs_free_bitmap(sb);
 	} else
-- 
1.6.6.1

[PATCHv4 03/17] BFS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Tigran A. Aivazian <tigran@aivazian.fsnet.co.uk>
---
 fs/bfs/inode.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/bfs/inode.c b/fs/bfs/inode.c
index f22a7d3..080115e 100644
--- a/fs/bfs/inode.c
+++ b/fs/bfs/inode.c
@@ -223,7 +223,7 @@ static int bfs_sync_fs(struct super_block *sb, int wait)
 
 	mutex_lock(&info->bfs_lock);
 	mark_buffer_dirty(info->si_sbh);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 	mutex_unlock(&info->bfs_lock);
 
 	return 0;
@@ -234,7 +234,7 @@ static void bfs_write_super(struct super_block *sb)
 	if (!(sb->s_flags & MS_RDONLY))
 		bfs_sync_fs(sb, 1);
 	else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 }
 
 static void bfs_put_super(struct super_block *s)
@@ -246,7 +246,7 @@ static void bfs_put_super(struct super_block *s)
 
 	lock_kernel();
 
-	if (s->s_dirt)
+	if (is_sb_dirty(s))
 		bfs_write_super(s);
 
 	brelse(info->si_sbh);
@@ -474,7 +474,7 @@ static int bfs_fill_super(struct super_block *s, void *data, int silent)
 	brelse(bh);
 	if (!(s->s_flags & MS_RDONLY)) {
 		mark_buffer_dirty(info->si_sbh);
-		s->s_dirt = 1;
+		mark_sb_dirty(s);
 	} 
 	dump_imap("read_super", s);
 	return 0;
-- 
1.6.6.1

[PATCHv4 04/17] BTRFS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Chris Mason <chris.mason@oracle.com>
---
 fs/btrfs/inode.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d601629..bb76735 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2577,7 +2577,7 @@ int btrfs_unlink_subvol(struct btrfs_trans_handle *trans,
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME;
 	ret = btrfs_update_inode(trans, root, dir);
 	BUG_ON(ret);
-	dir->i_sb->s_dirt = 1;
+	mark_sb_dirty(dir->i_sb);
 
 	btrfs_free_path(path);
 	return 0;
-- 
1.6.6.1

[PATCHv4 05/17] EXOFS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/file.c  |    2 +-
 fs/exofs/inode.c |    2 +-
 fs/exofs/super.c |    6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/exofs/file.c b/fs/exofs/file.c
index 839b9dc..be38834 100644
--- a/fs/exofs/file.c
+++ b/fs/exofs/file.c
@@ -58,7 +58,7 @@ static int exofs_file_fsync(struct file *filp, struct dentry *dentry,
 	/* This is a good place to write the sb */
 	/* TODO: Sechedule an sb-sync on create */
 	sb = inode->i_sb;
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		exofs_sync_fs(sb, 1);
 
 	return ret;
diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index d7c6afa..0195dcc 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -1122,7 +1122,7 @@ struct inode *exofs_new_inode(struct inode *dir, int mode)
 
 	sbi = sb->s_fs_info;
 
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	inode_init_owner(inode, dir, mode);
 	inode->i_ino = sbi->s_nextid++;
 	inode->i_blkbits = EXOFS_BLKSHIFT;
diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 03149b9..91a32ea 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -237,7 +237,7 @@ int exofs_sync_fs(struct super_block *sb, int wait)
 		EXOFS_ERR("%s: exofs_sbi_write failed.\n", __func__);
 		goto out;
 	}
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 
 out:
 	EXOFS_DBGMSG("s_nextid=0x%llx ret=%d\n", _LLU(sbi->s_nextid), ret);
@@ -251,7 +251,7 @@ static void exofs_write_super(struct super_block *sb)
 	if (!(sb->s_flags & MS_RDONLY))
 		exofs_sync_fs(sb, 1);
 	else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 }
 
 static void _exofs_print_device(const char *msg, const char *dev_path,
@@ -286,7 +286,7 @@ static void exofs_put_super(struct super_block *sb)
 	int num_pend;
 	struct exofs_sb_info *sbi = sb->s_fs_info;
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		exofs_write_super(sb);
 
 	/* make sure there are no pending commands */
-- 
1.6.6.1

[PATCHv4 06/17] EXT2: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: linux-ext4@vger.kernel.org
---
 fs/ext2/balloc.c |    4 ++--
 fs/ext2/ialloc.c |    4 ++--
 fs/ext2/super.c  |    6 +++---
 fs/ext2/xattr.c  |    2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index e8766a3..f2e830e 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -165,7 +165,7 @@ static void release_blocks(struct super_block *sb, int count)
 		struct ext2_sb_info *sbi = EXT2_SB(sb);
 
 		percpu_counter_add(&sbi->s_freeblocks_counter, count);
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 	}
 }
 
@@ -180,7 +180,7 @@ static void group_adjust_blocks(struct super_block *sb, int group_no,
 		free_blocks = le16_to_cpu(desc->bg_free_blocks_count);
 		desc->bg_free_blocks_count = cpu_to_le16(free_blocks + count);
 		spin_unlock(sb_bgl_lock(sbi, group_no));
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 		mark_buffer_dirty(bh);
 	}
 }
diff --git a/fs/ext2/ialloc.c b/fs/ext2/ialloc.c
index 938dbc7..bd2a472 100644
--- a/fs/ext2/ialloc.c
+++ b/fs/ext2/ialloc.c
@@ -81,7 +81,7 @@ static void ext2_release_inode(struct super_block *sb, int group, int dir)
 	spin_unlock(sb_bgl_lock(EXT2_SB(sb), group));
 	if (dir)
 		percpu_counter_dec(&EXT2_SB(sb)->s_dirs_counter);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	mark_buffer_dirty(bh);
 }
 
@@ -547,7 +547,7 @@ got:
 	}
 	spin_unlock(sb_bgl_lock(sbi, group));
 
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	mark_buffer_dirty(bh2);
 	if (test_opt(sb, GRPID)) {
 		inode->i_mode = mode;
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 71e9eb1..b8e23d3 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -119,7 +119,7 @@ static void ext2_put_super (struct super_block * sb)
 	int i;
 	struct ext2_sb_info *sbi = EXT2_SB(sb);
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		ext2_write_super(sb);
 
 	ext2_xattr_put_super(sb);
@@ -1147,7 +1147,7 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es,
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
 	if (wait)
 		sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 }
 
 /*
@@ -1181,7 +1181,7 @@ void ext2_write_super(struct super_block *sb)
 	if (!(sb->s_flags & MS_RDONLY))
 		ext2_sync_fs(sb, 1);
 	else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 }
 
 static int ext2_remount (struct super_block * sb, int * flags, char * data)
diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index 7c39157..b2e9cfb 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -348,7 +348,7 @@ static void ext2_xattr_update_super_block(struct super_block *sb)
 	spin_lock(&EXT2_SB(sb)->s_lock);
 	EXT2_SET_COMPAT_FEATURE(sb, EXT2_FEATURE_COMPAT_EXT_ATTR);
 	spin_unlock(&EXT2_SB(sb)->s_lock);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
 }
 
-- 
1.6.6.1

[PATCHv4 07/17] EXT4: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: linux-ext4@vger.kernel.org
Cc: Theodore Ts'o <tytso@mit.edu>
---
 fs/ext4/balloc.c  |    2 +-
 fs/ext4/file.c    |    2 +-
 fs/ext4/ialloc.c  |    4 ++--
 fs/ext4/inode.c   |    2 +-
 fs/ext4/mballoc.c |    4 ++--
 fs/ext4/resize.c  |    4 ++--
 fs/ext4/super.c   |    6 +++---
 fs/ext4/xattr.c   |    2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index d2f37a5..36b35cf 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -477,7 +477,7 @@ void ext4_add_groupblocks(handle_t *handle, struct super_block *sb,
 	ret = ext4_handle_dirty_metadata(handle, NULL, gd_bh);
 	if (!err)
 		err = ret;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 error_return:
 	brelse(bitmap_bh);
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index d0776e4..edef79f 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -123,7 +123,7 @@ static int ext4_file_open(struct inode * inode, struct file * filp)
 		if (!IS_ERR(cp)) {
 			memcpy(sbi->s_es->s_last_mounted, cp,
 			       sizeof(sbi->s_es->s_last_mounted));
-			sb->s_dirt = 1;
+			mark_sb_dirty(sb);
 		}
 	}
 	return dquot_file_open(inode, filp);
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 1a0e183..2de625e 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -289,7 +289,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode)
 	err = ext4_handle_dirty_metadata(handle, NULL, bitmap_bh);
 	if (!fatal)
 		fatal = err;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 error_return:
 	brelse(bitmap_bh);
 	ext4_std_error(sb, fatal);
@@ -972,7 +972,7 @@ got:
 	percpu_counter_dec(&sbi->s_freeinodes_counter);
 	if (S_ISDIR(mode))
 		percpu_counter_inc(&sbi->s_dirs_counter);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	if (sbi->s_log_groups_per_flex) {
 		flex_group = ext4_flex_group(sbi, group);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 3e0f6af..158c6d9 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5276,7 +5276,7 @@ static int ext4_do_update_inode(handle_t *handle,
 			ext4_update_dynamic_rev(sb);
 			EXT4_SET_RO_COMPAT_FEATURE(sb,
 					EXT4_FEATURE_RO_COMPAT_LARGE_FILE);
-			sb->s_dirt = 1;
+			mark_sb_dirty(sb);
 			ext4_handle_sync(handle);
 			err = ext4_handle_dirty_metadata(handle, NULL,
 					EXT4_SB(sb)->s_sbh);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index b423a36..1f363a0 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -2762,7 +2762,7 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
 	err = ext4_handle_dirty_metadata(handle, NULL, gdp_bh);
 
 out_err:
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	brelse(bitmap_bh);
 	return err;
 }
@@ -4630,7 +4630,7 @@ do_more:
 		put_bh(bitmap_bh);
 		goto do_more;
 	}
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 error_return:
 	if (freed)
 		dquot_free_block(inode, freed);
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 5692c48..fe7cc90 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -921,7 +921,7 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
 	}
 
 	ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 exit_journal:
 	mutex_unlock(&sbi->s_resize_lock);
@@ -1045,7 +1045,7 @@ int ext4_group_extend(struct super_block *sb, struct ext4_super_block *es,
 	}
 	ext4_blocks_count_set(es, o_blocks_count + add);
 	ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	mutex_unlock(&EXT4_SB(sb)->s_resize_lock);
 	ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count,
 		   o_blocks_count + add);
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e14d22c..b4ebc48 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -650,7 +650,7 @@ static void ext4_put_super(struct super_block *sb)
 
 	lock_super(sb);
 	lock_kernel();
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		ext4_commit_super(sb, 1);
 
 	if (sbi->s_journal) {
@@ -2680,7 +2680,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 #else
 		es->s_flags |= cpu_to_le32(EXT2_FLAGS_SIGNED_HASH);
 #endif
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 	}
 
 	if (sbi->s_blocks_per_group > blocksize * 8) {
@@ -3387,7 +3387,7 @@ static int ext4_commit_super(struct super_block *sb, int sync)
 					&EXT4_SB(sb)->s_freeblocks_counter));
 	es->s_free_inodes_count = cpu_to_le32(percpu_counter_sum_positive(
 					&EXT4_SB(sb)->s_freeinodes_counter));
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 	BUFFER_TRACE(sbh, "marking dirty");
 	mark_buffer_dirty(sbh);
 	if (sync) {
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 2de0e95..a8032a5 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -460,7 +460,7 @@ static void ext4_xattr_update_super_block(handle_t *handle,
 
 	if (ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh) == 0) {
 		EXT4_SET_COMPAT_FEATURE(sb, EXT4_FEATURE_COMPAT_EXT_ATTR);
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 		ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
 	}
 }
-- 
1.6.6.1

[PATCHv4 08/17] FAT: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/fat/fatent.c |    8 ++++----
 fs/fat/inode.c  |    8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index 81184d3..4a63301 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -498,7 +498,7 @@ int fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster)
 				sbi->prev_free = entry;
 				if (sbi->free_clusters != -1)
 					sbi->free_clusters--;
-				sb->s_dirt = 1;
+				mark_sb_dirty(sb);
 
 				cluster[idx_clus] = entry;
 				idx_clus++;
@@ -520,7 +520,7 @@ int fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster)
 	/* Couldn't allocate the free entries */
 	sbi->free_clusters = 0;
 	sbi->free_clus_valid = 1;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	err = -ENOSPC;
 
 out:
@@ -586,7 +586,7 @@ int fat_free_clusters(struct inode *inode, int cluster)
 		ops->ent_put(&fatent, FAT_ENT_FREE);
 		if (sbi->free_clusters != -1) {
 			sbi->free_clusters++;
-			sb->s_dirt = 1;
+			mark_sb_dirty(sb);
 		}
 
 		if (nr_bhs + fatent.nr_bhs > MAX_BUF_PER_PAGE) {
@@ -676,7 +676,7 @@ int fat_count_free_clusters(struct super_block *sb)
 	}
 	sbi->free_clusters = free;
 	sbi->free_clus_valid = 1;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	fatent_brelse(&fatent);
 out:
 	unlock_fat(sbi);
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 0ce143b..ea634fe 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -442,7 +442,7 @@ static void fat_clear_inode(struct inode *inode)
 static void fat_write_super(struct super_block *sb)
 {
 	lock_super(sb);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 
 	if (!(sb->s_flags & MS_RDONLY))
 		fat_clusters_flush(sb);
@@ -453,9 +453,9 @@ static int fat_sync_fs(struct super_block *sb, int wait)
 {
 	int err = 0;
 
-	if (sb->s_dirt) {
+	if (is_sb_dirty(sb)) {
 		lock_super(sb);
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 		err = fat_clusters_flush(sb);
 		unlock_super(sb);
 	}
@@ -469,7 +469,7 @@ static void fat_put_super(struct super_block *sb)
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		fat_write_super(sb);
 
 	iput(sbi->fat_inode);
-- 
1.6.6.1

[PATCHv4 09/17] HFS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Roman Zippel <zippel@linux-m68k.org>
---
 fs/hfs/extent.c |    2 +-
 fs/hfs/hfs_fs.h |    2 +-
 fs/hfs/inode.c  |    6 +++---
 fs/hfs/super.c  |    6 +++---
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index 2c16316..bff476a 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -432,7 +432,7 @@ out:
 		if (inode->i_ino < HFS_FIRSTUSER_CNID)
 			set_bit(HFS_FLG_ALT_MDB_DIRTY, &HFS_SB(sb)->flags);
 		set_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags);
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 	}
 	return res;
 
diff --git a/fs/hfs/hfs_fs.h b/fs/hfs/hfs_fs.h
index fe35e3b..2083002 100644
--- a/fs/hfs/hfs_fs.h
+++ b/fs/hfs/hfs_fs.h
@@ -251,7 +251,7 @@ static inline const char *hfs_mdb_name(struct super_block *sb)
 static inline void hfs_bitmap_dirty(struct super_block *sb)
 {
 	set_bit(HFS_FLG_BITMAP_DIRTY, &HFS_SB(sb)->flags);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 }
 
 static inline void hfs_buffer_sync(struct buffer_head *bh)
diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index 14f5cb1..74cb771 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -198,7 +198,7 @@ struct inode *hfs_new_inode(struct inode *dir, struct qstr *name, int mode)
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
 	set_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	return inode;
 }
@@ -213,7 +213,7 @@ void hfs_delete_inode(struct inode *inode)
 		if (HFS_I(inode)->cat_key.ParID == cpu_to_be32(HFS_ROOT_CNID))
 			HFS_SB(sb)->root_dirs--;
 		set_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags);
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 		return;
 	}
 	HFS_SB(sb)->file_count--;
@@ -226,7 +226,7 @@ void hfs_delete_inode(struct inode *inode)
 		}
 	}
 	set_bit(HFS_FLG_MDB_DIRTY, &HFS_SB(sb)->flags);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 }
 
 void hfs_inode_read_fork(struct inode *inode, struct hfs_extent *ext,
diff --git a/fs/hfs/super.c b/fs/hfs/super.c
index 0a81eb7..3fc4e9f 100644
--- a/fs/hfs/super.c
+++ b/fs/hfs/super.c
@@ -52,7 +52,7 @@ MODULE_LICENSE("GPL");
 static void hfs_write_super(struct super_block *sb)
 {
 	lock_super(sb);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 
 	/* sync everything to the buffers */
 	if (!(sb->s_flags & MS_RDONLY))
@@ -64,7 +64,7 @@ static int hfs_sync_fs(struct super_block *sb, int wait)
 {
 	lock_super(sb);
 	hfs_mdb_commit(sb);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 	unlock_super(sb);
 
 	return 0;
@@ -81,7 +81,7 @@ static void hfs_put_super(struct super_block *sb)
 {
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		hfs_write_super(sb);
 	hfs_mdb_close(sb);
 	/* release the MDB's resources */
-- 
1.6.6.1

[PATCHv4 10/17] HFSPLUS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/hfsplus/bitmap.c |    4 ++--
 fs/hfsplus/dir.c    |    2 +-
 fs/hfsplus/inode.c  |    6 +++---
 fs/hfsplus/super.c  |   16 ++++++++--------
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/hfsplus/bitmap.c b/fs/hfsplus/bitmap.c
index ea30afc..345b0e3 100644
--- a/fs/hfsplus/bitmap.c
+++ b/fs/hfsplus/bitmap.c
@@ -151,7 +151,7 @@ done:
 	kunmap(page);
 	*max = offset + (curr - pptr) * 32 + i - start;
 	HFSPLUS_SB(sb).free_blocks -= *max;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	dprint(DBG_BITMAP, "-> %u,%u\n", start, *max);
 out:
 	mutex_unlock(&HFSPLUS_SB(sb).alloc_file->i_mutex);
@@ -225,7 +225,7 @@ out:
 	set_page_dirty(page);
 	kunmap(page);
 	HFSPLUS_SB(sb).free_blocks += len;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	mutex_unlock(&HFSPLUS_SB(sb).alloc_file->i_mutex);
 
 	return 0;
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 5f40236..71868b5 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -305,7 +305,7 @@ static int hfsplus_link(struct dentry *src_dentry, struct inode *dst_dir,
 	inode->i_ctime = CURRENT_TIME_SEC;
 	mark_inode_dirty(inode);
 	HFSPLUS_SB(sb).file_count++;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	return 0;
 }
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 1bcf597..76fe933 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -333,7 +333,7 @@ struct inode *hfsplus_new_inode(struct super_block *sb, int mode)
 		HFSPLUS_SB(sb).file_count++;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	return inode;
 }
@@ -344,7 +344,7 @@ void hfsplus_delete_inode(struct inode *inode)
 
 	if (S_ISDIR(inode->i_mode)) {
 		HFSPLUS_SB(sb).folder_count--;
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 		return;
 	}
 	HFSPLUS_SB(sb).file_count--;
@@ -357,7 +357,7 @@ void hfsplus_delete_inode(struct inode *inode)
 		inode->i_size = 0;
 		hfsplus_file_truncate(inode);
 	}
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 }
 
 void hfsplus_inode_read_fork(struct inode *inode, struct hfsplus_fork_raw *fork)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 74b473a..edac7a5 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -106,7 +106,7 @@ static int hfsplus_write_inode(struct inode *inode,
 	case HFSPLUS_EXT_CNID:
 		if (vhdr->ext_file.total_size != cpu_to_be64(inode->i_size)) {
 			HFSPLUS_SB(inode->i_sb).flags |= HFSPLUS_SB_WRITEBACKUP;
-			inode->i_sb->s_dirt = 1;
+			mark_sb_dirty(inode->i_sb);
 		}
 		hfsplus_inode_write_fork(inode, &vhdr->ext_file);
 		hfs_btree_write(HFSPLUS_SB(inode->i_sb).ext_tree);
@@ -114,7 +114,7 @@ static int hfsplus_write_inode(struct inode *inode,
 	case HFSPLUS_CAT_CNID:
 		if (vhdr->cat_file.total_size != cpu_to_be64(inode->i_size)) {
 			HFSPLUS_SB(inode->i_sb).flags |= HFSPLUS_SB_WRITEBACKUP;
-			inode->i_sb->s_dirt = 1;
+			mark_sb_dirty(inode->i_sb);
 		}
 		hfsplus_inode_write_fork(inode, &vhdr->cat_file);
 		hfs_btree_write(HFSPLUS_SB(inode->i_sb).cat_tree);
@@ -122,21 +122,21 @@ static int hfsplus_write_inode(struct inode *inode,
 	case HFSPLUS_ALLOC_CNID:
 		if (vhdr->alloc_file.total_size != cpu_to_be64(inode->i_size)) {
 			HFSPLUS_SB(inode->i_sb).flags |= HFSPLUS_SB_WRITEBACKUP;
-			inode->i_sb->s_dirt = 1;
+			mark_sb_dirty(inode->i_sb);
 		}
 		hfsplus_inode_write_fork(inode, &vhdr->alloc_file);
 		break;
 	case HFSPLUS_START_CNID:
 		if (vhdr->start_file.total_size != cpu_to_be64(inode->i_size)) {
 			HFSPLUS_SB(inode->i_sb).flags |= HFSPLUS_SB_WRITEBACKUP;
-			inode->i_sb->s_dirt = 1;
+			mark_sb_dirty(inode->i_sb);
 		}
 		hfsplus_inode_write_fork(inode, &vhdr->start_file);
 		break;
 	case HFSPLUS_ATTR_CNID:
 		if (vhdr->attr_file.total_size != cpu_to_be64(inode->i_size)) {
 			HFSPLUS_SB(inode->i_sb).flags |= HFSPLUS_SB_WRITEBACKUP;
-			inode->i_sb->s_dirt = 1;
+			mark_sb_dirty(inode->i_sb);
 		}
 		hfsplus_inode_write_fork(inode, &vhdr->attr_file);
 		hfs_btree_write(HFSPLUS_SB(inode->i_sb).attr_tree);
@@ -161,7 +161,7 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
 	dprint(DBG_SUPER, "hfsplus_write_super\n");
 
 	lock_super(sb);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 
 	vhdr->free_blocks = cpu_to_be32(HFSPLUS_SB(sb).free_blocks);
 	vhdr->next_alloc = cpu_to_be32(HFSPLUS_SB(sb).next_alloc);
@@ -202,7 +202,7 @@ static void hfsplus_write_super(struct super_block *sb)
 	if (!(sb->s_flags & MS_RDONLY))
 		hfsplus_sync_fs(sb, 1);
 	else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 }
 
 static void hfsplus_put_super(struct super_block *sb)
@@ -213,7 +213,7 @@ static void hfsplus_put_super(struct super_block *sb)
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		hfsplus_write_super(sb);
 	if (!(sb->s_flags & MS_RDONLY) && HFSPLUS_SB(sb).s_vhdr) {
 		struct hfsplus_vh *vhdr = HFSPLUS_SB(sb).s_vhdr;
-- 
1.6.6.1

[PATCHv4 11/17] JFFS2: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 fs/jffs2/os-linux.h |    2 +-
 fs/jffs2/super.c    |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h
index 035a767..71459ce 100644
--- a/fs/jffs2/os-linux.h
+++ b/fs/jffs2/os-linux.h
@@ -142,7 +142,7 @@ void jffs2_nor_wbuf_flash_cleanup(struct jffs2_sb_info *c);
 
 static inline void jffs2_dirty_trigger(struct jffs2_sb_info *c)
 {
-	OFNI_BS_2SFFJ(c)->s_dirt = 1;
+	mark_sb_dirty(OFNI_BS_2SFFJ(c));
 }
 
 /* background.c */
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 511e2d6..b7f4b6b 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -59,7 +59,7 @@ static void jffs2_write_super(struct super_block *sb)
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
 
 	lock_super(sb);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 
 	if (!(sb->s_flags & MS_RDONLY)) {
 		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
@@ -194,7 +194,7 @@ static void jffs2_put_super (struct super_block *sb)
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		jffs2_write_super(sb);
 
 	mutex_lock(&c->alloc_sem);
-- 
1.6.6.1

[PATCHv4 12/17] reiserfs: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: reiserfs-devel@vger.kernel.org
---
 fs/reiserfs/journal.c |    6 +++---
 fs/reiserfs/resize.c  |    2 +-
 fs/reiserfs/super.c   |   10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 19fbc81..4016d0a 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -3337,7 +3337,7 @@ int journal_mark_dirty(struct reiserfs_transaction_handle *th,
 			       th->t_trans_id, journal->j_trans_id);
 	}
 
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	prepared = test_clear_buffer_journal_prepared(bh);
 	clear_buffer_journal_restore_dirty(bh);
@@ -3632,7 +3632,7 @@ int reiserfs_flush_old_commits(struct super_block *sb)
 			do_journal_end(&th, sb, 1, COMMIT_NOW | WAIT);
 		}
 	}
-	return sb->s_dirt;
+	return is_sb_dirty(sb);
 }
 
 /*
@@ -4062,7 +4062,7 @@ static int do_journal_end(struct reiserfs_transaction_handle *th,
 	 ** it tells us if we should continue with the journal_end, or just return
 	 */
 	if (!check_journal_end(th, sb, nblocks, flags)) {
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 		wake_queued_writers(sb);
 		reiserfs_async_progress_wait(sb);
 		goto out;
diff --git a/fs/reiserfs/resize.c b/fs/reiserfs/resize.c
index b3a94d2..588d55a 100644
--- a/fs/reiserfs/resize.c
+++ b/fs/reiserfs/resize.c
@@ -203,7 +203,7 @@ int reiserfs_resize(struct super_block *s, unsigned long block_count_new)
 					  (bmap_nr_new - bmap_nr)));
 	PUT_SB_BLOCK_COUNT(s, block_count_new);
 	PUT_SB_BMAP_NR(s, bmap_would_wrap(bmap_nr_new) ? : bmap_nr_new);
-	s->s_dirt = 1;
+	mark_sb_dirty(s);
 
 	journal_mark_dirty(&th, s, SB_BUFFER_WITH_SB(s));
 
diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 59125fb..93eb3fc 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -71,8 +71,8 @@ static int reiserfs_sync_fs(struct super_block *s, int wait)
 	if (!journal_begin(&th, s, 1))
 		if (!journal_end_sync(&th, s, 1))
 			reiserfs_flush_old_commits(s);
-	s->s_dirt = 0;	/* Even if it's not true.
-			 * We'll loop forever in sync_supers otherwise */
+	mark_sb_clean(s); /* Even if it's not true.
+			   * We'll loop forever in sync_supers otherwise */
 	reiserfs_write_unlock(s);
 	return 0;
 }
@@ -98,7 +98,7 @@ static int reiserfs_freeze(struct super_block *s)
 			journal_end_sync(&th, s, 1);
 		}
 	}
-	s->s_dirt = 0;
+	mark_sb_clean(s);
 	reiserfs_write_unlock(s);
 	return 0;
 }
@@ -468,7 +468,7 @@ static void reiserfs_put_super(struct super_block *s)
 
 	reiserfs_write_lock(s);
 
-	if (s->s_dirt)
+	if (is_sb_dirty(s))
 		reiserfs_write_super(s);
 
 	/* change file system state to current state if it was mounted with read-write permissions */
@@ -1292,7 +1292,7 @@ static int reiserfs_remount(struct super_block *s, int *mount_flags, char *arg)
 	err = journal_end(&th, s, 10);
 	if (err)
 		goto out_err;
-	s->s_dirt = 0;
+	mark_sb_clean(s);
 
 	if (!(*mount_flags & MS_RDONLY)) {
 		finish_unfinished(s);
-- 
1.6.6.1

[PATCHv4 13/17] SYSV: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/sysv/inode.c |    6 +++---
 fs/sysv/super.c |    2 +-
 fs/sysv/sysv.h  |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index 4573734..03a6387 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -61,7 +61,7 @@ static void sysv_write_super(struct super_block *sb)
 	if (!(sb->s_flags & MS_RDONLY))
 		sysv_sync_fs(sb, 1);
 	else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 }
 
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
@@ -71,7 +71,7 @@ static int sysv_remount(struct super_block *sb, int *flags, char *data)
 	if (sbi->s_forced_ro)
 		*flags |= MS_RDONLY;
 	if (!(*flags & MS_RDONLY))
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 	unlock_super(sb);
 	return 0;
 }
@@ -80,7 +80,7 @@ static void sysv_put_super(struct super_block *sb)
 {
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		sysv_write_super(sb);
 
 	if (!(sb->s_flags & MS_RDONLY)) {
diff --git a/fs/sysv/super.c b/fs/sysv/super.c
index 5a903da..f0d3679 100644
--- a/fs/sysv/super.c
+++ b/fs/sysv/super.c
@@ -347,7 +347,7 @@ static int complete_read_super(struct super_block *sb, int silent, int size)
 		sb->s_flags |= MS_RDONLY;
 	if (sbi->s_truncate)
 		sb->s_root->d_op = &sysv_dentry_operations;
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	return 1;
 }
 
diff --git a/fs/sysv/sysv.h b/fs/sysv/sysv.h
index 94cb9b4..f0441c2 100644
--- a/fs/sysv/sysv.h
+++ b/fs/sysv/sysv.h
@@ -118,7 +118,7 @@ static inline void dirty_sb(struct super_block *sb)
 	mark_buffer_dirty(sbi->s_bh1);
 	if (sbi->s_bh1 != sbi->s_bh2)
 		mark_buffer_dirty(sbi->s_bh2);
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 }
 
 
-- 
1.6.6.1

[PATCHv4 14/17] UDF: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c   |    6 +++---
 fs/udf/udfdecl.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 1e4543c..998c911 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1940,7 +1940,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 	sb->s_op = &udf_sb_ops;
 	sb->s_export_op = &udf_export_ops;
 	sb->dq_op = NULL;
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 	sb->s_magic = UDF_SUPER_MAGIC;
 	sb->s_time_gran = 1000;
 
@@ -2067,7 +2067,7 @@ static void udf_error(struct super_block *sb, const char *function,
 
 	if (!(sb->s_flags & MS_RDONLY)) {
 		/* mark sb error */
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 	}
 	va_start(args, fmt);
 	vsnprintf(error_buf, sizeof(error_buf), fmt, args);
@@ -2127,7 +2127,7 @@ static int udf_sync_fs(struct super_block *sb, int wait)
 		 * the buffer for IO
 		 */
 		mark_buffer_dirty(sbi->s_lvid_bh);
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 		sbi->s_lvid_dirty = 0;
 	}
 	mutex_unlock(&sbi->s_alloc_mutex);
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 9079ff7..9337e3f 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -120,7 +120,7 @@ static inline void udf_updated_lvid(struct super_block *sb)
 	WARN_ON_ONCE(((struct logicalVolIntegrityDesc *)
 		     bh->b_data)->integrityType !=
 		     cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN));
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	UDF_SB(sb)->s_lvid_dirty = 1;
 }
 
-- 
1.6.6.1

[PATCHv4 15/17] UFS: do not manipulate s_dirt directly

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... use new VFS helpers instead.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Evgeniy Dushistov <dushistov@mail.ru>
---
 fs/ufs/balloc.c |    8 ++++----
 fs/ufs/ialloc.c |    4 ++--
 fs/ufs/super.c  |   12 ++++++------
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/ufs/balloc.c b/fs/ufs/balloc.c
index 5cfa4d8..43e5c4f 100644
--- a/fs/ufs/balloc.c
+++ b/fs/ufs/balloc.c
@@ -122,7 +122,7 @@ void ufs_free_fragments(struct inode *inode, u64 fragment, unsigned count)
 		ubh_ll_rw_block(SWRITE, UCPI_UBH(ucpi));
 		ubh_wait_on_buffer (UCPI_UBH(ucpi));
 	}
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	
 	unlock_super (sb);
 	UFSD("EXIT\n");
@@ -223,7 +223,7 @@ do_more:
 		goto do_more;
 	}
 
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	unlock_super (sb);
 	UFSD("EXIT\n");
 	return;
@@ -573,7 +573,7 @@ static u64 ufs_add_fragments(struct inode *inode, u64 fragment,
 		ubh_ll_rw_block(SWRITE, UCPI_UBH(ucpi));
 		ubh_wait_on_buffer (UCPI_UBH(ucpi));
 	}
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	UFSD("EXIT, fragment %llu\n", (unsigned long long)fragment);
 	
@@ -702,7 +702,7 @@ succed:
 		ubh_ll_rw_block(SWRITE, UCPI_UBH(ucpi));
 		ubh_wait_on_buffer (UCPI_UBH(ucpi));
 	}
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	result += cgno * uspi->s_fpg;
 	UFSD("EXIT3, result %llu\n", (unsigned long long)result);
diff --git a/fs/ufs/ialloc.c b/fs/ufs/ialloc.c
index 3a959d5..765a21d 100644
--- a/fs/ufs/ialloc.c
+++ b/fs/ufs/ialloc.c
@@ -124,7 +124,7 @@ void ufs_free_inode (struct inode * inode)
 		ubh_wait_on_buffer (UCPI_UBH(ucpi));
 	}
 	
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 	unlock_super (sb);
 	UFSD("EXIT\n");
 }
@@ -300,7 +300,7 @@ cg_found:
 		ubh_ll_rw_block(SWRITE, UCPI_UBH(ucpi));
 		ubh_wait_on_buffer (UCPI_UBH(ucpi));
 	}
-	sb->s_dirt = 1;
+	mark_sb_dirty(sb);
 
 	inode->i_ino = cg * uspi->s_ipg + bit;
 	inode_init_owner(inode, dir, mode);
diff --git a/fs/ufs/super.c b/fs/ufs/super.c
index 14743d9..a0317d8 100644
--- a/fs/ufs/super.c
+++ b/fs/ufs/super.c
@@ -288,7 +288,7 @@ void ufs_error (struct super_block * sb, const char * function,
 	if (!(sb->s_flags & MS_RDONLY)) {
 		usb1->fs_clean = UFS_FSBAD;
 		ubh_mark_buffer_dirty(USPI_UBH(uspi));
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 		sb->s_flags |= MS_RDONLY;
 	}
 	va_start (args, fmt);
@@ -321,7 +321,7 @@ void ufs_panic (struct super_block * sb, const char * function,
 	if (!(sb->s_flags & MS_RDONLY)) {
 		usb1->fs_clean = UFS_FSBAD;
 		ubh_mark_buffer_dirty(USPI_UBH(uspi));
-		sb->s_dirt = 1;
+		mark_sb_dirty(sb);
 	}
 	va_start (args, fmt);
 	vsnprintf (error_buf, sizeof(error_buf), fmt, args);
@@ -1204,7 +1204,7 @@ static int ufs_sync_fs(struct super_block *sb, int wait)
 		ufs_set_fs_state(sb, usb1, usb3,
 				UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time));
 	ufs_put_cstotal(sb);
-	sb->s_dirt = 0;
+	mark_sb_clean(sb);
 
 	UFSD("EXIT\n");
 	unlock_kernel();
@@ -1218,7 +1218,7 @@ static void ufs_write_super(struct super_block *sb)
 	if (!(sb->s_flags & MS_RDONLY))
 		ufs_sync_fs(sb, 1);
 	else
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 }
 
 static void ufs_put_super(struct super_block *sb)
@@ -1227,7 +1227,7 @@ static void ufs_put_super(struct super_block *sb)
 		
 	UFSD("ENTER\n");
 
-	if (sb->s_dirt)
+	if (is_sb_dirty(sb))
 		ufs_write_super(sb);
 
 	if (!(sb->s_flags & MS_RDONLY))
@@ -1297,7 +1297,7 @@ static int ufs_remount (struct super_block *sb, int *mount_flags, char *data)
 			ufs_set_fs_state(sb, usb1, usb3,
 				UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time));
 		ubh_mark_buffer_dirty (USPI_UBH(uspi));
-		sb->s_dirt = 0;
+		mark_sb_clean(sb);
 		sb->s_flags |= MS_RDONLY;
 	} else {
 	/*
-- 
1.6.6.1

[PATCHv4 16/17] VFS: rename s_dirt to s_dirty

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

... in order to make sure no one accesses the "s_dirt" flag
direclty - this should help to identify build errors earlier.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 include/linux/fs.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21fe2b3..c2ddeee 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1318,7 +1318,7 @@ extern spinlock_t sb_lock;
 struct super_block {
 	struct list_head	s_list;		/* Keep this first */
 	dev_t			s_dev;		/* search index; _not_ kdev_t */
-	unsigned char		s_dirt;
+	unsigned char		s_dirty;
 	unsigned char		s_blocksize_bits;
 	unsigned long		s_blocksize;
 	loff_t			s_maxbytes;	/* Max file size */
@@ -1788,15 +1788,15 @@ extern void simple_set_mnt(struct vfsmount *mnt, struct super_block *sb);
  */
 static inline void mark_sb_dirty(struct super_block *sb)
 {
-	sb->s_dirt = 1;
+	sb->s_dirty = 1;
 }
 static inline void mark_sb_clean(struct super_block *sb)
 {
-	sb->s_dirt = 0;
+	sb->s_dirty = 0;
 }
 static inline int is_sb_dirty(struct super_block *sb)
 {
-	return sb->s_dirt;
+	return sb->s_dirty;
 }
 
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
-- 
1.6.6.1

[PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

The 'sync_supers' thread wakes up every 5 seconds (by default) and
writes back all super blocks. It keeps waking up even if there
are no dirty super-blocks. For many file-systems the superblock
becomes dirty very rarely, if ever, so 'sync_supers' does not do
anything most of the time.

This patch improves 'sync_supers' and makes sleep if all superblocks
are clean and there is nothing to do. This helps saving the power.
This optimization is important for small battery-powered devices.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 include/linux/fs.h |    5 +----
 mm/backing-dev.c   |   36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index c2ddeee..2d2560b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1786,10 +1786,7 @@ extern void simple_set_mnt(struct vfsmount *mnt, struct super_block *sb);
  * Note, VFS does not provide any serialization for the super block clean/dirty
  * state changes, file-systems should take care of this.
  */
-static inline void mark_sb_dirty(struct super_block *sb)
-{
-	sb->s_dirty = 1;
-}
+void mark_sb_dirty(struct super_block *sb);
 static inline void mark_sb_clean(struct super_block *sb)
 {
 	sb->s_dirty = 0;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 660a87a..14f3eb7 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -45,6 +45,8 @@ LIST_HEAD(bdi_pending_list);
 
 static struct task_struct *sync_supers_tsk;
 static struct timer_list sync_supers_timer;
+static int supers_timer_armed;
+static DEFINE_SPINLOCK(supers_timer_lock);
 
 static int bdi_sync_supers(void *);
 static void sync_supers_timer_fn(unsigned long);
@@ -355,6 +357,11 @@ static void bdi_flush_io(struct backing_dev_info *bdi)
  * or we risk deadlocking on ->s_umount. The longer term solution would be
  * to implement sync_supers_bdi() or similar and simply do it from the
  * bdi writeback tasks individually.
+ *
+ * Historically this thread woke up periodically, regardless of whether
+ * there was any dirty superblock. However, nowadays it is optimized to
+ * wake up only when there is something to synchronize - this helps to save
+ * power.
  */
 static int bdi_sync_supers(void *unused)
 {
@@ -364,10 +371,24 @@ static int bdi_sync_supers(void *unused)
 		set_current_state(TASK_INTERRUPTIBLE);
 		schedule();
 
+		spin_lock(&supers_timer_lock);
+		/* Indicate that 'sync_supers' is in progress */
+		supers_timer_armed = -1;
+		spin_unlock(&supers_timer_lock);
+
 		/*
 		 * Do this periodically, like kupdated() did before.
 		 */
 		sync_supers();
+
+		spin_lock(&supers_timer_lock);
+		if (supers_timer_armed == 1)
+			/* A super block was marked as dirty meanwhile */
+			bdi_arm_supers_timer();
+		else
+			/* No more dirty superblocks - we've synced'em all */
+			supers_timer_armed = 0;
+		spin_unlock(&supers_timer_lock);
 	}
 
 	return 0;
@@ -387,9 +408,22 @@ void bdi_arm_supers_timer(void)
 static void sync_supers_timer_fn(unsigned long unused)
 {
 	wake_up_process(sync_supers_tsk);
-	bdi_arm_supers_timer();
 }
 
+void mark_sb_dirty(struct super_block *sb)
+{
+	sb->s_dirty = 1;
+
+	spin_lock(&supers_timer_lock);
+	if (!supers_timer_armed) {
+		bdi_arm_supers_timer();
+		supers_timer_armed = 1;
+	} else if (supers_timer_armed == -1)
+		supers_timer_armed = 1;
+	spin_unlock(&supers_timer_lock);
+}
+EXPORT_SYMBOL(mark_sb_dirty);
+
 static int bdi_forker_task(void *ptr)
 {
 	struct bdi_writeback *me = ptr;
-- 
1.6.6.1

Re: [PATCHv4 14/17] UDF: do not manipulate s_dirt directly

[Jan Kara]

On Tue 25-05-10 16:49:09, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> ... use new VFS helpers instead.
  Looks. OK. You can add
Acked-by: Jan Kara <jack@suse.cz>

							Honza

> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/super.c   |    6 +++---
>  fs/udf/udfdecl.h |    2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 1e4543c..998c911 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -1940,7 +1940,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
>  	sb->s_op = &udf_sb_ops;
>  	sb->s_export_op = &udf_export_ops;
>  	sb->dq_op = NULL;
> -	sb->s_dirt = 0;
> +	mark_sb_clean(sb);
>  	sb->s_magic = UDF_SUPER_MAGIC;
>  	sb->s_time_gran = 1000;
>  
> @@ -2067,7 +2067,7 @@ static void udf_error(struct super_block *sb, const char *function,
>  
>  	if (!(sb->s_flags & MS_RDONLY)) {
>  		/* mark sb error */
> -		sb->s_dirt = 1;
> +		mark_sb_dirty(sb);
>  	}
>  	va_start(args, fmt);
>  	vsnprintf(error_buf, sizeof(error_buf), fmt, args);
> @@ -2127,7 +2127,7 @@ static int udf_sync_fs(struct super_block *sb, int wait)
>  		 * the buffer for IO
>  		 */
>  		mark_buffer_dirty(sbi->s_lvid_bh);
> -		sb->s_dirt = 0;
> +		mark_sb_clean(sb);
>  		sbi->s_lvid_dirty = 0;
>  	}
>  	mutex_unlock(&sbi->s_alloc_mutex);
> diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
> index 9079ff7..9337e3f 100644
> --- a/fs/udf/udfdecl.h
> +++ b/fs/udf/udfdecl.h
> @@ -120,7 +120,7 @@ static inline void udf_updated_lvid(struct super_block *sb)
>  	WARN_ON_ONCE(((struct logicalVolIntegrityDesc *)
>  		     bh->b_data)->integrityType !=
>  		     cpu_to_le32(LVID_INTEGRITY_TYPE_OPEN));
> -	sb->s_dirt = 1;
> +	mark_sb_dirty(sb);
>  	UDF_SB(sb)->s_lvid_dirty = 1;
>  }
>  
> -- 
> 1.6.6.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCHv4 05/17] EXOFS: do not manipulate s_dirt directly

[Boaz Harrosh]

On 05/25/2010 04:49 PM, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> ... use new VFS helpers instead.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Boaz Harrosh <bharrosh@panasas.com>

Ack-by: Boaz Harrosh <bharrosh@panasas.com>

> ---
>  fs/exofs/file.c  |    2 +-
>  fs/exofs/inode.c |    2 +-
>  fs/exofs/super.c |    6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/exofs/file.c b/fs/exofs/file.c
> index 839b9dc..be38834 100644
> --- a/fs/exofs/file.c
> +++ b/fs/exofs/file.c
> @@ -58,7 +58,7 @@ static int exofs_file_fsync(struct file *filp, struct dentry *dentry,
>  	/* This is a good place to write the sb */
>  	/* TODO: Sechedule an sb-sync on create */
>  	sb = inode->i_sb;
> -	if (sb->s_dirt)
> +	if (is_sb_dirty(sb))
>  		exofs_sync_fs(sb, 1);
>  
>  	return ret;
> diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
> index d7c6afa..0195dcc 100644
> --- a/fs/exofs/inode.c
> +++ b/fs/exofs/inode.c
> @@ -1122,7 +1122,7 @@ struct inode *exofs_new_inode(struct inode *dir, int mode)
>  
>  	sbi = sb->s_fs_info;
>  
> -	sb->s_dirt = 1;
> +	mark_sb_dirty(sb);
>  	inode_init_owner(inode, dir, mode);
>  	inode->i_ino = sbi->s_nextid++;
>  	inode->i_blkbits = EXOFS_BLKSHIFT;
> diff --git a/fs/exofs/super.c b/fs/exofs/super.c
> index 03149b9..91a32ea 100644
> --- a/fs/exofs/super.c
> +++ b/fs/exofs/super.c
> @@ -237,7 +237,7 @@ int exofs_sync_fs(struct super_block *sb, int wait)
>  		EXOFS_ERR("%s: exofs_sbi_write failed.\n", __func__);
>  		goto out;
>  	}
> -	sb->s_dirt = 0;
> +	mark_sb_clean(sb);
>  
>  out:
>  	EXOFS_DBGMSG("s_nextid=0x%llx ret=%d\n", _LLU(sbi->s_nextid), ret);
> @@ -251,7 +251,7 @@ static void exofs_write_super(struct super_block *sb)
>  	if (!(sb->s_flags & MS_RDONLY))
>  		exofs_sync_fs(sb, 1);
>  	else
> -		sb->s_dirt = 0;
> +		mark_sb_clean(sb);
>  }
>  
>  static void _exofs_print_device(const char *msg, const char *dev_path,
> @@ -286,7 +286,7 @@ static void exofs_put_super(struct super_block *sb)
>  	int num_pend;
>  	struct exofs_sb_info *sbi = sb->s_fs_info;
>  
> -	if (sb->s_dirt)
> +	if (is_sb_dirty(sb))
>  		exofs_write_super(sb);
>  
>  	/* make sure there are no pending commands */

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Al Viro]

On Tue, May 25, 2010 at 04:49:12PM +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> The 'sync_supers' thread wakes up every 5 seconds (by default) and
> writes back all super blocks. It keeps waking up even if there
> are no dirty super-blocks. For many file-systems the superblock
> becomes dirty very rarely, if ever, so 'sync_supers' does not do
> anything most of the time.
> 
> This patch improves 'sync_supers' and makes sleep if all superblocks
> are clean and there is nothing to do. This helps saving the power.
> This optimization is important for small battery-powered devices.

> +void mark_sb_dirty(struct super_block *sb)
> +{
> +	sb->s_dirty = 1;
> +
> +	spin_lock(&supers_timer_lock);
> +	if (!supers_timer_armed) {
> +		bdi_arm_supers_timer();
> +		supers_timer_armed = 1;
> +	} else if (supers_timer_armed == -1)
> +		supers_timer_armed = 1;
> +	spin_unlock(&supers_timer_lock);
> +}
> +EXPORT_SYMBOL(mark_sb_dirty);

Ouch...   That turns a previously trivial operation into something
much heavier; moreover, I'd rather see serious review of s_dirt
uses.

Note, e.g., that in your series you've touched udf; it can set s_dirt
until the cows come home, but without ->write_super() it'll be ignored
by everything in VFS and fs/udf itself never looks at the damn thing.

A look around it shows fs/sysv, where we never clean the damn flag anymore
for r/w mounts.  Yes, really (got broken a year ago, nobody noticed).

Or, e.g., BFS - there we have ->write_super() mark the buffer_head that
contains on-disk sb dirty, and the only place that sets ->s_dirt is doing
that immediately after having marked the same bh dirty itself.  Interesting
place, at that - bfs_fill_super() at r/w mount time...  Note that ->sync_fs()
there does *not* wait for anything, which is not the right thing to do.

IOW, this thing is a good topic for code review; I suspect that quite a few
users might be gone as the result.

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Nick Piggin]

On Thu, May 27, 2010 at 07:50:41AM +0100, Al Viro wrote:
> On Tue, May 25, 2010 at 04:49:12PM +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > 
> > The 'sync_supers' thread wakes up every 5 seconds (by default) and
> > writes back all super blocks. It keeps waking up even if there
> > are no dirty super-blocks. For many file-systems the superblock
> > becomes dirty very rarely, if ever, so 'sync_supers' does not do
> > anything most of the time.
> > 
> > This patch improves 'sync_supers' and makes sleep if all superblocks
> > are clean and there is nothing to do. This helps saving the power.
> > This optimization is important for small battery-powered devices.
> 
> > +void mark_sb_dirty(struct super_block *sb)
> > +{
> > +	sb->s_dirty = 1;
> > +
> > +	spin_lock(&supers_timer_lock);
> > +	if (!supers_timer_armed) {
> > +		bdi_arm_supers_timer();
> > +		supers_timer_armed = 1;
> > +	} else if (supers_timer_armed == -1)
> > +		supers_timer_armed = 1;
> > +	spin_unlock(&supers_timer_lock);
> > +}
> > +EXPORT_SYMBOL(mark_sb_dirty);
> 
> Ouch...   That turns a previously trivial operation into something
> much heavier; moreover, I'd rather see serious review of s_dirt
> uses.

Yeah, we definitely don't want to add global cacheline writes in the
common case. Also I don't know why you do the strange -1 value. I
couldn't seem to find where you defined bdi_arm_supers_timer();

But why doesn't this work?

  sb->s_dirty = 1;
  smp_mb(); /* corresponding MB is in test_and_clear_bit */
  if (unlikely(!supers_timer_armed)) {
    if (!test_and_set_bit(0, &supers_timer_armed))
        bdi_arm_supers_timer();
  }
    
  vs

  supers_timer_armed = 0;
again:
  sync_supers();
  if (test_and_clear_bit(0, &supers_timer_armed))
    goto again;
  

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Al Viro]

On Thu, May 27, 2010 at 05:22:40PM +1000, Nick Piggin wrote:
> Yeah, we definitely don't want to add global cacheline writes in the
> common case. Also I don't know why you do the strange -1 value. I
> couldn't seem to find where you defined bdi_arm_supers_timer();

bdi_arm_supers_timer() is in the mainline; basically, it's "schedule
timer that'll wake sync_supers_tsk in $INTERVAL from now".

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Thu, 2010-05-27 at 07:50 +0100, Al Viro wrote:
> On Tue, May 25, 2010 at 04:49:12PM +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > +void mark_sb_dirty(struct super_block *sb)
> > +{
> > +	sb->s_dirty = 1;
> > +
> > +	spin_lock(&supers_timer_lock);
> > +	if (!supers_timer_armed) {
> > +		bdi_arm_supers_timer();
> > +		supers_timer_armed = 1;
> > +	} else if (supers_timer_armed == -1)
> > +		supers_timer_armed = 1;
> > +	spin_unlock(&supers_timer_lock);
> > +}
> > +EXPORT_SYMBOL(mark_sb_dirty);
> 
> Ouch...   That turns a previously trivial operation into something
> much heavier; moreover, I'd rather see serious review of s_dirt
> uses.

OK, I'll try to do something lighter with atomic variables or something
like Nick posted - need to think about this. And I'll try to review
s_dirty usage as much as my time and knowledge allow. Thanks.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

Nick, thanks for serialization suggestion.

On Thu, 2010-05-27 at 17:22 +1000, Nick Piggin wrote:
> Yeah, we definitely don't want to add global cacheline writes in the
> common case. Also I don't know why you do the strange -1 value. I
> couldn't seem to find where you defined bdi_arm_supers_timer();

It is in mm/backing-dev.c:376 in today's Linus' tree. The -1 is used to
indicate that 'sync_supers()' is in progress and avoid arming timer in
that case. But yes, this is not really needed.

> But why doesn't this work?
> 
>   sb->s_dirty = 1;
>   smp_mb(); /* corresponding MB is in test_and_clear_bit */

AFAIU, test_and_clear_bit assumes 2 barriers - before the test and after
the clear. Then I do not really understand why this smp_mb is needed.

>   if (unlikely(!supers_timer_armed)) {
>     if (!test_and_set_bit(0, &supers_timer_armed))
>         bdi_arm_supers_timer();
>   }
>     
>   vs
> 
>   supers_timer_armed = 0;
> again:
>   sync_supers();
>   if (test_and_clear_bit(0, &supers_timer_armed))
>     goto again;

AFAIU, the following should be fine, no?:


    if (unlikely(!supers_timer_armed))
        if (!test_and_set_bit(0, &supers_timer_armed))
            bdi_arm_supers_timer();

vs

again:
    sync_supers();
    if (test_and_clear_bit(0, &supers_timer_armed))
        goto again;

I assume that it is harmless to run 'bdi_arm_supers_timer()'
concurrently;

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Nick Piggin]

On Thu, May 27, 2010 at 01:51:09PM +0300, Artem Bityutskiy wrote:
> Nick, thanks for serialization suggestion.
> 
> On Thu, 2010-05-27 at 17:22 +1000, Nick Piggin wrote:
> > Yeah, we definitely don't want to add global cacheline writes in the
> > common case. Also I don't know why you do the strange -1 value. I
> > couldn't seem to find where you defined bdi_arm_supers_timer();
> 
> It is in mm/backing-dev.c:376 in today's Linus' tree. The -1 is used to

Yep I should have grepped. /hangs head


> indicate that 'sync_supers()' is in progress and avoid arming timer in
> that case. But yes, this is not really needed.

OK please remove it.

 
> > But why doesn't this work?
> > 
> >   sb->s_dirty = 1;
> >   smp_mb(); /* corresponding MB is in test_and_clear_bit */
> 
> AFAIU, test_and_clear_bit assumes 2 barriers - before the test and after
> the clear. Then I do not really understand why this smp_mb is needed.

You almost always need barriers executed on all sides of the
synchronisation protocol. Actually we need another, I confused
myself with the test_and_clear at the end.

1. sb->s_dirty = 1; /* store */
2. if (!supers_timer_armed) /* load */
3.   supers_timer_armed = 1; /* store */

and

A. supers_timer_armed = 0; /* store */
B. if (sb->s_dirty) /* load */
C.   sb->s_dirty = 0 /* store */

If these two sequences are executed, it must result in
sb->s_dirty == 1 iff supers_timer_armed

* If 2 is executed before 1 is visible, then 2 may miss A before B sees 1.
* If B is executed before A is visible, then B may miss 1 before 2 sees A.

So we need smp_mb() between 1/2 and A/B (I missed the 2nd one).

Now we still have a problem. After sync task rechecks
supers_timer_armed, the supers timer might execute before we mark
ourself as sleeping, and so we have another lost wakeup. It needs
to be checked after set_current_state.

Let's try this again. I much prefer to name the variable something
that indicates whether there is more work to be done, or whether we
can sleep.

How about something like this?
--

Index: linux-2.6/mm/backing-dev.c
===================================================================
--- linux-2.6.orig/mm/backing-dev.c
+++ linux-2.6/mm/backing-dev.c
@@ -45,6 +45,7 @@ LIST_HEAD(bdi_pending_list);
 
 static struct task_struct *sync_supers_tsk;
 static struct timer_list sync_supers_timer;
+static unsigned long supers_dirty __read_mostly;
 
 static int bdi_sync_supers(void *);
 static void sync_supers_timer_fn(unsigned long);
@@ -251,7 +252,6 @@ static int __init default_bdi_init(void)
 
 	init_timer(&sync_supers_timer);
 	setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0);
-	bdi_arm_supers_timer();
 
 	err = bdi_init(&default_backing_dev_info);
 	if (!err)
@@ -362,17 +362,28 @@ static int bdi_sync_supers(void *unused)
 
 	while (!kthread_should_stop()) {
 		set_current_state(TASK_INTERRUPTIBLE);
-		schedule();
+		if (!supers_dirty)
+			schedule();
+		else
+			__set_current_state(TASK_RUNNING);
 
+		supers_dirty = 0;
 		/*
-		 * Do this periodically, like kupdated() did before.
+		 * supers_dirty store must be visible to mark_sb_dirty (below)
+		 * before sync_supers runs (which loads sb->s_dirty).
 		 */
+		smp_mb();
 		sync_supers();
 	}
 
 	return 0;
 }
 
+static void sync_supers_timer_fn(unsigned long unused)
+{
+	wake_up_process(sync_supers_tsk);
+}
+
 void bdi_arm_supers_timer(void)
 {
 	unsigned long next;
@@ -384,9 +395,17 @@ void bdi_arm_supers_timer(void)
 	mod_timer(&sync_supers_timer, round_jiffies_up(next));
 }
 
-static void sync_supers_timer_fn(unsigned long unused)
+void mark_sb_dirty(struct super_block *sb)
 {
-	wake_up_process(sync_supers_tsk);
+	sb->s_dirty = 1;
+	/*
+	 * sb->s_dirty store must be visible to sync_supers (above) before we
+	 * load supers_dirty in case we need to re-arm the timer.
+	 */
+	smp_mb();
+	if (likely(supers_dirty))
+		return;
+	supers_dirty = 1;
 	bdi_arm_supers_timer();
 }
 

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Thu, 2010-05-27 at 22:07 +1000, Nick Piggin wrote:
> 1. sb->s_dirty = 1; /* store */
> 2. if (!supers_timer_armed) /* load */
> 3.   supers_timer_armed = 1; /* store */
> 
> and
> 
> A. supers_timer_armed = 0; /* store */
> B. if (sb->s_dirty) /* load */
> C.   sb->s_dirty = 0 /* store */
> 
> If these two sequences are executed, it must result in
> sb->s_dirty == 1 iff supers_timer_armed
> 
> * If 2 is executed before 1 is visible, then 2 may miss A before B sees 1.
> * If B is executed before A is visible, then B may miss 1 before 2 sees A.
> 
> So we need smp_mb() between 1/2 and A/B (I missed the 2nd one).

Yes, thanks for elaboration.

> How about something like this?

It looks good, many thanks! But I have few small notes.

> Index: linux-2.6/mm/backing-dev.c
> ===================================================================
> --- linux-2.6.orig/mm/backing-dev.c
> +++ linux-2.6/mm/backing-dev.c
> @@ -45,6 +45,7 @@ LIST_HEAD(bdi_pending_list);
>  
>  static struct task_struct *sync_supers_tsk;
>  static struct timer_list sync_supers_timer;
> +static unsigned long supers_dirty __read_mostly;
>  
>  static int bdi_sync_supers(void *);
>  static void sync_supers_timer_fn(unsigned long);
> @@ -251,7 +252,6 @@ static int __init default_bdi_init(void)
>  
>  	init_timer(&sync_supers_timer);
>  	setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0);
> -	bdi_arm_supers_timer();
>  
>  	err = bdi_init(&default_backing_dev_info);
>  	if (!err)
> @@ -362,17 +362,28 @@ static int bdi_sync_supers(void *unused)
>  
>  	while (!kthread_should_stop()) {
>  		set_current_state(TASK_INTERRUPTIBLE);
> -		schedule();
> +		if (!supers_dirty)
> +			schedule();
> +		else
> +			__set_current_state(TASK_RUNNING);

I think this will change the behavior of 'sync_supers()' too much. ATM,
it makes only one SB pass, then sleeps, then another one, then sleeps.
And we should probably preserve this behavior. So I'd rather make it:

if (supers_dirty)
	bdi_arm_supers_timer();
set_current_state(TASK_INTERRUPTIBLE);
schedule();

This way we will keep the behavior closer to the original.

> +		supers_dirty = 0;
>  		/*
> -		 * Do this periodically, like kupdated() did before.
> +		 * supers_dirty store must be visible to mark_sb_dirty (below)
> +		 * before sync_supers runs (which loads sb->s_dirty).
>  		 */

Very minor, but the code tends to change quickly, and this note (below)
will probably become out-of-date soon.

> +		smp_mb();

There is spin_lock(&sb_lock) in sync_supers(), so strictly speak this
'smp_mb()' is not needed if we move supers_dirty = 0 into
'sync_supers()' and add a comment that a mb is required, in case some
one modifies the code later?

Or this is not worth it?

>  		sync_supers();
>  	}
>  
>  	return 0;
>  }
>  
> +static void sync_supers_timer_fn(unsigned long unused)
> +{
> +	wake_up_process(sync_supers_tsk);
> +}
> +
>  void bdi_arm_supers_timer(void)
>  {
>  	unsigned long next;
> @@ -384,9 +395,17 @@ void bdi_arm_supers_timer(void)
>  	mod_timer(&sync_supers_timer, round_jiffies_up(next));
>  }
>  
> -static void sync_supers_timer_fn(unsigned long unused)
> +void mark_sb_dirty(struct super_block *sb)
>  {
> -	wake_up_process(sync_supers_tsk);
> +	sb->s_dirty = 1;
> +	/*
> +	 * sb->s_dirty store must be visible to sync_supers (above) before we
> +	 * load supers_dirty in case we need to re-arm the timer.
> +	 */
Similar for the "(above)" note.

> +	smp_mb();
> +	if (likely(supers_dirty))
> +		return;
> +	supers_dirty = 1;
>  	bdi_arm_supers_timer();
>  }

Here is the with my modifications. 

BTW, do you want me to keep you to be the patch author, add your
signed-off-by and my original commit message?

---
 fs/super.c       |    7 +++++++
 mm/backing-dev.c |   21 ++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 2b418fb..c9ff6e2 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -364,6 +364,13 @@ void sync_supers(void)
 {
 	struct super_block *sb, *n;
 
+	supers_dirty = 0;
+	/* smp_mb();
+	 *
+	 * supers_dirty store must be visible to mark_sb_dirty before
+	 * sync_supers runs (which loads sb->s_dirty), so a barrier is needed
+	 * but there is a spin_lock, thus smp_mb is commented out.
+	 */
 	spin_lock(&sb_lock);
 	list_for_each_entry_safe(sb, n, &super_blocks, s_list) {
 		if (list_empty(&sb->s_instances))
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 660a87a..be7f734 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -45,6 +45,7 @@ LIST_HEAD(bdi_pending_list);
 
 static struct task_struct *sync_supers_tsk;
 static struct timer_list sync_supers_timer;
+static unsigned long supers_dirty __read_mostly;
 
 static int bdi_sync_supers(void *);
 static void sync_supers_timer_fn(unsigned long);
@@ -251,7 +252,6 @@ static int __init default_bdi_init(void)
 
 	init_timer(&sync_supers_timer);
 	setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0);
-	bdi_arm_supers_timer();
 
 	err = bdi_init(&default_backing_dev_info);
 	if (!err)
@@ -361,6 +361,8 @@ static int bdi_sync_supers(void *unused)
 	set_user_nice(current, 0);
 
 	while (!kthread_should_stop()) {
+		if (supers_dirty)
+			bdi_arm_supers_timer();
 		set_current_state(TASK_INTERRUPTIBLE);
 		schedule();
 
@@ -373,6 +375,11 @@ static int bdi_sync_supers(void *unused)
 	return 0;
 }
 
+static void sync_supers_timer_fn(unsigned long unused)
+{
+	wake_up_process(sync_supers_tsk);
+}
+
 void bdi_arm_supers_timer(void)
 {
 	unsigned long next;
@@ -384,9 +391,17 @@ void bdi_arm_supers_timer(void)
 	mod_timer(&sync_supers_timer, round_jiffies_up(next));
 }
 
-static void sync_supers_timer_fn(unsigned long unused)
+void mark_sb_dirty(struct super_block *sb)
 {
-	wake_up_process(sync_supers_tsk);
+	sb->s_dirty = 1;
+	/*
+	 * sb->s_dirty store must be visible to sync_supers before we load
+	 * supers_dirty in case we need to re-arm the timer.
+	 */
+	smp_mb();
+	if (likely(supers_dirty))
+		return;
+	supers_dirty = 1;
 	bdi_arm_supers_timer();
 }
 
-- 

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Nick Piggin]

On Thu, May 27, 2010 at 06:21:33PM +0300, Artem Bityutskiy wrote:
> On Thu, 2010-05-27 at 22:07 +1000, Nick Piggin wrote:
> > 1. sb->s_dirty = 1; /* store */
> > 2. if (!supers_timer_armed) /* load */
> > 3.   supers_timer_armed = 1; /* store */
> > 
> > and
> > 
> > A. supers_timer_armed = 0; /* store */
> > B. if (sb->s_dirty) /* load */
> > C.   sb->s_dirty = 0 /* store */
> > 
> > If these two sequences are executed, it must result in
> > sb->s_dirty == 1 iff supers_timer_armed
> > 
> > * If 2 is executed before 1 is visible, then 2 may miss A before B sees 1.
> > * If B is executed before A is visible, then B may miss 1 before 2 sees A.
> > 
> > So we need smp_mb() between 1/2 and A/B (I missed the 2nd one).
> 
> Yes, thanks for elaboration.
> 
> > How about something like this?
> 
> It looks good, many thanks! But I have few small notes.
> 
> > Index: linux-2.6/mm/backing-dev.c
> > ===================================================================
> > --- linux-2.6.orig/mm/backing-dev.c
> > +++ linux-2.6/mm/backing-dev.c
> > @@ -45,6 +45,7 @@ LIST_HEAD(bdi_pending_list);
> >  
> >  static struct task_struct *sync_supers_tsk;
> >  static struct timer_list sync_supers_timer;
> > +static unsigned long supers_dirty __read_mostly;
> >  
> >  static int bdi_sync_supers(void *);
> >  static void sync_supers_timer_fn(unsigned long);
> > @@ -251,7 +252,6 @@ static int __init default_bdi_init(void)
> >  
> >  	init_timer(&sync_supers_timer);
> >  	setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0);
> > -	bdi_arm_supers_timer();
> >  
> >  	err = bdi_init(&default_backing_dev_info);
> >  	if (!err)
> > @@ -362,17 +362,28 @@ static int bdi_sync_supers(void *unused)
> >  
> >  	while (!kthread_should_stop()) {
> >  		set_current_state(TASK_INTERRUPTIBLE);
> > -		schedule();
> > +		if (!supers_dirty)
> > +			schedule();
> > +		else
> > +			__set_current_state(TASK_RUNNING);
> 
> I think this will change the behavior of 'sync_supers()' too much. ATM,
> it makes only one SB pass, then sleeps, then another one, then sleeps.
> And we should probably preserve this behavior. So I'd rather make it:
> 
> if (supers_dirty)
> 	bdi_arm_supers_timer();
> set_current_state(TASK_INTERRUPTIBLE);
> schedule();
> 
> This way we will keep the behavior closer to the original.

Well your previous code had the same issue (ie. it could loop again
in sync_supers). But fair point perhaps.

But we cannot do the above, because again the timer might go off
before we set current state. We'd lose the wakeup and never wake
up again.

Putting it inside set_current_state() should be OK. I suppose.


> > +		supers_dirty = 0;
> >  		/*
> > -		 * Do this periodically, like kupdated() did before.
> > +		 * supers_dirty store must be visible to mark_sb_dirty (below)
> > +		 * before sync_supers runs (which loads sb->s_dirty).
> >  		 */
> 
> Very minor, but the code tends to change quickly, and this note (below)
> will probably become out-of-date soon.

Oh sure, get rid of the "(below)"

 
> > +		smp_mb();
> 
> There is spin_lock(&sb_lock) in sync_supers(), so strictly speak this
> 'smp_mb()' is not needed if we move supers_dirty = 0 into
> 'sync_supers()' and add a comment that a mb is required, in case some
> one modifies the code later?
> 
> Or this is not worth it?

It's a bit tricky. spin_lock only gives an acquire barrier, which
prevents CPU executing instructions inside the critical section
before acquiring the lock. It actually allows stores to be deferred
from becoming visible to other CPUs until inside the critical section.
So the load of sb->s_dirty could indeed still happen before the
store is seen.

Locks do allow you to avoid thinking about barriers, but *only* when
all memory accesses to all shared variables are inside the locks
(or when a section has just a single access, which by definition don't
need ordering with another access).

 
> >  		sync_supers();
> >  	}
> >  
> >  	return 0;
> >  }
> >  
> > +static void sync_supers_timer_fn(unsigned long unused)
> > +{
> > +	wake_up_process(sync_supers_tsk);
> > +}
> > +
> >  void bdi_arm_supers_timer(void)
> >  {
> >  	unsigned long next;
> > @@ -384,9 +395,17 @@ void bdi_arm_supers_timer(void)
> >  	mod_timer(&sync_supers_timer, round_jiffies_up(next));
> >  }
> >  
> > -static void sync_supers_timer_fn(unsigned long unused)
> > +void mark_sb_dirty(struct super_block *sb)
> >  {
> > -	wake_up_process(sync_supers_tsk);
> > +	sb->s_dirty = 1;
> > +	/*
> > +	 * sb->s_dirty store must be visible to sync_supers (above) before we
> > +	 * load supers_dirty in case we need to re-arm the timer.
> > +	 */
> Similar for the "(above)" note.

Sure.

 
> > +	smp_mb();
> > +	if (likely(supers_dirty))
> > +		return;
> > +	supers_dirty = 1;
> >  	bdi_arm_supers_timer();
> >  }
> 
> Here is the with my modifications. 
> 
> BTW, do you want me to keep you to be the patch author, add your
> signed-off-by and my original commit message?

I'm not concerned. You contributed more to the idea+implementation,
so record yourself as author.

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Fri, 2010-05-28 at 01:44 +1000, Nick Piggin wrote:
> > I think this will change the behavior of 'sync_supers()' too much. ATM,
> > it makes only one SB pass, then sleeps, then another one, then sleeps.
> > And we should probably preserve this behavior. So I'd rather make it:
> > 
> > if (supers_dirty)
> > 	bdi_arm_supers_timer();
> > set_current_state(TASK_INTERRUPTIBLE);
> > schedule();
> > 
> > This way we will keep the behavior closer to the original.
> 
> Well your previous code had the same issue (ie. it could loop again
> in sync_supers). But fair point perhaps.

I think no, it either armed the timer of went to sleep, but it does not
matter much :-)

> But we cannot do the above, because again the timer might go off
> before we set current state. We'd lose the wakeup and never wake
> up again.
> 
> Putting it inside set_current_state() should be OK. I suppose.

Oh, right!

> > There is spin_lock(&sb_lock) in sync_supers(), so strictly speak this
> > 'smp_mb()' is not needed if we move supers_dirty = 0 into
> > 'sync_supers()' and add a comment that a mb is required, in case some
> > one modifies the code later?
> > 
> > Or this is not worth it?
> 
> It's a bit tricky. spin_lock only gives an acquire barrier, which
> prevents CPU executing instructions inside the critical section
> before acquiring the lock. It actually allows stores to be deferred
> from becoming visible to other CPUs until inside the critical section.
> So the load of sb->s_dirty could indeed still happen before the
> store is seen.
> 
> Locks do allow you to avoid thinking about barriers, but *only* when
> all memory accesses to all shared variables are inside the locks
> (or when a section has just a single access, which by definition don't
> need ordering with another access).

Oh, ok. I need to read carefully Documentation/memory-barriers.txt.

> > BTW, do you want me to keep you to be the patch author, add your
> > signed-off-by and my original commit message?
> 
> I'm not concerned. You contributed more to the idea+implementation,
> so record yourself as author.

Ok, but thank you a lot for helping!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Andrew Morton]

On Tue, 25 May 2010 16:48:56 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> This patch introduces 3 new VFS helpers: 'mark_sb_dirty()',
> 'mark_sb_clean()', and 'is_sb_dirty()'. The helpers simply
> set 'sb->s_dirt' or test 'sb->s_dirt'. The plan is to make
> every FS use these helpers instead of manipulating the
> 'sb->s_dirt' flag directly.
> 
> Ultimately, this change is a preparation for the periodic
> superblock synchronization optimization which is about
> preventing the "sync_supers" kernel thread from waking up
> even if there is nothing to synchronize.
> 
> This patch also makes VFS use the new helpers.

Patchset generally looks good to me.  But I don't like the names :(

> +static inline void mark_sb_dirty(struct super_block *sb)
> +{
> +	sb->s_dirt = 1;
> +}
> +static inline void mark_sb_clean(struct super_block *sb)
> +{
> +	sb->s_dirt = 0;
> +}
> +static inline int is_sb_dirty(struct super_block *sb)
> +{
> +	return sb->s_dirt;
> +}

A more conventional and superior naming scheme is
subsystemid_specific_function_identifier().  eg, bio_add_page() instead
of add_page_to_bio().

So these want to be sb_mark_dirty(), etc.

Being very old code written by very yound people, the VFS kinda ignores
that convention, but it doesn't hurt to use it for new code.

Feel free to ignore me if that's too much of a PITA ;)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Andrew Morton]

On Tue, 25 May 2010 16:49:12 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> The 'sync_supers' thread wakes up every 5 seconds (by default) and
> writes back all super blocks. It keeps waking up even if there
> are no dirty super-blocks. For many file-systems the superblock
> becomes dirty very rarely, if ever, so 'sync_supers' does not do
> anything most of the time.
> 
> This patch improves 'sync_supers' and makes sleep if all superblocks
> are clean and there is nothing to do. This helps saving the power.
> This optimization is important for small battery-powered devices.
> 
> Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> ---
>  include/linux/fs.h |    5 +----
>  mm/backing-dev.c   |   36 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c2ddeee..2d2560b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1786,10 +1786,7 @@ extern void simple_set_mnt(struct vfsmount *mnt, struct super_block *sb);
>   * Note, VFS does not provide any serialization for the super block clean/dirty
>   * state changes, file-systems should take care of this.
>   */
> -static inline void mark_sb_dirty(struct super_block *sb)
> -{
> -	sb->s_dirty = 1;
> -}
> +void mark_sb_dirty(struct super_block *sb);
>  static inline void mark_sb_clean(struct super_block *sb)
>  {
>  	sb->s_dirty = 0;
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 660a87a..14f3eb7 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -45,6 +45,8 @@ LIST_HEAD(bdi_pending_list);
>  
>  static struct task_struct *sync_supers_tsk;
>  static struct timer_list sync_supers_timer;
> +static int supers_timer_armed;

This thing's a bit ugly.

> +static DEFINE_SPINLOCK(supers_timer_lock);
>  
>  static int bdi_sync_supers(void *);
>  static void sync_supers_timer_fn(unsigned long);
> @@ -355,6 +357,11 @@ static void bdi_flush_io(struct backing_dev_info *bdi)
>   * or we risk deadlocking on ->s_umount. The longer term solution would be
>   * to implement sync_supers_bdi() or similar and simply do it from the
>   * bdi writeback tasks individually.
> + *
> + * Historically this thread woke up periodically, regardless of whether
> + * there was any dirty superblock. However, nowadays it is optimized to
> + * wake up only when there is something to synchronize - this helps to save
> + * power.
>   */
>  static int bdi_sync_supers(void *unused)
>  {
> @@ -364,10 +371,24 @@ static int bdi_sync_supers(void *unused)
>  		set_current_state(TASK_INTERRUPTIBLE);
>  		schedule();
>  
> +		spin_lock(&supers_timer_lock);
> +		/* Indicate that 'sync_supers' is in progress */
> +		supers_timer_armed = -1;
> +		spin_unlock(&supers_timer_lock);
> +
>  		/*
>  		 * Do this periodically, like kupdated() did before.
>  		 */
>  		sync_supers();
> +
> +		spin_lock(&supers_timer_lock);
> +		if (supers_timer_armed == 1)
> +			/* A super block was marked as dirty meanwhile */
> +			bdi_arm_supers_timer();
> +		else
> +			/* No more dirty superblocks - we've synced'em all */
> +			supers_timer_armed = 0;
> +		spin_unlock(&supers_timer_lock);
>  	}

I suspect the spinlock could be removed if you switched to bitops:
test_and_set_bit(0, supers_timer_armed);

Ahother possibility is to nuke supers_timer_armed() and use
timer_pending(), mod_timer(), etc directly.


>  	return 0;
> @@ -387,9 +408,22 @@ void bdi_arm_supers_timer(void)
>  static void sync_supers_timer_fn(unsigned long unused)
>  {
>  	wake_up_process(sync_supers_tsk);
> -	bdi_arm_supers_timer();
>  }
>  
> +void mark_sb_dirty(struct super_block *sb)
> +{
> +	sb->s_dirty = 1;
> +
> +	spin_lock(&supers_timer_lock);
> +	if (!supers_timer_armed) {
> +		bdi_arm_supers_timer();
> +		supers_timer_armed = 1;
> +	} else if (supers_timer_armed == -1)
> +		supers_timer_armed = 1;
> +	spin_unlock(&supers_timer_lock);
> +}
> +EXPORT_SYMBOL(mark_sb_dirty);

This looks inefficient.  Could we not do

void mark_sb_dirty(struct super_block *sb)
{
	sb->s_dirty = 1;

	if (!supers_timer_armed) {
		spin_lock(&supers_timer_lock);
		if (!supers_timer_armed) {
			bdi_arm_supers_timer();
			supers_timer_armed = 1;
		}
	} else if (supers_timer_armed == -1)
		spin_lock(&supers_timer_lock);
		if (supers_timer_armed == -1)
			supers_timer_armed = 1;
		spin_unlock(&supers_timer_lock);
	}
}

I didn't try very hard there, but you get the idea: examine the state
before taking that expensive global spinlock, so we only end up taking
the lock once per five seconds, rather than once per possible
superblock dirtying.  That's like a six-orders-of-magnitude reduction
in locking frequency, which is worth putting some effort into.

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Al Viro]

On Fri, May 28, 2010 at 01:23:18PM -0700, Andrew Morton wrote:

> A more conventional and superior naming scheme is
> subsystemid_specific_function_identifier().  eg, bio_add_page() instead
> of add_page_to_bio().
> 
> So these want to be sb_mark_dirty(), etc.
> 
> Being very old code written by very yound people, the VFS kinda ignores
> that convention, but it doesn't hurt to use it for new code.
> 
> Feel free to ignore me if that's too much of a PITA ;)

The real issue is that it's almost certainly an overdesign.  Let's
get rid of the bogus uses first and figure out what's happening in
what remains, OK?

I have no problems with doing such wrappers, but if we touch every
place using ->s_dirt anyway, let's at least take a good look at them.

I'm mostly OK with what had emerged for the final patch in series,
but...

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Andrew Morton]

On Fri, 28 May 2010 22:14:32 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Fri, May 28, 2010 at 01:23:18PM -0700, Andrew Morton wrote:
> 
> > A more conventional and superior naming scheme is
> > subsystemid_specific_function_identifier().  eg, bio_add_page() instead
> > of add_page_to_bio().
> > 
> > So these want to be sb_mark_dirty(), etc.
> > 
> > Being very old code written by very yound people, the VFS kinda ignores
> > that convention, but it doesn't hurt to use it for new code.
> > 
> > Feel free to ignore me if that's too much of a PITA ;)
> 
> The real issue is that it's almost certainly an overdesign.  Let's
> get rid of the bogus uses first and figure out what's happening in
> what remains, OK?

That would be good.

> I have no problems with doing such wrappers, but if we touch every
> place using ->s_dirt anyway, let's at least take a good look at them.

When adding wrappers we should also rename ->s_dirt (say, to __s_dirt)
to catch out any unconverted code.

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Artem Bityutskiy]

On Fri, 2010-05-28 at 13:23 -0700, Andrew Morton wrote:
> > +static inline void mark_sb_dirty(struct super_block *sb)
> > +{
> > +	sb->s_dirt = 1;
> > +}
> > +static inline void mark_sb_clean(struct super_block *sb)
> > +{
> > +	sb->s_dirt = 0;
> > +}
> > +static inline int is_sb_dirty(struct super_block *sb)
> > +{
> > +	return sb->s_dirt;
> > +}
> 
> A more conventional and superior naming scheme is
> subsystemid_specific_function_identifier().  eg, bio_add_page() instead
> of add_page_to_bio().
> 
> So these want to be sb_mark_dirty(), etc.
> 
> Being very old code written by very yound people, the VFS kinda ignores
> that convention, but it doesn't hurt to use it for new code.
> 
> Feel free to ignore me if that's too much of a PITA ;)

Sure I'll re-name them, thanks!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Fri, 2010-05-28 at 13:29 -0700, Andrew Morton wrote:
> void mark_sb_dirty(struct super_block *sb)
> {
> 	sb->s_dirty = 1;
> 
> 	if (!supers_timer_armed) {
> 		spin_lock(&supers_timer_lock);
> 		if (!supers_timer_armed) {
> 			bdi_arm_supers_timer();
> 			supers_timer_armed = 1;
> 		}
> 	} else if (supers_timer_armed == -1)
> 		spin_lock(&supers_timer_lock);
> 		if (supers_timer_armed == -1)
> 			supers_timer_armed = 1;
> 		spin_unlock(&supers_timer_lock);
> 	}
> }
> 
> I didn't try very hard there, but you get the idea: examine the state
> before taking that expensive global spinlock, so we only end up taking
> the lock once per five seconds, rather than once per possible
> superblock dirtying.  That's like a six-orders-of-magnitude reduction
> in locking frequency, which is worth putting some effort into.

Andrew, thanks for review!

I just did not consider spinlock to be expensive because I thought that
marking superblock as dirty is a relatively rare operation. And my small
experiments kind of confirmed that.

But  Nick suggested a good locking scheme which uses only smp_mb() in
this thread, which I am going to stick with.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Artem Bityutskiy]

On Fri, 2010-05-28 at 14:17 -0700, Andrew Morton wrote:
> On Fri, 28 May 2010 22:14:32 +0100
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Fri, May 28, 2010 at 01:23:18PM -0700, Andrew Morton wrote:
> > 
> > > A more conventional and superior naming scheme is
> > > subsystemid_specific_function_identifier().  eg, bio_add_page() instead
> > > of add_page_to_bio().
> > > 
> > > So these want to be sb_mark_dirty(), etc.
> > > 
> > > Being very old code written by very yound people, the VFS kinda ignores
> > > that convention, but it doesn't hurt to use it for new code.
> > > 
> > > Feel free to ignore me if that's too much of a PITA ;)
> > 
> > The real issue is that it's almost certainly an overdesign.  Let's
> > get rid of the bogus uses first and figure out what's happening in
> > what remains, OK?
> 
> That would be good.

Yes, I just mechanically introduced the wrappers to all FS-es. But as
per Al's request, I am going to try looking at how FSwe use it and
validate the usage. It'll take some time as this stuff is my background
task. Will see.

> > I have no problems with doing such wrappers, but if we touch every
> > place using ->s_dirt anyway, let's at least take a good look at them.
> 
> When adding wrappers we should also rename ->s_dirt (say, to __s_dirt)
> to catch out any unconverted code.

Right, I did this in the following patch:
	[PATCHv4 16/17] VFS: rename s_dirt to s_dirty

I thought that adding a leading '_' is not very neat, so added 'y' at
the end.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Fri, 2010-05-28 at 01:44 +1000, Nick Piggin wrote:
> > if (supers_dirty)
> > 	bdi_arm_supers_timer();
> > set_current_state(TASK_INTERRUPTIBLE);
> > schedule();

> But we cannot do the above, because again the timer might go off
> before we set current state. We'd lose the wakeup and never wake
> up again.
> 
> Putting it inside set_current_state() should be OK. I suppose.

Hmm, but it looks like we cannot do that either. If we do

set_current_state(TASK_INTERRUPTIBLE);
if (supers_dirty)
	bdi_arm_supers_timer();
schedule();

and the kernel is preemptive, is it possible that we get preempted
before we run 'bdi_arm_supers_timer()', but after we do
'set_current_state(TASK_INTERRUPTIBLE)'. And we will never wake up if
the timer armed in mark_sb_dirty() went off.

So it looks like this is the way to go:

/*
 * Disable preemption for a while to make sure we are not
 * preempted before the timer is armed.
 */
preempt_disable();
set_current_state(TASK_INTERRUPTIBLE);
if (supers_dirty)
	bdi_arm_supers_timer();
preempt_enable();
schedule();

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Nick Piggin]

On Mon, May 31, 2010 at 11:25:52AM +0300, Artem Bityutskiy wrote:
> On Fri, 2010-05-28 at 01:44 +1000, Nick Piggin wrote:
> > > if (supers_dirty)
> > > 	bdi_arm_supers_timer();
> > > set_current_state(TASK_INTERRUPTIBLE);
> > > schedule();
> 
> > But we cannot do the above, because again the timer might go off
> > before we set current state. We'd lose the wakeup and never wake
> > up again.
> > 
> > Putting it inside set_current_state() should be OK. I suppose.
> 
> Hmm, but it looks like we cannot do that either. If we do
> 
> set_current_state(TASK_INTERRUPTIBLE);
> if (supers_dirty)
> 	bdi_arm_supers_timer();
> schedule();
> 
> and the kernel is preemptive, is it possible that we get preempted
> before we run 'bdi_arm_supers_timer()', but after we do
> 'set_current_state(TASK_INTERRUPTIBLE)'. And we will never wake up if
> the timer armed in mark_sb_dirty() went off.
> 
> So it looks like this is the way to go:
> 
> /*
>  * Disable preemption for a while to make sure we are not
>  * preempted before the timer is armed.
>  */
> preempt_disable();
> set_current_state(TASK_INTERRUPTIBLE);
> if (supers_dirty)
> 	bdi_arm_supers_timer();
> preempt_enable();
> schedule();

This should not be required because preempt is transparent to these
task sleep/schedule APIs.

The preempt event will not clear TASK_INTERRUPTIBLE, and so the timer
wakeup will set it to TASK_RUNNING (whether or not it has called
schedule() yet and whether or not it is currently preempted).

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

> On Mon, May 31, 2010 at 11:25:52AM +0300, Artem Bityutskiy wrote:
> > Hmm, but it looks like we cannot do that either. If we do
> > 
> > set_current_state(TASK_INTERRUPTIBLE);
> > if (supers_dirty)
> > 	bdi_arm_supers_timer();
> > schedule();
> > 
> > and the kernel is preemptive, is it possible that we get preempted
> > before we run 'bdi_arm_supers_timer()', but after we do
> > 'set_current_state(TASK_INTERRUPTIBLE)'. And we will never wake up if
> > the timer armed in mark_sb_dirty() went off.
> > 
> > So it looks like this is the way to go:
> > 
> > /*
> >  * Disable preemption for a while to make sure we are not
> >  * preempted before the timer is armed.
> >  */
> > preempt_disable();
> > set_current_state(TASK_INTERRUPTIBLE);
> > if (supers_dirty)
> > 	bdi_arm_supers_timer();
> > preempt_enable();
> > schedule();
> 
> This should not be required because preempt is transparent to these
> task sleep/schedule APIs.
> 
> The preempt event will not clear TASK_INTERRUPTIBLE, and so the timer
> wakeup will set it to TASK_RUNNING (whether or not it has called
> schedule() yet and whether or not it is currently preempted).

Nick, I'm sorry, but could you please elaborate:

set_current_state(TASK_INTERRUPTIBLE);
/* 
 * XXX: what if we are preempted here. No timer is armed. Our state is
 * TASK_INTERRUPTIBLE, supers_dirty is 1, so no one will ever wake us
 * up. Thus, we'll sleep forever.
 */
if (supers_dirty)
	bdi_arm_supers_timer();
schedule();

Not sure, but I did quick search and it looks like in preemptive kernel,
an interrupt may happen in the XXX place above, then it will call
'preempt_schedule_irq()', which sill call 'schedule()'.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Nick Piggin]

On Mon, May 31, 2010 at 12:04:04PM +0300, Artem Bityutskiy wrote:
> > On Mon, May 31, 2010 at 11:25:52AM +0300, Artem Bityutskiy wrote:
> > > Hmm, but it looks like we cannot do that either. If we do
> > > 
> > > set_current_state(TASK_INTERRUPTIBLE);
> > > if (supers_dirty)
> > > 	bdi_arm_supers_timer();
> > > schedule();
> > > 
> > > and the kernel is preemptive, is it possible that we get preempted
> > > before we run 'bdi_arm_supers_timer()', but after we do
> > > 'set_current_state(TASK_INTERRUPTIBLE)'. And we will never wake up if
> > > the timer armed in mark_sb_dirty() went off.
> > > 
> > > So it looks like this is the way to go:
> > > 
> > > /*
> > >  * Disable preemption for a while to make sure we are not
> > >  * preempted before the timer is armed.
> > >  */
> > > preempt_disable();
> > > set_current_state(TASK_INTERRUPTIBLE);
> > > if (supers_dirty)
> > > 	bdi_arm_supers_timer();
> > > preempt_enable();
> > > schedule();
> > 
> > This should not be required because preempt is transparent to these
> > task sleep/schedule APIs.
> > 
> > The preempt event will not clear TASK_INTERRUPTIBLE, and so the timer
> > wakeup will set it to TASK_RUNNING (whether or not it has called
> > schedule() yet and whether or not it is currently preempted).
> 
> Nick, I'm sorry, but could you please elaborate:
> 
> set_current_state(TASK_INTERRUPTIBLE);
> /* 
>  * XXX: what if we are preempted here. No timer is armed. Our state is
>  * TASK_INTERRUPTIBLE, supers_dirty is 1, so no one will ever wake us
>  * up. Thus, we'll sleep forever.
>  */
> if (supers_dirty)
> 	bdi_arm_supers_timer();
> schedule();
> 
> Not sure, but I did quick search and it looks like in preemptive kernel,
> an interrupt may happen in the XXX place above, then it will call
> 'preempt_schedule_irq()', which sill call 'schedule()'.

Yes, preempt does not participate in tsak sleeping exactly for reasons
such as this.

>From kernel/sched.c:schedule()

        if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
                if (unlikely(signal_pending_state(prev->state, prev)))
                        prev->state = TASK_RUNNING;
                else
                        deactivate_task(rq, prev, DEQUEUE_SLEEP);
                switch_count = &prev->nvcsw;
        }

If the task is not running, then is only removed from the runqueue
(or reset to running in case of pending signal) IFF it has not been
scheduled from an involuntary kernel preemption.

So in the XXX region, the task will actually be allowed to run again
until it calls schedule().

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Mon, 2010-05-31 at 22:47 +1000, Nick Piggin wrote:
> > set_current_state(TASK_INTERRUPTIBLE);
> > /* 
> >  * XXX: what if we are preempted here. No timer is armed. Our state is
> >  * TASK_INTERRUPTIBLE, supers_dirty is 1, so no one will ever wake us
> >  * up. Thus, we'll sleep forever.
> >  */
> > if (supers_dirty)
> > 	bdi_arm_supers_timer();
> > schedule();
> > 
> > Not sure, but I did quick search and it looks like in preemptive kernel,
> > an interrupt may happen in the XXX place above, then it will call
> > 'preempt_schedule_irq()', which sill call 'schedule()'.
> 
> Yes, preempt does not participate in tsak sleeping exactly for reasons
> such as this.
> 
> From kernel/sched.c:schedule()
> 
>         if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) {
>                 if (unlikely(signal_pending_state(prev->state, prev)))
>                         prev->state = TASK_RUNNING;
>                 else
>                         deactivate_task(rq, prev, DEQUEUE_SLEEP);
>                 switch_count = &prev->nvcsw;
>         }
> 
> If the task is not running, then is only removed from the runqueue
> (or reset to running in case of pending signal) IFF it has not been
> scheduled from an involuntary kernel preemption.
> 
> So in the XXX region, the task will actually be allowed to run again
> until it calls schedule().

Clear now, thanks a lot again!

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Thu, 2010-05-27 at 07:50 +0100, Al Viro wrote:
> Note, e.g., that in your series you've touched udf; it can set s_dirt
> until the cows come home, but without ->write_super() it'll be ignored
> by everything in VFS and fs/udf itself never looks at the damn thing.
> 
> A look around it shows fs/sysv, where we never clean the damn flag anymore
> for r/w mounts.  Yes, really (got broken a year ago, nobody noticed).
> 
> Or, e.g., BFS - there we have ->write_super() mark the buffer_head that
> contains on-disk sb dirty, and the only place that sets ->s_dirt is doing
> that immediately after having marked the same bh dirty itself.  Interesting
> place, at that - bfs_fill_super() at r/w mount time...  Note that ->sync_fs()
> there does *not* wait for anything, which is not the right thing to do.
> 
> IOW, this thing is a good topic for code review; I suspect that quite a few
> users might be gone as the result.

Al,

you requested me to review s_dirt usage, well, I'm trying now. One thing
I do not understand is s_dirt serialization, which seems to be just
absent in some FSes. I checked affs and ext2. E.g., affs does:

affs_alloc_block()
{
        mark_buffer_dirty(bh);
        sb->s_dirt = 1;
}

vs

affs_write_super()
{
	affs_commit_super();
	/* YYY: what if sb is marked as dirty right here? */
	sb->s_dirt = 0;
}

vs

/* This wakes up periodically */
sync_super()
{
	if (sb->s_root && sb->s_dirt)
		sb->s_op->write_super(sb);
}

ext2 seems to be doing something similar. It seems to me that FSes
should serialize s_dirt changes somehow, but they don't? Why this is not
a problem?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Al Viro]

On Mon, May 31, 2010 at 05:07:00PM +0300, Artem Bityutskiy wrote:
> you requested me to review s_dirt usage, well, I'm trying now. One thing
> I do not understand is s_dirt serialization, which seems to be just
> absent in some FSes. I checked affs and ext2. E.g., affs does:
> 
> affs_alloc_block()
> {
>         mark_buffer_dirty(bh);
>         sb->s_dirt = 1;
> }
> 
> vs
> 
> affs_write_super()
> {
> 	affs_commit_super();
> 	/* YYY: what if sb is marked as dirty right here? */
> 	sb->s_dirt = 0;
> }
> 
> vs
> 
> /* This wakes up periodically */
> sync_super()
> {
> 	if (sb->s_root && sb->s_dirt)
> 		sb->s_op->write_super(sb);
> }
> 
> ext2 seems to be doing something similar. It seems to me that FSes
> should serialize s_dirt changes somehow, but they don't? Why this is not
> a problem?

I suspect that most of those used to rely on lock_super() way back.
In case of ext2_sync_super() we probably want just to move ->s_dirt = 0
into the very beginning; no serialization is really needed beyond (_maybe_)
some barriers.  No idea about affs, needs to be checked.

Re: [PATCHv4 17/17] writeback: lessen sync_supers wakeup count

[Artem Bityutskiy]

On Fri, 2010-06-04 at 05:26 +0100, Al Viro wrote:
> I suspect that most of those used to rely on lock_super() way back.
> In case of ext2_sync_super() we probably want just to move ->s_dirt = 0
> into the very beginning; no serialization is really needed beyond (_maybe_)
> some barriers.  No idea about affs, needs to be checked.

Yeah, moving s_dirt to the beginning should work. I think the same
should be done for affs. I'll look at other FS-es WRT this as well.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[tytso]

On Fri, May 28, 2010 at 02:17:55PM -0700, Andrew Morton wrote:
> > 
> > The real issue is that it's almost certainly an overdesign.  Let's
> > get rid of the bogus uses first and figure out what's happening in
> > what remains, OK?
> 
> That would be good.

Can we figure out what the new names will be for these accessor
functions, and then pursuade Linus to be willing to add patch #1 in
this series to add these accessor functions (without any users for
these functions, that would wait until the next merge window) to
2.6.35-rc3 or -rc4, please?

It will make life much easier for fs maintainers to merge the patches,
especially if they've done some cleanup to reduce the bogus places
where s_dirt was getting set in the first place.  That way I can apply
my patch to reduce the use of s_dirt[1], then apply a patch I carry in
my own tree to convert to the new accessor functions without worrying
about patch conflicts.

[1] http://article.gmane.org/gmane.comp.file-systems.ext4/19499

Thanks,

						- Ted

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Artem Bityutskiy]

On Wed, 2010-06-09 at 11:44 -0400, tytso@mit.edu wrote:
> On Fri, May 28, 2010 at 02:17:55PM -0700, Andrew Morton wrote:
> > > 
> > > The real issue is that it's almost certainly an overdesign.  Let's
> > > get rid of the bogus uses first and figure out what's happening in
> > > what remains, OK?
> > 
> > That would be good.
> 
> Can we figure out what the new names will be for these accessor
> functions, and then pursuade Linus to be willing to add patch #1 in
> this series to add these accessor functions (without any users for
> these functions, that would wait until the next merge window) to
> 2.6.35-rc3 or -rc4, please?
> 
> It will make life much easier for fs maintainers to merge the patches,
> especially if they've done some cleanup to reduce the bogus places
> where s_dirt was getting set in the first place.  That way I can apply
> my patch to reduce the use of s_dirt[1], then apply a patch I carry in
> my own tree to convert to the new accessor functions without worrying
> about patch conflicts.

Yes, that would be nice, Al?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Andrew Morton]

On Wed, 9 Jun 2010 11:44:49 -0400 tytso@mit.edu wrote:

> On Fri, May 28, 2010 at 02:17:55PM -0700, Andrew Morton wrote:
> > > 
> > > The real issue is that it's almost certainly an overdesign.  Let's
> > > get rid of the bogus uses first and figure out what's happening in
> > > what remains, OK?
> > 
> > That would be good.
> 
> Can we figure out what the new names will be for these accessor
> functions,

sb_mark_dirty(), sb_mark_clean(), sb_is_dirty().

> and then pursuade Linus to be willing to add patch #1 in
> this series to add these accessor functions (without any users for
> these functions, that would wait until the next merge window) to
> 2.6.35-rc3 or -rc4, please?

I expect he'd be OK with that.

> It will make life much easier for fs maintainers to merge the patches,
> especially if they've done some cleanup to reduce the bogus places
> where s_dirt was getting set in the first place.

For that reason.

Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag

[Al Viro]

On Wed, Jun 09, 2010 at 09:31:57AM -0700, Andrew Morton wrote:
> > Can we figure out what the new names will be for these accessor
> > functions,
> 
> sb_mark_dirty(), sb_mark_clean(), sb_is_dirty().

Fine by me.  If Linus doesn't take such a patch, I certainly will and put
it into for-next ASAP.