| Message ID | 1274795352-3551-2-git-send-email-dedekind1@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 ;) -- 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
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... -- 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
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. -- 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
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!
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.
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 -- 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
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?
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. -- 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
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. -- 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
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))