[PATCH[RFC] kill sysrq-u (emergency remount r/o)

[PATCH[RFC] kill sysrq-u (emergency remount r/o)

[Christoph Hellwig]

Hi there,

in two recent discussions (file_list_lock scalability and remount r/o
on suspend) I stumbled over this emergency remount feature.  It's not
actually useful because it tries a potentially dangerous remount
despite writers still beeing in progress, which we can't get rid.

I've attached one patch in this mail that simply kills the
functionality, and in a reply to this mail I'll send a second one
that keeps the sysrq functionality, but removes the force argument
from do_remount_sb that overrides the still busy check.  This version
is currently not useful, but makes a lot of sense once Dave Hansens
per-mountpoint r/o patches get in, as we can check for a real write
in progress instead of simply a file opened with write permission.

Any ideas and comments?


Index: linux-2.6/drivers/char/sysrq.c
===================================================================
--- linux-2.6.orig/drivers/char/sysrq.c	2007-02-05 18:05:11.000000000 +0100
+++ linux-2.6/drivers/char/sysrq.c	2007-02-05 18:12:47.000000000 +0100
@@ -159,17 +159,6 @@
 	.enable_mask	= SYSRQ_ENABLE_SYNC,
 };
 
-static void sysrq_handle_mountro(int key, struct tty_struct *tty)
-{
-	emergency_remount();
-}
-static struct sysrq_key_op sysrq_mountro_op = {
-	.handler	= sysrq_handle_mountro,
-	.help_msg	= "Unmount",
-	.action_msg	= "Emergency Remount R/O",
-	.enable_mask	= SYSRQ_ENABLE_REMOUNT,
-};
-
 #ifdef CONFIG_LOCKDEP
 static void sysrq_handle_showlocks(int key, struct tty_struct *tty)
 {
@@ -340,7 +329,7 @@
 	&sysrq_unraw_op,		/* r */
 	&sysrq_sync_op,			/* s */
 	&sysrq_showstate_op,		/* t */
-	&sysrq_mountro_op,		/* u */
+	NULL,				/* u */
 	/* v: May be registered at init time by SMP VOYAGER */
 	NULL,				/* v */
 	&sysrq_showstate_blocked_op,	/* w */
Index: linux-2.6/fs/namespace.c
===================================================================
--- linux-2.6.orig/fs/namespace.c	2007-02-05 18:03:23.000000000 +0100
+++ linux-2.6/fs/namespace.c	2007-02-05 18:04:24.000000000 +0100
@@ -597,7 +597,7 @@
 		if (!(sb->s_flags & MS_RDONLY)) {
 			lock_kernel();
 			DQUOT_OFF(sb);
-			retval = do_remount_sb(sb, MS_RDONLY, NULL, 0);
+			retval = do_remount_sb(sb, MS_RDONLY, NULL);
 			unlock_kernel();
 		}
 		up_write(&sb->s_umount);
@@ -964,7 +964,7 @@
 		return -EINVAL;
 
 	down_write(&sb->s_umount);
-	err = do_remount_sb(sb, flags, data, 0);
+	err = do_remount_sb(sb, flags, data);
 	if (!err)
 		nd->mnt->mnt_flags = mnt_flags;
 	up_write(&sb->s_umount);
Index: linux-2.6/fs/super.c
===================================================================
--- linux-2.6.orig/fs/super.c	2007-02-05 18:03:23.000000000 +0100
+++ linux-2.6/fs/super.c	2007-02-05 18:04:54.000000000 +0100
@@ -557,35 +557,14 @@
 }
 
 /**
- *	mark_files_ro
- *	@sb: superblock in question
- *
- *	All files are marked read/only.  We don't care about pending
- *	delete files so this should be used in 'force' mode only
- */
-
-static void mark_files_ro(struct super_block *sb)
-{
-	struct file *f;
-
-	file_list_lock();
-	list_for_each_entry(f, &sb->s_files, f_u.fu_list) {
-		if (S_ISREG(f->f_path.dentry->d_inode->i_mode) && file_count(f))
-			f->f_mode &= ~FMODE_WRITE;
-	}
-	file_list_unlock();
-}
-
-/**
  *	do_remount_sb - asks filesystem to change mount options.
  *	@sb:	superblock in question
  *	@flags:	numeric part of options
  *	@data:	the rest of options
- *      @force: whether or not to force the change
  *
  *	Alters the mount options of a mounted file system.
  */
-int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
+int do_remount_sb(struct super_block *sb, int flags, void *data)
 {
 	int retval;
 	
@@ -601,9 +580,7 @@
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
 	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
-		if (force)
-			mark_files_ro(sb);
-		else if (!fs_may_remount_ro(sb))
+		if (!fs_may_remount_ro(sb))
 			return -EBUSY;
 	}
 
@@ -618,37 +595,6 @@
 	return 0;
 }
 
-static void do_emergency_remount(unsigned long foo)
-{
-	struct super_block *sb;
-
-	spin_lock(&sb_lock);
-	list_for_each_entry(sb, &super_blocks, s_list) {
-		sb->s_count++;
-		spin_unlock(&sb_lock);
-		down_read(&sb->s_umount);
-		if (sb->s_root && sb->s_bdev && !(sb->s_flags & MS_RDONLY)) {
-			/*
-			 * ->remount_fs needs lock_kernel().
-			 *
-			 * What lock protects sb->s_flags??
-			 */
-			lock_kernel();
-			do_remount_sb(sb, MS_RDONLY, NULL, 1);
-			unlock_kernel();
-		}
-		drop_super(sb);
-		spin_lock(&sb_lock);
-	}
-	spin_unlock(&sb_lock);
-	printk("Emergency Remount complete\n");
-}
-
-void emergency_remount(void)
-{
-	pdflush_operation(do_emergency_remount, 0);
-}
-
 /*
  * Unnamed block devices are dummy devices used by virtual
  * filesystems which don't use real block-devices.  -- jrs
@@ -861,7 +807,7 @@
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	do_remount_sb(s, flags, data, 0);
+	do_remount_sb(s, flags, data);
 	return simple_set_mnt(mnt, s);
 }
 
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2007-02-05 18:03:23.000000000 +0100
+++ linux-2.6/include/linux/fs.h	2007-02-05 18:05:16.000000000 +0100
@@ -1599,9 +1599,7 @@
 extern void sync_filesystems(int wait);
 extern void __fsync_super(struct super_block *sb);
 extern void emergency_sync(void);
-extern void emergency_remount(void);
-extern int do_remount_sb(struct super_block *sb, int flags,
-			 void *data, int force);
+extern int do_remount_sb(struct super_block *sb, int flags, void *data);
 #ifdef CONFIG_BLOCK
 extern sector_t bmap(struct inode *, sector_t);
 #endif

[PATCH[RFC] don't force remount in sysrq-u (emergency remount r/o)

[Christoph Hellwig]

On Mon, Feb 05, 2007 at 06:32:47PM +0100, Christoph Hellwig wrote:
> Hi there,
> 
> in two recent discussions (file_list_lock scalability and remount r/o
> on suspend) I stumbled over this emergency remount feature.  It's not
> actually useful because it tries a potentially dangerous remount
> despite writers still beeing in progress, which we can't get rid.
> 
> I've attached one patch in this mail that simply kills the
> functionality, and in a reply to this mail I'll send a second one
> that keeps the sysrq functionality, but removes the force argument
> from do_remount_sb that overrides the still busy check.  This version
> is currently not useful, but makes a lot of sense once Dave Hansens
> per-mountpoint r/o patches get in, as we can check for a real write
> in progress instead of simply a file opened with write permission.

And here is the alternate patch that simply removes the forced remount
r/o hack:


Index: linux-2.6/fs/namespace.c
===================================================================
--- linux-2.6.orig/fs/namespace.c	2007-02-05 18:21:03.000000000 +0100
+++ linux-2.6/fs/namespace.c	2007-02-05 18:21:18.000000000 +0100
@@ -597,7 +597,7 @@
 		if (!(sb->s_flags & MS_RDONLY)) {
 			lock_kernel();
 			DQUOT_OFF(sb);
-			retval = do_remount_sb(sb, MS_RDONLY, NULL, 0);
+			retval = do_remount_sb(sb, MS_RDONLY, NULL);
 			unlock_kernel();
 		}
 		up_write(&sb->s_umount);
@@ -964,7 +964,7 @@
 		return -EINVAL;
 
 	down_write(&sb->s_umount);
-	err = do_remount_sb(sb, flags, data, 0);
+	err = do_remount_sb(sb, flags, data);
 	if (!err)
 		nd->mnt->mnt_flags = mnt_flags;
 	up_write(&sb->s_umount);
Index: linux-2.6/fs/super.c
===================================================================
--- linux-2.6.orig/fs/super.c	2007-02-05 18:21:03.000000000 +0100
+++ linux-2.6/fs/super.c	2007-02-05 18:21:48.000000000 +0100
@@ -557,26 +557,6 @@
 }
 
 /**
- *	mark_files_ro
- *	@sb: superblock in question
- *
- *	All files are marked read/only.  We don't care about pending
- *	delete files so this should be used in 'force' mode only
- */
-
-static void mark_files_ro(struct super_block *sb)
-{
-	struct file *f;
-
-	file_list_lock();
-	list_for_each_entry(f, &sb->s_files, f_u.fu_list) {
-		if (S_ISREG(f->f_path.dentry->d_inode->i_mode) && file_count(f))
-			f->f_mode &= ~FMODE_WRITE;
-	}
-	file_list_unlock();
-}
-
-/**
  *	do_remount_sb - asks filesystem to change mount options.
  *	@sb:	superblock in question
  *	@flags:	numeric part of options
@@ -585,7 +565,7 @@
  *
  *	Alters the mount options of a mounted file system.
  */
-int do_remount_sb(struct super_block *sb, int flags, void *data, int force)
+int do_remount_sb(struct super_block *sb, int flags, void *data)
 {
 	int retval;
 	
@@ -601,9 +581,7 @@
 	/* If we are remounting RDONLY and current sb is read/write,
 	   make sure there are no rw files opened */
 	if ((flags & MS_RDONLY) && !(sb->s_flags & MS_RDONLY)) {
-		if (force)
-			mark_files_ro(sb);
-		else if (!fs_may_remount_ro(sb))
+		if (!fs_may_remount_ro(sb))
 			return -EBUSY;
 	}
 
@@ -634,7 +612,7 @@
 			 * What lock protects sb->s_flags??
 			 */
 			lock_kernel();
-			do_remount_sb(sb, MS_RDONLY, NULL, 1);
+			do_remount_sb(sb, MS_RDONLY, NULL);
 			unlock_kernel();
 		}
 		drop_super(sb);
@@ -861,7 +839,7 @@
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	do_remount_sb(s, flags, data, 0);
+	do_remount_sb(s, flags, data);
 	return simple_set_mnt(mnt, s);
 }
 
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2007-02-05 18:21:03.000000000 +0100
+++ linux-2.6/include/linux/fs.h	2007-02-05 18:21:27.000000000 +0100
@@ -1600,8 +1600,7 @@
 extern void __fsync_super(struct super_block *sb);
 extern void emergency_sync(void);
 extern void emergency_remount(void);
-extern int do_remount_sb(struct super_block *sb, int flags,
-			 void *data, int force);
+extern int do_remount_sb(struct super_block *sb, int flags, void *data);
 #ifdef CONFIG_BLOCK
 extern sector_t bmap(struct inode *, sector_t);
 #endif

Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)

[Jan Engelhardt]

On Feb 5 2007 18:32, Christoph Hellwig wrote:
>
>in two recent discussions (file_list_lock scalability and remount r/o
>on suspend) I stumbled over this emergency remount feature.  It's not
>actually useful because it tries a potentially dangerous remount
>despite writers still beeing in progress, which we can't get rid.

The current way is to remount things, and return -EROFS to any process
that attempts to write(). Unless we want to kill processes to get rid of
them [most likely we possibly won't], I am fine with how things are atm.
So, what's the "dangerous" part, actually?

>Any ideas and comments?

sysrq+u is helpful. It is like \( sysrq+s && make sure no further writes
go to disk \).



Jan
-- 

Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)

[Eric W. Biederman]

Christoph Hellwig <hch@lst.de> writes:

> Hi there,
>
> in two recent discussions (file_list_lock scalability and remount r/o
> on suspend) I stumbled over this emergency remount feature.  It's not
> actually useful because it tries a potentially dangerous remount
> despite writers still beeing in progress, which we can't get rid.
>
> I've attached one patch in this mail that simply kills the
> functionality, and in a reply to this mail I'll send a second one
> that keeps the sysrq functionality, but removes the force argument
> from do_remount_sb that overrides the still busy check.  This version
> is currently not useful, but makes a lot of sense once Dave Hansens
> per-mountpoint r/o patches get in, as we can check for a real write
> in progress instead of simply a file opened with write permission.
>
> Any ideas and comments?

Most things that sysrq allows you to do are dubious but work most
of the time, and are much better than the alternatives.  Synching and
remounting read-only are several of those, so is rebooting.  If you
look at the emergency reboot path it doesn't do the clean shutdown
that every other reboot path does.

So if it is a maintenance problem I'm inclined to believe there is
a problem that needs fixing.  Otherwise I'm not certain you understand
the point of making thees things available with sysrq.

Eric

Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)

[Nigel Cunningham]

Hi.

On Mon, 2007-02-05 at 18:32 +0100, Christoph Hellwig wrote:
> Hi there,
> 
> in two recent discussions (file_list_lock scalability and remount r/o
> on suspend) I stumbled over this emergency remount feature.  It's not
> actually useful because it tries a potentially dangerous remount
> despite writers still beeing in progress, which we can't get rid.
> 
> I've attached one patch in this mail that simply kills the
> functionality, and in a reply to this mail I'll send a second one
> that keeps the sysrq functionality, but removes the force argument
> from do_remount_sb that overrides the still busy check.  This version
> is currently not useful, but makes a lot of sense once Dave Hansens
> per-mountpoint r/o patches get in, as we can check for a real write
> in progress instead of simply a file opened with write permission.
> 
> Any ideas and comments?

I'm not really keen - it sometimes get's invoked here and by others in a
sysrq-s sysrq-u sysrq-b sequence (sync, unmount, reboot) in a context
where things have gone south (particularly if there's some process
stuck). In that context it helps make filesystems cleaner than they'd
otherwise be, and the fact that writers might still be in progress is
irrelevant because the next keypress is going to reboot anyway.

Ok. I'll admit to being a heretic ext3 user, loving not having to fsck
after the above and still getting zero corruption as a result.

Regards,

Nigel

Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)

[Theodore Tso]

On Mon, Feb 05, 2007 at 09:40:08PM +0100, Jan Engelhardt wrote:
> 
> On Feb 5 2007 18:32, Christoph Hellwig wrote:
> >
> >in two recent discussions (file_list_lock scalability and remount r/o
> >on suspend) I stumbled over this emergency remount feature.  It's not
> >actually useful because it tries a potentially dangerous remount
> >despite writers still beeing in progress, which we can't get rid.
> 
> The current way is to remount things, and return -EROFS to any process
> that attempts to write(). Unless we want to kill processes to get rid of
> them [most likely we possibly won't], I am fine with how things are atm.
> So, what's the "dangerous" part, actually?

The dangerous part is that we change f->f_mode for all open files
without regard for whether there might be any writes underway at the
time.  This isn't *serious* although the results might be a little
strange and it might result in a confused return from write(2).  More
seriously, mark_files_ro() in super.c *only* changes f->f_mode and
doesn't deal with the possibility that the file might be mapped
read-write.  For filesystems that do delayed allocation, I'm not at
all convinced that an emergency read-only will result in the
filesystem doing anything at all sane, depending on what else the
filesystem might do when the filesystem is forced into read-only state.

> sysrq+u is helpful. It is like \( sysrq+s && make sure no further writes
> go to disk \).

I agree it is useful, but if we're going to do it we really should do
it right.  We should have real revoke() functionality on file
descriptors, which revokes all of the mmap()'s (any attempt to write
into a previously read/write mmap will cause a SEGV) as well as
changing f_mode, and then use that to implement emergency read-only
remount.

						- Ted

Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)

[Christoph Hellwig]

On Mon, Feb 05, 2007 at 10:17:44PM -0500, Theodore Tso wrote:
> > sysrq+u is helpful. It is like \( sysrq+s && make sure no further writes
> > go to disk \).
> 
> I agree it is useful, but if we're going to do it we really should do
> it right.  We should have real revoke() functionality on file
> descriptors, which revokes all of the mmap()'s (any attempt to write
> into a previously read/write mmap will cause a SEGV) as well as
> changing f_mode, and then use that to implement emergency read-only
> remount.

Revoke is only part of it.  What we really need is proper forced unmount
support.  That means revoking any kind of userspace access, blocking new
access and making sure the ondisk image is coherent.  This would definitly
be a useful feature, but it's a lot of work.