[PATCH][e2fsprogs] Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

[PATCH][e2fsprogs] Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

[Jose R. Santos]

From: Jose R. Santos <jrs@us.ibm.com>

Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

Undo manager is a bit annoying when doing e2fsprogs testing since it
makes mke2fs significatly slower.  Use the MKE2FS_SCRATCH_DIR=disable
enviroment value to disable undo manager for those of us that blow up
filesystems on a regular basis.

Signed-off-by: Jose R. Santos <jrs@us.ibm.com>
--

 misc/mke2fs.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)


diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index c8170f0..3f9cbe2 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1802,6 +1802,13 @@ static int filesystem_exist(const char *name)
 	__u16	s_magic;
 	struct ext2_super_block super;
 	io_manager manager = unix_io_manager;
+	char *tdb_dir;
+
+	tdb_dir = getenv("MKE2FS_SCRATCH_DIR");
+	if (tdb_dir && !strcmp(tdb_dir, "disable")) {
+		retval = 0;
+		goto open_err_out;
+	}
 
 	retval = manager->open(name, IO_FLAG_EXCLUSIVE,  &channel);
 	if (retval) {

Re: [PATCH][e2fsprogs] Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

[Theodore Tso]

On Fri, Apr 04, 2008 at 09:02:35AM -0500, Jose R. Santos wrote:
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index c8170f0..3f9cbe2 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1802,6 +1802,13 @@ static int filesystem_exist(const char *name)
>  	__u16	s_magic;
>  	struct ext2_super_block super;
>  	io_manager manager = unix_io_manager;
> +	char *tdb_dir;
> +
> +	tdb_dir = getenv("MKE2FS_SCRATCH_DIR");
> +	if (tdb_dir && !strcmp(tdb_dir, "disable")) {
> +		retval = 0;
> +		goto open_err_out;
> +	}


Yuck.  Yuck, Yuck, Yuck, Yuck.

This functionality has nothing to do with filesystem_exist() and
putting this kind of magic (a) means you have to read the environment
variable twice, and (b) makes the code less maintainable in the
long run.

While I was looking at the code, I also realized that the way the undo
code was written it also wasn't compatible with configure
--enable-testio-deug.

So here's a much better patch that does the right thing.

Looking over the undo code more closely, there's much I find that's
ugly about this patch, including the fact that after you make a
filesystem at some device, say /dev/sda1, you have to manually go and
rm the file /var/lib/e2fsprogs/mke2fs-sda1 or you get the
non-intuitive error message "File exist /var/lib/e2fsprogs/mke2fs-sda1".  
This made me reach for the barf bag; the undo-mgr patch branch needs
cleanup before it's going into mainline.  In any case, here's a much
better patch which co-exists with testio-debugging, and which actually
decreases the number of lines of the code to support the undo feature.

(This will be merged into the patch "e2fsprogs: Make mke2fs use undo
I/O manager" before the whole branch gets integrated into the next or
master branches, using the magic that is git rebase --interactive.
Also needing fixing is the code to hook into the profile lookup.)

	  	      	     	      	     - Ted

commit 11079defe6c4bc3577381a8669f9944408d77023
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Sun Apr 6 18:08:12 2008 -0400

    Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR
    
    Undo manager is a bit annoying when doing e2fsprogs testing since it
    makes mke2fs significatly slower.  Use the MKE2FS_SCRATCH_DIR=disable
    enviroment value to disable undo manager for those of us that blow up
    filesystems on a regular basis.
    
    Also fix so that the undo manager doesn't conflict with configure
    --enable-testio-debug.
    
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index a23d841..06558d8 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1605,7 +1605,7 @@ open_err_out:
 	return retval;
 }
 
-static int mke2fs_setup_tdb(const char *name)
+static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 {
 	errcode_t retval = 0;
 	char *tdb_dir, tdb_file[PATH_MAX];
@@ -1620,24 +1620,24 @@ static int mke2fs_setup_tdb(const char *name)
 			"directory", 0, 0,
 			&tdb_dir);
 #endif
-	tmp_name = strdup(name);
-	device_name = basename(tmp_name);
-
 	tdb_dir = getenv("MKE2FS_SCRATCH_DIR");
 	if (!tdb_dir) {
 		printf(_("MKE2FS_SCRATCH_DIR not configured\n"));
 		printf(_("Using /var/lib/e2fsprogs\n"));
 		tdb_dir="/var/lib/e2fsprogs";
 	}
+	if (!strcmp(tdb_dir, "disable"))
+		return 0;
+
 	if (access(tdb_dir, W_OK)) {
 		fprintf(stderr,
 			_("Cannot create file under %s\n"),
 			tdb_dir);
-		retval = EXT2_ET_INVALID_ARGUMENT;
-		goto err_out;
-
+		return (EXT2_ET_INVALID_ARGUMENT);
 	}
 
+	tmp_name = strdup(name);
+	device_name = basename(tmp_name);
 	sprintf(tdb_file, "%s/mke2fs-%s", tdb_dir, device_name);
 
 	if (!access(tdb_file, F_OK)) {
@@ -1647,6 +1647,8 @@ static int mke2fs_setup_tdb(const char *name)
 		goto err_out;
 	}
 
+	set_undo_io_backing_manager(*io_ptr);
+	*io_ptr = undo_io_manager;
 	set_undo_io_backup_file(tdb_file);
 	printf(_("previous filesystem detected; to undo "
 		"the mke2fs operation, please run the "
@@ -1679,18 +1681,14 @@ int main (int argc, char *argv[])
 	io_ptr = test_io_manager;
 	test_io_backing_manager = unix_io_manager;
 #else
-	if (filesystem_exist(device_name)) {
+	io_ptr = unix_io_manager;
+#endif
 
-		io_ptr = undo_io_manager;
-		set_undo_io_backing_manager(unix_io_manager);
-		retval = mke2fs_setup_tdb(device_name);
+	if (filesystem_exist(device_name)) {
+		retval = mke2fs_setup_tdb(device_name, &io_ptr);
 		if (retval)
 			exit(1);

Re: [PATCH][e2fsprogs] Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

[Eric Sandeen]

Theodore Tso wrote:

> (This will be merged into the patch "e2fsprogs: Make mke2fs use undo
> I/O manager" before the whole branch gets integrated into the next or
> master branches, using the magic that is git rebase --interactive.
> Also needing fixing is the code to hook into the profile lookup.)

What is the rationale for turning mke2fs into a nanny for
administrators, anyway?  Maybe to complete the transformation we should
just make it a gtk application with a windows-like "Are you sure? [Yes]
[No]" alert dialog box that pops up?

Seriously, what does this gain us, other than a slowdown of an
already-slow mkfs?  I'm sure there are stories of people who mkfs'd the
wrong device but there are a million sad stories out there; rm -rf /, dd
if=/dev/null of=/dev/sda, fdisk the wrong device, you name it.  We can't
save them all.  :)

The notion of an (optional) undo IO manager is fine in general, I like
the idea that if I have dicey fsck to do I can in theory recover from it
if it goes badly, though even there I'd personally rather not have it on
by default...  (how do I turn it off for fsck?)  But mkfs, by default -
really?  I don't much like it, and on my boxes I'd like a way to
permanently turn it off, regardless of whether I'm testing or not...
Sure I could put it in my .bashrc or whatnot, but really, what does this
gain us?

-Eric

Re: [PATCH][e2fsprogs] Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

[Theodore Tso]

On Sun, Apr 06, 2008 at 08:44:39PM -0500, Eric Sandeen wrote:
> Theodore Tso wrote:
> 
> > (This will be merged into the patch "e2fsprogs: Make mke2fs use undo
> > I/O manager" before the whole branch gets integrated into the next or
> > master branches, using the magic that is git rebase --interactive.
> > Also needing fixing is the code to hook into the profile lookup.)
> 
> What is the rationale for turning mke2fs into a nanny for
> administrators, anyway?  Maybe to complete the transformation we should
> just make it a gtk application with a windows-like "Are you sure? [Yes]
> [No]" alert dialog box that pops up?
> 
> Seriously, what does this gain us, other than a slowdown of an
> already-slow mkfs?  I'm sure there are stories of people who mkfs'd the
> wrong device but there are a million sad stories out there; rm -rf /, dd
> if=/dev/null of=/dev/sda, fdisk the wrong device, you name it.  We can't
> save them all.  :)

The plan is to only enable it for uninit_groups, once uninit_groups
actually really does what the name implies (i.e., actually not
initialize the inode tables).  Unfortunately uninit_groups still needs
some fix-up work.  (As does flex_bg.)  That's one of the reasons why
I've been holding off on merging the undo manager at all.

So the idea is that we can make a reversible mke2fs in such a way that
it's way cheaper than it currently is today.  Sure, there are many
tales of woe out there, but we have made things a bit harder to
prevent users from accidentally running mke2fs on half of an MD
device, by adding the exclusive open feature in the kernel.

The fact that its defaults are bested right now is a problem, and
maybe I'll just fix it up so that for now, the MKE2FS_SCRATCH_DIR
environment variable must be set for it to save the undo file.

> The notion of an (optional) undo IO manager is fine in general, I like
> the idea that if I have dicey fsck to do I can in theory recover from it
> if it goes badly, though even there I'd personally rather not have it on
> by default...  (how do I turn it off for fsck?)  But mkfs, by default -
> really?  I don't much like it, and on my boxes I'd like a way to
> permanently turn it off, regardless of whether I'm testing or not...
> Sure I could put it in my .bashrc or whatnot, but really, what does this
> gain us?

Ultimately, a line in mke2fs.conf would also turn it off, but again,
there's a reason why the patch series has *not* been merged into the
"next" or "master" branch.  It still has a bunch of rough spots that
certainly does make it very annoying in its current state ---
completely granted.

						- Ted

Re: [PATCH][e2fsprogs] Allow user to disable Undo manager through MKE2FS_SCRATCH_DIR

[Aneesh Kumar K.V]

On Sun, Apr 06, 2008 at 06:19:47PM -0400, Theodore Tso wrote:
> On Fri, Apr 04, 2008 at 09:02:35AM -0500, Jose R. Santos wrote:
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index c8170f0..3f9cbe2 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -1802,6 +1802,13 @@ static int filesystem_exist(const char *name)
> >  	__u16	s_magic;
> >  	struct ext2_super_block super;
> >  	io_manager manager = unix_io_manager;
> > +	char *tdb_dir;
> > +
> > +	tdb_dir = getenv("MKE2FS_SCRATCH_DIR");
> > +	if (tdb_dir && !strcmp(tdb_dir, "disable")) {
> > +		retval = 0;
> > +		goto open_err_out;
> > +	}
> 
> 
> Yuck.  Yuck, Yuck, Yuck, Yuck.
> 
> This functionality has nothing to do with filesystem_exist() and
> putting this kind of magic (a) means you have to read the environment
> variable twice, and (b) makes the code less maintainable in the
> long run.
> 
> While I was looking at the code, I also realized that the way the undo
> code was written it also wasn't compatible with configure
> --enable-testio-deug.
> 
> So here's a much better patch that does the right thing.
> 
> Looking over the undo code more closely, there's much I find that's
> ugly about this patch, including the fact that after you make a
> filesystem at some device, say /dev/sda1, you have to manually go and
> rm the file /var/lib/e2fsprogs/mke2fs-sda1 or you get the
> non-intuitive error message "File exist /var/lib/e2fsprogs/mke2fs-sda1".  

One of the reason for doing that is to make sure we don't overwrite the
backup file. I decided not to go for multiple backup files because that
would confuse the user when trying to undo the mke2fs. So at any point
there would be only one backup file for a partition and that would
contain data needed to undo the last operation.

> This made me reach for the barf bag; the undo-mgr patch branch needs
> cleanup before it's going into mainline.  In any case, here's a much
> better patch which co-exists with testio-debugging, and which actually
> decreases the number of lines of the code to support the undo feature.
> 
> (This will be merged into the patch "e2fsprogs: Make mke2fs use undo
> I/O manager" before the whole branch gets integrated into the next or
> master branches, using the magic that is git rebase --interactive.
> Also needing fixing is the code to hook into the profile lookup.)
> 


I haven't looked at undo manager for some time. I will try to work
on it after April 15th. If you can list down what changes you would
like to see in the patches please let me know. I will definitely try
to address them.

-aneesh