From: Artem Bityutskiy <[email protected]>
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 <[email protected]>
Cc: Roman Zippel <[email protected]>
Cc: Tigran A. Aivazian <[email protected]>
Cc: Chris Mason <[email protected]>
Cc: Boaz Harrosh <[email protected]>
Cc: [email protected]
Cc: Theodore Ts'o <[email protected]>
Cc: OGAWA Hirofumi <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: [email protected]
Cc: Jan Kara <[email protected]>
Cc: Evgeniy Dushistov <[email protected]>
---
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
On Tue, 25 May 2010 16:48:56 +0300
Artem Bityutskiy <[email protected]> wrote:
> From: Artem Bityutskiy <[email protected]>
>
> 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 ;)
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...
On Fri, 28 May 2010 22:14:32 +0100
Al Viro <[email protected]> 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.
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 (Артём Битюцкий)
On Fri, 2010-05-28 at 14:17 -0700, Andrew Morton wrote:
> On Fri, 28 May 2010 22:14:32 +0100
> Al Viro <[email protected]> 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 (Артём Битюцкий)
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
On Wed, 2010-06-09 at 11:44 -0400, [email protected] 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 (Артём Битюцкий)
On Wed, 9 Jun 2010 11:44:49 -0400 [email protected] 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.
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.