Re: [PATCH] 2.5.x write_super is not for syncing

[Andrew Morton <akpm@digeo.com>]

Chris Mason wrote:
> 
> On Mon, 2002-12-02 at 17:07, Andrew Morton wrote:
> 
> > Except for the s_dirt test in here.  If s_dirt is zero and we
> > have dirty inodes, the filesystem _still_ is not told what to
> > do.
> 
> Perhaps dirty inodes imply s_dirt should be true?

Well afaik (why doesn't anybody ever comment stuff?  Do they prefer
buggy software?) s_dirt purely means "the superblock needs to be
written out".

> >
> > I don't think we'll ever get this right until we start telling the
> > filesystem what's happening.  So instead of all these little
> > presumptuous micro-syncs which we're doing in there, we need to turn
> > this inside out and just call sb->s_op->sync_everything_for_umount()
> > and let the fs decide how to get everything tight on disk.
> >
> > That's a bit drastic.  At a minimum we need to remove that s_dirt
> > test and make the commit_super() call unconditional.
> >
> > What would that break?
> 
> Not much, since foofs_commit_super could always check for s_dirt if the
> local FS really cared.
> 
> The downside is that since we don't check for s_dirt in commit_supers(),
> the FS might get sunk twice if the we need to restart at the head of the
> supers list due to unmount.  But, a double sync is possible anyway under
> FS load in the same situation.
> 

But you do check:

+static inline void commit_super(struct super_block *sb)
+{
+       lock_super(sb);
+       if (sb->s_root && sb->s_dirt) {
+               if (sb->s_op->write_super)
+                       sb->s_op->write_super(sb);
+               if (sb->s_op->commit_super)
+                       sb->s_op->commit_super(sb);
+       }
+       unlock_super(sb);
+}
+

I'm thinking that for this particular problem, the solution isn't
quite appropriate.  We don't really give a toss about the dirty
state of the superblock.  We want to sync the darn filesystem.
Sure, we can do that in ->commit_super(), but it really has nothing
to do with the superblock.

What about something along these lines?


--- 25/fs/buffer.c~sync-fs	Mon Dec  2 16:00:18 2002
+++ 25-akpm/fs/buffer.c	Mon Dec  2 16:02:37 2002
@@ -213,7 +213,7 @@ EXPORT_SYMBOL(sync_blockdev);
  * superblock.  Filesystem data as well as the underlying block
  * device.  Takes the superblock lock.
  */
-int fsync_super(struct super_block *sb)
+static int __sync_filesystem(struct super_block *sb)
 {
 	sync_inodes_sb(sb, 0);
 	DQUOT_SYNC(sb);
@@ -225,6 +225,13 @@ int fsync_super(struct super_block *sb)
 	sync_inodes_sb(sb, 1);
 
 	return sync_blockdev(sb->s_bdev);
+}
+
+int sync_filesystem(struct super_block *sb)
+{
+	if (sb->s_op && sb->s_op->sync_filesystem)
+		return sb->s_op->sync_filesystem(sb);
+	return __sync_filesytem(sb);
 }
 
 /*

_