linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
       [not found] <1274795352-3551-1-git-send-email-dedekind1@gmail.com>
@ 2010-05-25 13:48 ` Artem Bityutskiy
  2010-05-28 20:23   ` Andrew Morton
  2010-05-25 13:49 ` [PATCHv4 06/17] EXT2: do not manipulate s_dirt directly Artem Bityutskiy
  2010-05-25 13:49 ` [PATCHv4 07/17] EXT4: " Artem Bityutskiy
  2 siblings, 1 reply; 12+ messages in thread
From: Artem Bityutskiy @ 2010-05-25 13:48 UTC (permalink / raw)
  To: Al Viro
  Cc: LKML, Jens Axboe, linux-fsdevel, Roman Zippel, Tigran A. Aivazian,
	Chris Mason, Boaz Harrosh, linux-ext4, Theodore Ts'o,
	OGAWA Hirofumi, David Woodhouse, reiserfs-devel, Jan Kara,
	Evgeniy Dushistov

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCHv4 06/17] EXT2: do not manipulate s_dirt directly
       [not found] <1274795352-3551-1-git-send-email-dedekind1@gmail.com>
  2010-05-25 13:48 ` [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag Artem Bityutskiy
@ 2010-05-25 13:49 ` Artem Bityutskiy
  2010-05-25 13:49 ` [PATCHv4 07/17] EXT4: " Artem Bityutskiy
  2 siblings, 0 replies; 12+ messages in thread
From: Artem Bityutskiy @ 2010-05-25 13:49 UTC (permalink / raw)
  To: Al Viro; +Cc: LKML, Jens Axboe, linux-fsdevel, linux-ext4

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCHv4 07/17] EXT4: do not manipulate s_dirt directly
       [not found] <1274795352-3551-1-git-send-email-dedekind1@gmail.com>
  2010-05-25 13:48 ` [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag Artem Bityutskiy
  2010-05-25 13:49 ` [PATCHv4 06/17] EXT2: do not manipulate s_dirt directly Artem Bityutskiy
@ 2010-05-25 13:49 ` Artem Bityutskiy
  2 siblings, 0 replies; 12+ messages in thread
From: Artem Bityutskiy @ 2010-05-25 13:49 UTC (permalink / raw)
  To: Al Viro; +Cc: LKML, Jens Axboe, linux-fsdevel, linux-ext4, Theodore Ts'o

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-05-25 13:48 ` [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag Artem Bityutskiy
@ 2010-05-28 20:23   ` Andrew Morton
  2010-05-28 21:14     ` Al Viro
  2010-05-29  7:59     ` Artem Bityutskiy
  0 siblings, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2010-05-28 20:23 UTC (permalink / raw)
  To: Artem Bityutskiy
  Cc: Al Viro, LKML, Jens Axboe, linux-fsdevel, Roman Zippel,
	Tigran A. Aivazian, Chris Mason, Boaz Harrosh, linux-ext4,
	Theodore Ts'o, OGAWA Hirofumi, David Woodhouse,
	reiserfs-devel, Jan Kara, Evgeniy Dushistov

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 ;)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-05-28 20:23   ` Andrew Morton
@ 2010-05-28 21:14     ` Al Viro
  2010-05-28 21:17       ` Andrew Morton
  2010-05-29  7:59     ` Artem Bityutskiy
  1 sibling, 1 reply; 12+ messages in thread
From: Al Viro @ 2010-05-28 21:14 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Artem Bityutskiy, LKML, Jens Axboe, linux-fsdevel, Roman Zippel,
	Tigran A. Aivazian, Chris Mason, Boaz Harrosh, linux-ext4,
	Theodore Ts'o, OGAWA Hirofumi, David Woodhouse,
	reiserfs-devel, Jan Kara, Evgeniy Dushistov

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-05-28 21:14     ` Al Viro
@ 2010-05-28 21:17       ` Andrew Morton
  2010-05-29  8:11         ` Artem Bityutskiy
  2010-06-09 15:44         ` tytso
  0 siblings, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2010-05-28 21:17 UTC (permalink / raw)
  To: Al Viro
  Cc: Artem Bityutskiy, LKML, Jens Axboe, linux-fsdevel, Roman Zippel,
	Tigran A. Aivazian, Chris Mason, Boaz Harrosh, linux-ext4,
	Theodore Ts'o, OGAWA Hirofumi, David Woodhouse,
	reiserfs-devel, Jan Kara, Evgeniy Dushistov

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-05-28 20:23   ` Andrew Morton
  2010-05-28 21:14     ` Al Viro
@ 2010-05-29  7:59     ` Artem Bityutskiy
  1 sibling, 0 replies; 12+ messages in thread
From: Artem Bityutskiy @ 2010-05-29  7:59 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Al Viro, LKML, Jens Axboe, linux-fsdevel, Roman Zippel,
	Tigran A. Aivazian, Chris Mason, Boaz Harrosh, linux-ext4,
	Theodore Ts'o, OGAWA Hirofumi, David Woodhouse,
	reiserfs-devel, Jan Kara, Evgeniy Dushistov

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 (Артём Битюцкий)

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-05-28 21:17       ` Andrew Morton
@ 2010-05-29  8:11         ` Artem Bityutskiy
  2010-06-09 15:44         ` tytso
  1 sibling, 0 replies; 12+ messages in thread
From: Artem Bityutskiy @ 2010-05-29  8:11 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Al Viro, LKML, Jens Axboe, linux-fsdevel, Roman Zippel,
	Tigran A. Aivazian, Chris Mason, Boaz Harrosh, linux-ext4,
	Theodore Ts'o, OGAWA Hirofumi, David Woodhouse,
	reiserfs-devel, Jan Kara, Evgeniy Dushistov

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 (Артём Битюцкий)

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-05-28 21:17       ` Andrew Morton
  2010-05-29  8:11         ` Artem Bityutskiy
@ 2010-06-09 15:44         ` tytso
  2010-06-09 15:49           ` Artem Bityutskiy
  2010-06-09 16:31           ` Andrew Morton
  1 sibling, 2 replies; 12+ messages in thread
From: tytso @ 2010-06-09 15:44 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Al Viro, Artem Bityutskiy, LKML, Jens Axboe, linux-fsdevel,
	Roman Zippel, Tigran A. Aivazian, Chris Mason, Boaz Harrosh,
	linux-ext4, OGAWA Hirofumi, David Woodhouse, reiserfs-devel,
	Jan Kara, Evgeniy Dushistov

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-06-09 15:44         ` tytso
@ 2010-06-09 15:49           ` Artem Bityutskiy
  2010-06-09 16:31           ` Andrew Morton
  1 sibling, 0 replies; 12+ messages in thread
From: Artem Bityutskiy @ 2010-06-09 15:49 UTC (permalink / raw)
  To: tytso
  Cc: Andrew Morton, Al Viro, LKML, Jens Axboe, linux-fsdevel,
	Roman Zippel, Tigran A. Aivazian, Chris Mason, Boaz Harrosh,
	linux-ext4, OGAWA Hirofumi, David Woodhouse, reiserfs-devel,
	Jan Kara, Evgeniy Dushistov

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 (Артём Битюцкий)

--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-06-09 15:44         ` tytso
  2010-06-09 15:49           ` Artem Bityutskiy
@ 2010-06-09 16:31           ` Andrew Morton
  2010-06-09 22:33             ` Al Viro
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2010-06-09 16:31 UTC (permalink / raw)
  To: tytso
  Cc: Al Viro, Artem Bityutskiy, LKML, Jens Axboe, linux-fsdevel,
	Roman Zippel, Tigran A. Aivazian, Chris Mason, Boaz Harrosh,
	linux-ext4, OGAWA Hirofumi, David Woodhouse, reiserfs-devel,
	Jan Kara, Evgeniy Dushistov

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag
  2010-06-09 16:31           ` Andrew Morton
@ 2010-06-09 22:33             ` Al Viro
  0 siblings, 0 replies; 12+ messages in thread
From: Al Viro @ 2010-06-09 22:33 UTC (permalink / raw)
  To: Andrew Morton
  Cc: tytso, Artem Bityutskiy, LKML, Jens Axboe, linux-fsdevel,
	Roman Zippel, Tigran A. Aivazian, Chris Mason, Boaz Harrosh,
	linux-ext4, OGAWA Hirofumi, David Woodhouse, reiserfs-devel,
	Jan Kara, Evgeniy Dushistov

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2010-06-09 22:33 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1274795352-3551-1-git-send-email-dedekind1@gmail.com>
2010-05-25 13:48 ` [PATCHv4 01/17] VFS: introduce helpers for the s_dirty flag Artem Bityutskiy
2010-05-28 20:23   ` Andrew Morton
2010-05-28 21:14     ` Al Viro
2010-05-28 21:17       ` Andrew Morton
2010-05-29  8:11         ` Artem Bityutskiy
2010-06-09 15:44         ` tytso
2010-06-09 15:49           ` Artem Bityutskiy
2010-06-09 16:31           ` Andrew Morton
2010-06-09 22:33             ` Al Viro
2010-05-29  7:59     ` Artem Bityutskiy
2010-05-25 13:49 ` [PATCHv4 06/17] EXT2: do not manipulate s_dirt directly Artem Bityutskiy
2010-05-25 13:49 ` [PATCHv4 07/17] EXT4: " Artem Bityutskiy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).