[PATCH 0/12] make ->sync_fs mandatory

[PATCH 0/12] make ->sync_fs mandatory

[Christoph Hellwig]

Currently the superblock methods are a bit confusing.  There is the
newer ->sync_fs method which is unconditionally called from
__sync_filesystem for data integrity writebacks.  And there is the older
->write_super which is called from pdflush for the periodic superblock
writeback, but also from __sync_filesystem just before calling
->sync_fs. (And still from file_fsync, but that is about to go away).

This series makes sure all filesystems that need superblock writeouts
define a ->sync_fs so we can stop calling ->write_super for the data
integrity writeback.  This means the presence of ->write_super indicates
a filesystem does want periodic superblock writeback and can implement
these independently from the data integrity writeback. It also means
we don't need to bother with possible s_dirt races for the data
integrity syncs.

This patch series first adds ->sync_fs methods to all filesystems having
->write_super, often refactoring the code in that are (and also
preparing the ->write_super instances for moving MS_RDONLY checking into
the caller), then it makes sure the existing ->sync_fs instaces cover
the work previously done in ->write_super for the two cases where they
differed widely, and lastly removes the call to ->write_super from
__sync_filesystem.

[PATCH 1/12] affs: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs.  Factor out common code
between affs_put_super, affs_write_super and the new affs_sync_fs into
a helper.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/affs/super.c
===================================================================
--- vfs-2.6.orig/fs/affs/super.c	2009-06-07 20:34:18.534814529 +0200
+++ vfs-2.6/fs/affs/super.c	2009-06-07 21:20:51.308814840 +0200
@@ -25,6 +25,19 @@ static int affs_statfs(struct dentry *de
 static int affs_remount (struct super_block *sb, int *flags, char *data);
 
 static void
+affs_commit_super(struct super_block *sb, int clean)
+{
+	struct affs_sb_info *sbi = AFFS_SB(sb);
+	struct buffer_head *bh = sbi->s_root_bh;
+	struct affs_root_tail *tail = AFFS_ROOT_TAIL(sb, bh);
+
+	tail->bm_flag = cpu_to_be32(clean);
+	secs_to_datestamp(get_seconds(), &tail->disk_change);
+	affs_fix_checksum(sb, bh);
+	mark_buffer_dirty(bh);
+}
+
+static void
 affs_put_super(struct super_block *sb)
 {
 	struct affs_sb_info *sbi = AFFS_SB(sb);
@@ -32,13 +45,8 @@ affs_put_super(struct super_block *sb)
 
 	lock_kernel();
 
-	if (!(sb->s_flags & MS_RDONLY)) {
-		AFFS_ROOT_TAIL(sb, sbi->s_root_bh)->bm_flag = cpu_to_be32(1);
-		secs_to_datestamp(get_seconds(),
-				  &AFFS_ROOT_TAIL(sb, sbi->s_root_bh)->disk_change);
-		affs_fix_checksum(sb, sbi->s_root_bh);
-		mark_buffer_dirty(sbi->s_root_bh);
-	}
+	if (!(sb->s_flags & MS_RDONLY))
+		affs_commit_super(sb, 1);
 
 	kfree(sbi->s_prefix);
 	affs_free_bitmap(sb);
@@ -53,18 +61,13 @@ static void
 affs_write_super(struct super_block *sb)
 {
 	int clean = 2;
-	struct affs_sb_info *sbi = AFFS_SB(sb);
 
 	lock_super(sb);
 	if (!(sb->s_flags & MS_RDONLY)) {
 		//	if (sbi->s_bitmap[i].bm_bh) {
 		//		if (buffer_dirty(sbi->s_bitmap[i].bm_bh)) {
 		//			clean = 0;
-		AFFS_ROOT_TAIL(sb, sbi->s_root_bh)->bm_flag = cpu_to_be32(clean);
-		secs_to_datestamp(get_seconds(),
-				  &AFFS_ROOT_TAIL(sb, sbi->s_root_bh)->disk_change);
-		affs_fix_checksum(sb, sbi->s_root_bh);
-		mark_buffer_dirty(sbi->s_root_bh);
+		affs_commit_super(sb, clean);
 		sb->s_dirt = !clean;	/* redo until bitmap synced */
 	} else
 		sb->s_dirt = 0;
@@ -73,6 +76,16 @@ affs_write_super(struct super_block *sb)
 	pr_debug("AFFS: write_super() at %lu, clean=%d\n", get_seconds(), clean);
 }
 
+static int
+affs_sync_fs(struct super_block *sb, int wait)
+{
+	lock_super(sb);
+	affs_commit_super(sb, 2);
+	sb->s_dirt = 0;
+	unlock_super(sb);
+	return 0;
+}
+
 static struct kmem_cache * affs_inode_cachep;
 
 static struct inode *affs_alloc_inode(struct super_block *sb)
@@ -130,6 +143,7 @@ static const struct super_operations aff
 	.clear_inode	= affs_clear_inode,
 	.put_super	= affs_put_super,
 	.write_super	= affs_write_super,
+	.sync_fs	= affs_sync_fs,
 	.statfs		= affs_statfs,
 	.remount_fs	= affs_remount,
 	.show_options	= generic_show_options,

[PATCH 2/12] bfs: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs, and reimplement
->write_super ontop of it.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/bfs/inode.c
===================================================================
--- vfs-2.6.orig/fs/bfs/inode.c	2009-06-05 15:54:45.060844037 +0200
+++ vfs-2.6/fs/bfs/inode.c	2009-06-05 15:58:45.082941409 +0200
@@ -210,6 +210,26 @@ static void bfs_delete_inode(struct inod
 	clear_inode(inode);
 }
 
+static int bfs_sync_fs(struct super_block *sb, int wait)
+{
+	struct bfs_sb_info *info = BFS_SB(sb);
+
+	mutex_lock(&info->bfs_lock);
+	mark_buffer_dirty(info->si_sbh);
+	sb->s_dirt = 0;
+	mutex_unlock(&info->bfs_lock);
+
+	return 0;
+}
+
+static void bfs_write_super(struct super_block *sb)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		bfs_sync_fs(sb, 1);
+	else
+		sb->s_dirt = 0;
+}
+
 static void bfs_put_super(struct super_block *s)
 {
 	struct bfs_sb_info *info = BFS_SB(s);
@@ -248,17 +268,6 @@ static int bfs_statfs(struct dentry *den
 	return 0;
 }
 
-static void bfs_write_super(struct super_block *s)
-{
-	struct bfs_sb_info *info = BFS_SB(s);
-
-	mutex_lock(&info->bfs_lock);
-	if (!(s->s_flags & MS_RDONLY))
-		mark_buffer_dirty(info->si_sbh);
-	s->s_dirt = 0;
-	mutex_unlock(&info->bfs_lock);
-}
-
 static struct kmem_cache *bfs_inode_cachep;
 
 static struct inode *bfs_alloc_inode(struct super_block *sb)
@@ -306,6 +315,7 @@ static const struct super_operations bfs
 	.delete_inode	= bfs_delete_inode,
 	.put_super	= bfs_put_super,
 	.write_super	= bfs_write_super,
+	.sync_fs	= bfs_sync_fs,
 	.statfs		= bfs_statfs,
 };
 

[PATCH 3/12] exofs: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs, and reimplement
->write_super ontop of it.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/exofs/super.c
===================================================================
--- vfs-2.6.orig/fs/exofs/super.c	2009-06-05 15:59:08.521843147 +0200
+++ vfs-2.6/fs/exofs/super.c	2009-06-05 16:02:20.687950326 +0200
@@ -200,18 +200,18 @@ static const struct export_operations ex
 /*
  * Write the superblock to the OSD
  */
-static void exofs_write_super(struct super_block *sb)
+static int exofs_sync_fs(struct super_block *sb, int wait)
 {
 	struct exofs_sb_info *sbi;
 	struct exofs_fscb *fscb;
 	struct osd_request *or;
 	struct osd_obj_id obj;
-	int ret;
+	int ret = -ENOMEM;
 
 	fscb = kzalloc(sizeof(struct exofs_fscb), GFP_KERNEL);
 	if (!fscb) {
 		EXOFS_ERR("exofs_write_super: memory allocation failed.\n");
-		return;
+		return -ENOMEM;
 	}
 
 	lock_super(sb);
@@ -249,6 +249,15 @@ out:
 	unlock_kernel();
 	unlock_super(sb);
 	kfree(fscb);
+	return ret;
+}
+
+static void exofs_write_super(struct super_block *sb)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		exofs_sync_fs(sb, 1);
+	else
+		sb->s_dirt = 0;
 }
 
 /*
@@ -493,6 +502,7 @@ static const struct super_operations exo
 	.delete_inode   = exofs_delete_inode,
 	.put_super      = exofs_put_super,
 	.write_super    = exofs_write_super,
+	.sync_fs	= exofs_sync_fs,
 	.statfs         = exofs_statfs,
 };
 

[PATCH 4/12] ext2: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs, and reimplement
->write_super ontop of it.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/ext2/super.c
===================================================================
--- vfs-2.6.orig/fs/ext2/super.c	2009-06-05 16:02:40.495959958 +0200
+++ vfs-2.6/fs/ext2/super.c	2009-06-05 16:10:15.579941946 +0200
@@ -42,6 +42,7 @@ static void ext2_sync_super(struct super
 			    struct ext2_super_block *es);
 static int ext2_remount (struct super_block * sb, int * flags, char * data);
 static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf);
+static int ext2_sync_fs(struct super_block *sb, int wait);
 
 void ext2_error (struct super_block * sb, const char * function,
 		 const char * fmt, ...)
@@ -309,6 +310,7 @@ static const struct super_operations ext
 	.delete_inode	= ext2_delete_inode,
 	.put_super	= ext2_put_super,
 	.write_super	= ext2_write_super,
+	.sync_fs	= ext2_sync_fs,
 	.statfs		= ext2_statfs,
 	.remount_fs	= ext2_remount,
 	.clear_inode	= ext2_clear_inode,
@@ -1131,25 +1133,36 @@ static void ext2_sync_super(struct super
  * set s_state to EXT2_VALID_FS after some corrections.
  */
 
-void ext2_write_super (struct super_block * sb)
+static int ext2_sync_fs(struct super_block *sb, int wait)
 {
-	struct ext2_super_block * es;
-	lock_kernel();
-	if (!(sb->s_flags & MS_RDONLY)) {
-		es = EXT2_SB(sb)->s_es;
+	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
 
-		if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
-			ext2_debug ("setting valid to 0\n");
-			es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
-			es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb));
-			es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb));
-			es->s_mtime = cpu_to_le32(get_seconds());
-			ext2_sync_super(sb, es);
-		} else
-			ext2_commit_super (sb, es);
+	lock_kernel();
+	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
+		ext2_debug("setting valid to 0\n");
+		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
+		es->s_free_blocks_count =
+			cpu_to_le32(ext2_count_free_blocks(sb));
+		es->s_free_inodes_count =
+			cpu_to_le32(ext2_count_free_inodes(sb));
+		es->s_mtime = cpu_to_le32(get_seconds());
+		ext2_sync_super(sb, es);
+	} else {
+		ext2_commit_super(sb, es);
 	}
 	sb->s_dirt = 0;
 	unlock_kernel();
+
+	return 0;
+}
+
+
+void ext2_write_super(struct super_block *sb)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		ext2_sync_fs(sb, 1);
+	else
+		sb->s_dirt = 0;
 }
 
 static int ext2_remount (struct super_block * sb, int * flags, char * data)

Re: [PATCH 0/12] make ->sync_fs mandatory

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2009-06-05 16:13:39.945939714 +0200
+++ vfs-2.6/fs/fat/inode.c	2009-06-05 16:15:30.220939883 +0200
@@ -449,6 +449,16 @@ static void fat_write_super(struct super
 	unlock_super(sb);
 }
 
+static int fat_sync_fs(struct super_block *sb, int wait)
+{
+	lock_super(sb);
+	fat_clusters_flush(sb);
+	sb->s_dirt = 0;
+	unlock_super(sb);
+
+	return 0;
+}
+
 static void fat_put_super(struct super_block *sb)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
@@ -641,6 +651,7 @@ static const struct super_operations fat
 	.delete_inode	= fat_delete_inode,
 	.put_super	= fat_put_super,
 	.write_super	= fat_write_super,
+	.sync_fs	= fat_sync_fs,
 	.statfs		= fat_statfs,
 	.clear_inode	= fat_clear_inode,
 	.remount_fs	= fat_remount,

[PATCH 6/12] hfs: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/hfs/super.c
===================================================================
--- vfs-2.6.orig/fs/hfs/super.c	2009-06-05 16:15:48.967939658 +0200
+++ vfs-2.6/fs/hfs/super.c	2009-06-05 16:16:53.655939519 +0200
@@ -58,6 +58,16 @@ static void hfs_write_super(struct super
 	unlock_super(sb);
 }
 
+static int hfs_sync_fs(struct super_block *sb, int wait)
+{
+	lock_super(sb);
+	hfs_mdb_commit(sb);
+	sb->s_dirt = 0;
+	unlock_super(sb);
+
+	return 0;
+}
+
 /*
  * hfs_put_super()
  *
@@ -172,6 +182,7 @@ static const struct super_operations hfs
 	.clear_inode	= hfs_clear_inode,
 	.put_super	= hfs_put_super,
 	.write_super	= hfs_write_super,
+	.sync_fs	= hfs_sync_fs,
 	.statfs		= hfs_statfs,
 	.remount_fs     = hfs_remount,
 	.show_options	= hfs_show_options,

[PATCH 7/12] hfsplus: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs, and reimplement
->write_super ontop of it.


Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: vfs-2.6/fs/hfsplus/super.c
===================================================================
--- vfs-2.6.orig/fs/hfsplus/super.c	2009-06-05 16:17:32.048939821 +0200
+++ vfs-2.6/fs/hfsplus/super.c	2009-06-05 16:19:06.525940458 +0200
@@ -152,7 +152,7 @@ static void hfsplus_clear_inode(struct i
 	}
 }
 
-static void hfsplus_write_super(struct super_block *sb)
+static int hfsplus_sync_fs(struct super_block *sb, int wait)
 {
 	struct hfsplus_vh *vhdr = HFSPLUS_SB(sb).s_vhdr;
 
@@ -160,9 +160,6 @@ static void hfsplus_write_super(struct s
 
 	lock_super(sb);
 	sb->s_dirt = 0;
-	if (sb->s_flags & MS_RDONLY)
-		/* warn? */
-		goto out;
 
 	vhdr->free_blocks = cpu_to_be32(HFSPLUS_SB(sb).free_blocks);
 	vhdr->next_alloc = cpu_to_be32(HFSPLUS_SB(sb).next_alloc);
@@ -194,8 +191,16 @@ static void hfsplus_write_super(struct s
 		}
 		HFSPLUS_SB(sb).flags &= ~HFSPLUS_SB_WRITEBACKUP;
 	}
- out:
 	unlock_super(sb);
+	return 0;
+}
+
+static void hfsplus_write_super(struct super_block *sb)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		hfsplus_sync_fs(sb, 1);
+	else
+		sb->s_dirt = 0;
 }
 
 static void hfsplus_put_super(struct super_block *sb)
@@ -290,6 +295,7 @@ static const struct super_operations hfs
 	.clear_inode	= hfsplus_clear_inode,
 	.put_super	= hfsplus_put_super,
 	.write_super	= hfsplus_write_super,
+	.sync_fs	= hfsplus_sync_fs,
 	.statfs		= hfsplus_statfs,
 	.remount_fs	= hfsplus_remount,
 	.show_options	= hfsplus_show_options,

[PATCH 8/12] sysv: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs, and reimplement
->write_super ontop of it.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/sysv/inode.c
===================================================================
--- vfs-2.6.orig/fs/sysv/inode.c	2009-06-05 16:25:55.677939770 +0200
+++ vfs-2.6/fs/sysv/inode.c	2009-06-05 16:27:30.132939652 +0200
@@ -31,16 +31,13 @@
 #include <asm/byteorder.h>
 #include "sysv.h"
 
-/* This is only called on sync() and umount(), when s_dirt=1. */
-static void sysv_write_super(struct super_block *sb)
+static int sysv_sync_fs(struct super_block *sb, int wait)
 {
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
 	unsigned long time = get_seconds(), old_time;
 
 	lock_super(sb);
 	lock_kernel();
-	if (sb->s_flags & MS_RDONLY)
-		goto clean;
 
 	/*
 	 * If we are going to write out the super block,
@@ -54,10 +51,19 @@ static void sysv_write_super(struct supe
 		*sbi->s_sb_time = cpu_to_fs32(sbi, time);
 		mark_buffer_dirty(sbi->s_bh2);
 	}
-clean:
-	sb->s_dirt = 0;
+
 	unlock_kernel();
 	unlock_super(sb);
+
+	return 0;
+}
+
+static void sysv_write_super(struct super_block *sb)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		sysv_sync_fs(sb, 1);
+	else
+		sb->s_dirt = 0;
 }
 
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
@@ -358,6 +364,7 @@ const struct super_operations sysv_sops 
 	.delete_inode	= sysv_delete_inode,
 	.put_super	= sysv_put_super,
 	.write_super	= sysv_write_super,
+	.sync_fs	= sysv_sync_fs,
 	.remount_fs	= sysv_remount,
 	.statfs		= sysv_statfs,
 };

[PATCH 9/12] ufs: add ->sync_fs

[Christoph Hellwig]

Add a ->sync_fs method for data integrity syncs, and reimplement
->write_super ontop of it.


Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: vfs-2.6/fs/ufs/super.c
===================================================================
--- vfs-2.6.orig/fs/ufs/super.c	2009-06-05 16:27:47.634965023 +0200
+++ vfs-2.6/fs/ufs/super.c	2009-06-05 16:29:38.547814343 +0200
@@ -1124,7 +1124,7 @@ failed_nomem:
 	return -ENOMEM;
 }
 
-static void ufs_write_super(struct super_block *sb)
+static int ufs_sync_fs(struct super_block *sb, int wait)
 {
 	struct ufs_sb_private_info * uspi;
 	struct ufs_super_block_first * usb1;
@@ -1133,25 +1133,36 @@ static void ufs_write_super(struct super
 
 	lock_super(sb);
 	lock_kernel();
+
 	UFSD("ENTER\n");
+
 	flags = UFS_SB(sb)->s_flags;
 	uspi = UFS_SB(sb)->s_uspi;
 	usb1 = ubh_get_usb_first(uspi);
 	usb3 = ubh_get_usb_third(uspi);
 
-	if (!(sb->s_flags & MS_RDONLY)) {
-		usb1->fs_time = cpu_to_fs32(sb, get_seconds());
-		if ((flags & UFS_ST_MASK) == UFS_ST_SUN 
-		  || (flags & UFS_ST_MASK) == UFS_ST_SUNOS
-		  || (flags & UFS_ST_MASK) == UFS_ST_SUNx86)
-			ufs_set_fs_state(sb, usb1, usb3,
-					UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time));
-		ufs_put_cstotal(sb);
-	}
+	usb1->fs_time = cpu_to_fs32(sb, get_seconds());
+	if ((flags & UFS_ST_MASK) == UFS_ST_SUN  ||
+	    (flags & UFS_ST_MASK) == UFS_ST_SUNOS ||
+	    (flags & UFS_ST_MASK) == UFS_ST_SUNx86)
+		ufs_set_fs_state(sb, usb1, usb3,
+				UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time));
+	ufs_put_cstotal(sb);
 	sb->s_dirt = 0;
+
 	UFSD("EXIT\n");
 	unlock_kernel();
 	unlock_super(sb);
+
+	return 0;
+}
+
+static void ufs_write_super(struct super_block *sb)
+{
+	if (!(sb->s_flags & MS_RDONLY))
+		ufs_sync_fs(sb, 1);
+	else
+		sb->s_dirt = 0;
 }
 
 static void ufs_put_super(struct super_block *sb)
@@ -1372,6 +1383,7 @@ static const struct super_operations ufs
 	.delete_inode	= ufs_delete_inode,
 	.put_super	= ufs_put_super,
 	.write_super	= ufs_write_super,
+	.sync_fs	= ufs_sync_fs,
 	.statfs		= ufs_statfs,
 	.remount_fs	= ufs_remount,
 	.show_options   = ufs_show_options,

[PATCH 10/12] jffs2: call jffs2_write_super from jffs2_sync_fs

[Christoph Hellwig]

The call to ->write_super from __sync_filesystem will go away, so make
sure jffs2 performs the same actions from inside ->sync_fs.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/jffs2/super.c
===================================================================
--- vfs-2.6.orig/fs/jffs2/super.c	2009-06-05 16:22:29.959939587 +0200
+++ vfs-2.6/fs/jffs2/super.c	2009-06-05 16:22:49.270946332 +0200
@@ -74,6 +74,8 @@ static int jffs2_sync_fs(struct super_bl
 {
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
 
+	jffs2_write_super(sb);
+
 	mutex_lock(&c->alloc_sem);
 	jffs2_flush_wbuf_pad(c);
 	mutex_unlock(&c->alloc_sem);

[PATCH 11/12] nilfs2: call nilfs2_write_super from nilfs2_sync_fs

[Christoph Hellwig]

The call to ->write_super from __sync_filesystem will go away, so make
sure nilfs2 performs the same actions from inside ->sync_fs.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/nilfs2/super.c
===================================================================
--- vfs-2.6.orig/fs/nilfs2/super.c	2009-06-05 16:23:27.776843986 +0200
+++ vfs-2.6/fs/nilfs2/super.c	2009-06-05 16:23:46.850942403 +0200
@@ -391,6 +391,8 @@ static int nilfs_sync_fs(struct super_bl
 {
 	int err = 0;
 
+	nilfs_write_super(sb);
+
 	/* This function is called when super block should be written back */
 	if (wait)
 		err = nilfs_construct_segment(sb);

[PATCH 12/12] remove the call to ->write_super in __sync_filesystem

[Christoph Hellwig]

Now that all filesystems provide ->sync_fs methods we can change
__sync_filesystem to only call ->sync_fs.

This gives us a clear separation between periodic writeouts which
are driven by ->write_super and data integrity syncs that go
through ->sync_fs. (modulo file_fsync which is also going away)


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/sync.c
===================================================================
--- vfs-2.6.orig/fs/sync.c	2009-06-05 16:29:58.691814461 +0200
+++ vfs-2.6/fs/sync.c	2009-06-05 16:31:13.702939647 +0200
@@ -33,8 +33,6 @@ static int __sync_filesystem(struct supe
 	else
 		sync_quota_sb(sb, -1);
 	sync_inodes_sb(sb, wait);
-	if (sb->s_dirt && sb->s_op->write_super)
-		sb->s_op->write_super(sb);
 	if (sb->s_op->sync_fs)
 		sb->s_op->sync_fs(sb, wait);
 	return __sync_blockdev(sb->s_bdev, wait);

Re: [PATCH 0/12] make ->sync_fs mandatory

[Al Viro]

On Mon, Jun 08, 2009 at 10:02:52AM +0200, Christoph Hellwig wrote:
> Currently the superblock methods are a bit confusing.  There is the
> newer ->sync_fs method which is unconditionally called from
> __sync_filesystem for data integrity writebacks.  And there is the older
> ->write_super which is called from pdflush for the periodic superblock
> writeback, but also from __sync_filesystem just before calling
> ->sync_fs. (And still from file_fsync, but that is about to go away).
> 
> This series makes sure all filesystems that need superblock writeouts
> define a ->sync_fs so we can stop calling ->write_super for the data
> integrity writeback.  This means the presence of ->write_super indicates
> a filesystem does want periodic superblock writeback and can implement
> these independently from the data integrity writeback. It also means
> we don't need to bother with possible s_dirt races for the data
> integrity syncs.
> 
> This patch series first adds ->sync_fs methods to all filesystems having
> ->write_super, often refactoring the code in that are (and also
> preparing the ->write_super instances for moving MS_RDONLY checking into
> the caller), then it makes sure the existing ->sync_fs instaces cover
> the work previously done in ->write_super for the two cases where they
> differed widely, and lastly removes the call to ->write_super from
> __sync_filesystem.

Applied.  FWIW, file_fsync() is nearly gone; the last remnants are exofs,
hfs and hfsplus.  Is there any sane way to keep track of dirty metadata
blocks there?  Or is "just flush all dirty buffer_head for the device"
really the best we can do?

Re: [PATCH 0/12] make ->sync_fs mandatory

[Boaz Harrosh]

On 06/08/2009 05:45 PM, Al Viro wrote:
> On Mon, Jun 08, 2009 at 10:02:52AM +0200, Christoph Hellwig wrote:
>> Currently the superblock methods are a bit confusing.  There is the
>> newer ->sync_fs method which is unconditionally called from
>> __sync_filesystem for data integrity writebacks.  And there is the older
>> ->write_super which is called from pdflush for the periodic superblock
>> writeback, but also from __sync_filesystem just before calling
>> ->sync_fs. (And still from file_fsync, but that is about to go away).
>>
>> This series makes sure all filesystems that need superblock writeouts
>> define a ->sync_fs so we can stop calling ->write_super for the data
>> integrity writeback.  This means the presence of ->write_super indicates
>> a filesystem does want periodic superblock writeback and can implement
>> these independently from the data integrity writeback. It also means
>> we don't need to bother with possible s_dirt races for the data
>> integrity syncs.
>>
>> This patch series first adds ->sync_fs methods to all filesystems having
>> ->write_super, often refactoring the code in that are (and also
>> preparing the ->write_super instances for moving MS_RDONLY checking into
>> the caller), then it makes sure the existing ->sync_fs instaces cover
>> the work previously done in ->write_super for the two cases where they
>> differed widely, and lastly removes the call to ->write_super from
>> __sync_filesystem.
> 
> Applied.  FWIW, file_fsync() is nearly gone; the last remnants are exofs,
> hfs and hfsplus.  Is there any sane way to keep track of dirty metadata
> blocks there?  Or is "just flush all dirty buffer_head for the device"
> really the best we can do?

Hi Al

Below is just an RFC for review concerning the file_fsync() use. I've
open coded the parts of file_fsync() that are actually needed. Note
the TODO as the sb update is only needed for new files, I'll farther
look into it later.

The below patch is no good because it's based on an old tree. The lock_super
was moved to inside write_super right? and also I should call the new
exofs_sync_fs() directly. Please just give me the OK that this is the
idea here and I'll prepare the right patch.

What is the most uptodate tree that has all these changes? so I can cut
the patches over? (Sorry for the novice-ness)
---
[RFC] exofs: Avoid using file_fsync()

The use of file_fsync() in exofs_file_sync() is not necessary since it
does some extra stuff not used by exofs. Open code just the parts that
are currently needed.

TODO: Farther optimization can be done to sync the sb only on inode update
of new files, Usually the sb update is not needed in exofs.

Boaz
---
git diff --stat -p fs/exofs/file.c
 fs/exofs/file.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/exofs/file.c b/fs/exofs/file.c
index 6ed7fe4..9e60d68 100644
--- a/fs/exofs/file.c
+++ b/fs/exofs/file.c
@@ -47,16 +47,24 @@ static int exofs_file_fsync(struct file *filp, struct dentry *dentry,
 {
 	int ret;
 	struct address_space *mapping = filp->f_mapping;
+	struct inode * inode = dentry->d_inode;
+	struct super_block * sb;
 
 	ret = filemap_write_and_wait(mapping);
 	if (ret)
 		return ret;
 
-	/*Note: file_fsync below also calles sync_blockdev, which is a no-op
-	 *      for exofs, but other then that it does sync_inode and
-	 *      sync_superblock which is what we need here.
-	 */
-	return file_fsync(filp, dentry, datasync);
+	/* sync the inode attributes */
+	ret = write_inode_now(inode, 0);
+
+	/* This is a good place to write the sb */
+	/* TODO: Schedule an sb-sync on create */
+	sb = inode->i_sb;
+	lock_super(sb);
+	if (sb->s_dirt && sb->s_op->write_super)
+		sb->s_op->write_super(sb);
+	unlock_super(sb);
+	return ret;
 }
 
 static int exofs_flush(struct file *file, fl_owner_t id)

Re: [PATCH 3/12] exofs: add ->sync_fs

[Boaz Harrosh]

On 06/08/2009 11:03 AM, Christoph Hellwig wrote:
> Add a ->sync_fs method for data integrity syncs, and reimplement
> ->write_super ontop of it.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks 

> 
> Index: vfs-2.6/fs/exofs/super.c
> ===================================================================
> --- vfs-2.6.orig/fs/exofs/super.c	2009-06-05 15:59:08.521843147 +0200
> +++ vfs-2.6/fs/exofs/super.c	2009-06-05 16:02:20.687950326 +0200
> @@ -200,18 +200,18 @@ static const struct export_operations ex
>  /*
>   * Write the superblock to the OSD
>   */
> -static void exofs_write_super(struct super_block *sb)
> +static int exofs_sync_fs(struct super_block *sb, int wait)
>  {
>  	struct exofs_sb_info *sbi;
>  	struct exofs_fscb *fscb;
>  	struct osd_request *or;
>  	struct osd_obj_id obj;
> -	int ret;
> +	int ret = -ENOMEM;
>  
>  	fscb = kzalloc(sizeof(struct exofs_fscb), GFP_KERNEL);
>  	if (!fscb) {
>  		EXOFS_ERR("exofs_write_super: memory allocation failed.\n");
> -		return;
> +		return -ENOMEM;
>  	}
>  
>  	lock_super(sb);
> @@ -249,6 +249,15 @@ out:
>  	unlock_kernel();
>  	unlock_super(sb);
>  	kfree(fscb);
> +	return ret;
> +}
> +
> +static void exofs_write_super(struct super_block *sb)
> +{
> +	if (!(sb->s_flags & MS_RDONLY))
> +		exofs_sync_fs(sb, 1);
> +	else
> +		sb->s_dirt = 0;

I never tested read-only mounts. Unlike ext3 (Journals)
Read-only mount should also work with:

		BUG_ON(sb->s_dirt)

As the actual journal is on the OSD targets, and is abstracted
away form the file system.

>  }
>  
>  /*
> @@ -493,6 +502,7 @@ static const struct super_operations exo
>  	.delete_inode   = exofs_delete_inode,
>  	.put_super      = exofs_put_super,
>  	.write_super    = exofs_write_super,
> +	.sync_fs	= exofs_sync_fs,
>  	.statfs         = exofs_statfs,
>  };
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html