Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)
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. - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH[RFC] kill sysrq-u (emergency remount r/o)
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.0 +0100
+++ linux-2.6/drivers/char/sysrq.c 2007-02-05 18:12:47.0 +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.0 +0100
+++ linux-2.6/fs/namespace.c2007-02-05 18:04:24.0 +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.0 +0100
+++ linux-2.6/fs/super.c2007-02-05 18:04:54.0 +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)) {
- /*
-
Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)
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 - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH[RFC] kill sysrq-u (emergency remount r/o)
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 - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
