[PATCH 1/2] push MS_RDONLY check from ->write_super into caller

[PATCH 1/2] push MS_RDONLY check from ->write_super into caller

[Christoph Hellwig]

Add a central MS_RDONLY to sync_super instead of having it in the ->write_super
methods (and even that not consistently).  The other two callers are already
protected:  sync_filesystems has the same s_umount protected MS_RDONLY check
and file_fsync can only be called on a writeable file descriptor.

Also make sure to clear s_dirt if it was set on a read-only superblock to
avoid calling into these filesystems again and again.

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-05-06 21:12:59.419666674 +0200
+++ vfs-2.6/fs/affs/super.c	2009-05-06 21:21:06.586663246 +0200
@@ -54,18 +54,15 @@ affs_write_super(struct super_block *sb)
 	int clean = 2;
 	struct affs_sb_info *sbi = AFFS_SB(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);
-		sb->s_dirt = !clean;	/* redo until bitmap synced */
-	} else
-		sb->s_dirt = 0;
+	//	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);
+	sb->s_dirt = !clean;	/* redo until bitmap synced */
 
 	pr_debug("AFFS: write_super() at %lu, clean=%d\n", get_seconds(), clean);
 }
Index: vfs-2.6/fs/bfs/inode.c
===================================================================
--- vfs-2.6.orig/fs/bfs/inode.c	2009-05-06 21:12:59.432658761 +0200
+++ vfs-2.6/fs/bfs/inode.c	2009-05-06 21:14:22.613661552 +0200
@@ -219,7 +219,7 @@ static void bfs_put_super(struct super_b
 
 	lock_kernel();
 
-	if (s->s_dirt)
+	if (s->s_dirt && !(s->s_flags & MS_RDONLY))
 		bfs_write_super(s);
 
 	brelse(info->si_sbh);
@@ -253,8 +253,7 @@ static void bfs_write_super(struct super
 	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);
+	mark_buffer_dirty(info->si_sbh);
 	s->s_dirt = 0;
 	mutex_unlock(&info->bfs_lock);
 }
Index: vfs-2.6/fs/ext2/super.c
===================================================================
--- vfs-2.6.orig/fs/ext2/super.c	2009-05-06 21:12:59.465658841 +0200
+++ vfs-2.6/fs/ext2/super.c	2009-05-06 21:15:55.284661659 +0200
@@ -116,7 +116,7 @@ static void ext2_put_super (struct super
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		ext2_write_super(sb);
 
 	ext2_xattr_put_super(sb);
@@ -1135,19 +1135,18 @@ void ext2_write_super (struct super_bloc
 {
 	struct ext2_super_block * es;
 	lock_kernel();
-	if (!(sb->s_flags & MS_RDONLY)) {
-		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);
-	}
+	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);
 	sb->s_dirt = 0;
 	unlock_kernel();
 }
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2009-05-06 21:12:59.548659280 +0200
+++ vfs-2.6/fs/fat/inode.c	2009-05-06 21:16:42.422661350 +0200
@@ -442,9 +442,7 @@ static void fat_clear_inode(struct inode
 static void fat_write_super(struct super_block *sb)
 {
 	sb->s_dirt = 0;
-
-	if (!(sb->s_flags & MS_RDONLY))
-		fat_clusters_flush(sb);
+	fat_clusters_flush(sb);
 }
 
 static void fat_put_super(struct super_block *sb)
@@ -453,7 +451,7 @@ static void fat_put_super(struct super_b
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		fat_write_super(sb);
 
 	if (sbi->nls_disk) {
Index: vfs-2.6/fs/hfs/super.c
===================================================================
--- vfs-2.6.orig/fs/hfs/super.c	2009-05-06 21:12:59.562658897 +0200
+++ vfs-2.6/fs/hfs/super.c	2009-05-06 21:17:17.361661811 +0200
@@ -50,8 +50,7 @@ MODULE_LICENSE("GPL");
 static void hfs_write_super(struct super_block *sb)
 {
 	sb->s_dirt = 0;
-	if (sb->s_flags & MS_RDONLY)
-		return;
+
 	/* sync everything to the buffers */
 	hfs_mdb_commit(sb);
 }
@@ -67,7 +66,7 @@ static void hfs_put_super(struct super_b
 {
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		hfs_write_super(sb);
 	hfs_mdb_close(sb);
 	/* release the MDB's resources */
Index: vfs-2.6/fs/hfsplus/super.c
===================================================================
--- vfs-2.6.orig/fs/hfsplus/super.c	2009-05-06 21:12:59.611659198 +0200
+++ vfs-2.6/fs/hfsplus/super.c	2009-05-06 21:18:07.448661459 +0200
@@ -158,9 +158,6 @@ static void hfsplus_write_super(struct s
 
 	dprint(DBG_SUPER, "hfsplus_write_super\n");
 	sb->s_dirt = 0;
-	if (sb->s_flags & MS_RDONLY)
-		/* warn? */
-		return;
 
 	vhdr->free_blocks = cpu_to_be32(HFSPLUS_SB(sb).free_blocks);
 	vhdr->next_alloc = cpu_to_be32(HFSPLUS_SB(sb).next_alloc);
@@ -202,8 +199,9 @@ static void hfsplus_put_super(struct sup
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		hfsplus_write_super(sb);
+
 	if (!(sb->s_flags & MS_RDONLY) && HFSPLUS_SB(sb).s_vhdr) {
 		struct hfsplus_vh *vhdr = HFSPLUS_SB(sb).s_vhdr;
 
Index: vfs-2.6/fs/jffs2/fs.c
===================================================================
--- vfs-2.6.orig/fs/jffs2/fs.c	2009-05-06 21:18:29.645659579 +0200
+++ vfs-2.6/fs/jffs2/fs.c	2009-05-06 21:18:36.248661832 +0200
@@ -407,9 +407,6 @@ void jffs2_write_super (struct super_blo
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
 	sb->s_dirt = 0;
 
-	if (sb->s_flags & MS_RDONLY)
-		return;
-
 	D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
 	jffs2_garbage_collect_trigger(c);
 	jffs2_erase_pending_blocks(c, 0);
Index: vfs-2.6/fs/jffs2/super.c
===================================================================
--- vfs-2.6.orig/fs/jffs2/super.c	2009-05-06 21:12:59.669659039 +0200
+++ vfs-2.6/fs/jffs2/super.c	2009-05-06 21:18:47.434661682 +0200
@@ -176,7 +176,7 @@ static void jffs2_put_super (struct supe
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt&& !(sb->s_flags & MS_RDONLY))
 		jffs2_write_super(sb);
 
 	mutex_lock(&c->alloc_sem);
Index: vfs-2.6/fs/nilfs2/super.c
===================================================================
--- vfs-2.6.orig/fs/nilfs2/super.c	2009-05-06 21:12:59.706659768 +0200
+++ vfs-2.6/fs/nilfs2/super.c	2009-05-06 21:19:52.668661692 +0200
@@ -318,7 +318,7 @@ static void nilfs_put_super(struct super
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		nilfs_write_super(sb);
 
 	nilfs_detach_segment_constructor(sbi);
@@ -368,21 +368,21 @@ static void nilfs_write_super(struct sup
 {
 	struct nilfs_sb_info *sbi = NILFS_SB(sb);
 	struct the_nilfs *nilfs = sbi->s_nilfs;
+	struct nilfs_super_block **sbp = nilfs->ns_sbp;
+	int dupsb;
+	u64 t;
 
 	down_write(&nilfs->ns_sem);
-	if (!(sb->s_flags & MS_RDONLY)) {
-		struct nilfs_super_block **sbp = nilfs->ns_sbp;
-		u64 t = get_seconds();
-		int dupsb;
-
-		if (!nilfs_discontinued(nilfs) && t >= nilfs->ns_sbwtime[0] &&
-		    t < nilfs->ns_sbwtime[0] + NILFS_SB_FREQ) {
-			up_write(&nilfs->ns_sem);
-			return;
-		}
-		dupsb = sbp[1] && t > nilfs->ns_sbwtime[1] + NILFS_ALTSB_FREQ;
-		nilfs_commit_super(sbi, dupsb);
+	t = get_seconds();
+
+	if (!nilfs_discontinued(nilfs) && t >= nilfs->ns_sbwtime[0] &&
+	    t < nilfs->ns_sbwtime[0] + NILFS_SB_FREQ) {
+		up_write(&nilfs->ns_sem);
+		return;
 	}
+	dupsb = sbp[1] && t > nilfs->ns_sbwtime[1] + NILFS_ALTSB_FREQ;
+	nilfs_commit_super(sbi, dupsb);
+
 	sb->s_dirt = 0;
 	up_write(&nilfs->ns_sem);
 }
Index: vfs-2.6/fs/super.c
===================================================================
--- vfs-2.6.orig/fs/super.c	2009-05-06 21:12:18.139659181 +0200
+++ vfs-2.6/fs/super.c	2009-05-06 21:24:41.541661530 +0200
@@ -422,8 +422,20 @@ restart:
 
 			down_read(&sb->s_umount);
 			lock_super(sb);
-			if (sb->s_root && sb->s_dirt)
-				sb->s_op->write_super(sb);
+			if (sb->s_root) {
+				/*
+				 * Avoid loops if a filesystem left s_dirt
+				 * set after remount r/o.
+				 *
+				 * Might be worth an audit that it always gets
+				 * cleared and then be turned into WARN_ON.
+				 */
+				if (sb->s_flags & MS_RDONLY)
+					sb->s_dirt = 0;
+
+				if (sb->s_dirt)
+					sb->s_op->write_super(sb);
+			}
 			unlock_super(sb);
 			up_read(&sb->s_umount);
 
Index: vfs-2.6/fs/sysv/inode.c
===================================================================
--- vfs-2.6.orig/fs/sysv/inode.c	2009-05-06 21:12:59.891659572 +0200
+++ vfs-2.6/fs/sysv/inode.c	2009-05-06 21:20:24.210661655 +0200
@@ -38,9 +38,6 @@ static void sysv_write_super(struct supe
 	unsigned long time = get_seconds(), old_time;
 
 	lock_kernel();
-	if (sb->s_flags & MS_RDONLY)
-		goto clean;
-
 	/*
 	 * If we are going to write out the super block,
 	 * then attach current time stamp.
@@ -53,7 +50,7 @@ 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();
 }
@@ -76,7 +73,7 @@ static void sysv_put_super(struct super_
 
 	lock_kernel();
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		sysv_write_super(sb);
 
 	if (!(sb->s_flags & MS_RDONLY)) {
Index: vfs-2.6/fs/ufs/super.c
===================================================================
--- vfs-2.6.orig/fs/ufs/super.c	2009-05-06 21:12:59.952659235 +0200
+++ vfs-2.6/fs/ufs/super.c	2009-05-06 21:21:04.195661283 +0200
@@ -1138,15 +1138,14 @@ static void ufs_write_super(struct super
 	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();
@@ -1158,7 +1157,7 @@ static void ufs_put_super(struct super_b
 		
 	UFSD("ENTER\n");
 
-	if (sb->s_dirt)
+	if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
 		ufs_write_super(sb);
 
 	if (!(sb->s_flags & MS_RDONLY))

Re: [PATCH 1/2] push MS_RDONLY check from ->write_super into caller

[Al Viro]

On Wed, May 06, 2009 at 10:16:05PM +0200, Christoph Hellwig wrote:
> Add a central MS_RDONLY to sync_super instead of having it in the ->write_super
> methods (and even that not consistently).  The other two callers are already
> protected:  sync_filesystems has the same s_umount protected MS_RDONLY check
> and file_fsync can only be called on a writeable file descriptor.
> 
> Also make sure to clear s_dirt if it was set on a read-only superblock to
> avoid calling into these filesystems again and again.

> Index: vfs-2.6/fs/affs/super.c
> Index: vfs-2.6/fs/bfs/inode.c
> Index: vfs-2.6/fs/ext2/super.c
> Index: vfs-2.6/fs/fat/inode.c
> Index: vfs-2.6/fs/hfs/super.c
> Index: vfs-2.6/fs/hfsplus/super.c
> Index: vfs-2.6/fs/jffs2/fs.c
> Index: vfs-2.6/fs/jffs2/super.c
> Index: vfs-2.6/fs/nilfs2/super.c
> Index: vfs-2.6/fs/super.c
> Index: vfs-2.6/fs/sysv/inode.c
> Index: vfs-2.6/fs/ufs/super.c

You've missed xfs.  Added and applied (reiserfs, ext4 and exofs are also
not touched, but they don't need a change here).

Re: [PATCH 1/2] push MS_RDONLY check from ->write_super into caller

[Al Viro]

On Wed, May 06, 2009 at 11:19:12PM +0100, Al Viro wrote:
> On Wed, May 06, 2009 at 10:16:05PM +0200, Christoph Hellwig wrote:
> > Add a central MS_RDONLY to sync_super instead of having it in the ->write_super
> > methods (and even that not consistently).  The other two callers are already
> > protected:  sync_filesystems has the same s_umount protected MS_RDONLY check
> > and file_fsync can only be called on a writeable file descriptor.
> > 
> > Also make sure to clear s_dirt if it was set on a read-only superblock to
> > avoid calling into these filesystems again and again.
> 
> > Index: vfs-2.6/fs/affs/super.c
> > Index: vfs-2.6/fs/bfs/inode.c
> > Index: vfs-2.6/fs/ext2/super.c
> > Index: vfs-2.6/fs/fat/inode.c
> > Index: vfs-2.6/fs/hfs/super.c
> > Index: vfs-2.6/fs/hfsplus/super.c
> > Index: vfs-2.6/fs/jffs2/fs.c
> > Index: vfs-2.6/fs/jffs2/super.c
> > Index: vfs-2.6/fs/nilfs2/super.c
> > Index: vfs-2.6/fs/super.c
> > Index: vfs-2.6/fs/sysv/inode.c
> > Index: vfs-2.6/fs/ufs/super.c
> 
> You've missed xfs.  Added and applied (reiserfs, ext4 and exofs are also
> not touched, but they don't need a change here).

Eh...  Actually, this is wrong as described.  ->fsync() *can* be called
for file opened r/o.  Moreover, fs might decide to go r/o on its own.
file_fsync() is not widely used, but users include affs, bfs, fat, hfs,
hfsplus and ufs.  No go, at least as long as file_fsync() is used for
those.

Patch dropped until we decide what to do with those.  It shouldn't be
all that hard to push file_fsync() out of existence, so it might be
doable, but...

Re: [PATCH 1/2] push MS_RDONLY check from ->write_super into caller

[Christoph Hellwig]

On Wed, May 06, 2009 at 11:19:12PM +0100, Al Viro wrote:
> You've missed xfs.  Added and applied (reiserfs, ext4 and exofs are also
> not touched, but they don't need a change here).

xfs_write_super is gone in my tree, but that change will be pushed
through the xfs tree.