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

[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

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

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

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

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.

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

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

[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