Date:   Fri, 11 May 2018 14:00:49 +0200
From:   Lukas Czerner <lczerner@...hat.com>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     tytso@....edu, linux-ext4@...r.kernel.org,
        Shuichi Ihara <sihara@....com>, Li Xi <lixi@....com>,
        Wang Shilong <wshilong@....com>
Subject: Re: [PATCH 10/10] misc: add e2mmpstatus utility via debugfs

On Fri, May 04, 2018 at 12:31:10PM -0600, Andreas Dilger wrote:
> > Subject: [PATCH 10/10] misc: add e2mmpstatus utility via debugfs
> 
> I just noticed that I made an error in the patch summary.  This
> should be "dumpe2fs" rather than "debugfs".
> 
> On May 4, 2018, at 4:06 AM, Lukas Czerner <lczerner@...hat.com> wrote:
> > 
> > On Tue, May 01, 2018 at 10:26:06PM -0600, Andreas Dilger wrote:
> >> From: Shuichi Ihara <sihara@....com>
> >> 
> >> e2mmpstatus is a Multi-Mount Protection (MMP) helper utility to read
> >> an MMP block to see if it is being updated.  It can also output the
> >> latest update time, nodename, and device from the MMP block.
> >> 
> >> This is useful for HA and other maintenance scripts to determine if
> >> the filesystem is in use on another node, and which node it is.
> >> 
> >> Signed-off-by: Shuichi Ihara <sihara@....com>
> >> Signed-off-by: Li Xi <lixi@....com>
> >> Signed-off-by: Wang Shilong <wshilong@....com>
> >> 
> >> Moved e2mmpstatus checking/dumping code to be part of dumpe2fs rather
> >> than a standalone program, using the "-m" option to check MMP status,
> >> and "-i" to dump info.  If dumpe2fs is called as "e2mmpstatus" (and
> >> also "mmpstatus" for compatibility reasons), assume "-m" is specified.
> > 
> > Hmm, why do we need an alias to dumpe2fs options ? For what
> > compatibility you need mmpstatus as well ? I do not like it, see below.
> 
> For compatibility with people who are using the old (standalone) version
> of mmpstatus on their systems.  I'm trying to reduce the delta between
> the Lustre-specific e2fsprogs and upstream, and hopefully one day get rid
> of the Lustre version.  The "e2" part of "e2mmpstatus" name was my change,
> since it otherwise isn't clear that the tool is related to e2fsprogs at
> all, the original tool was named "mmpstatus".  Since the string comparison
> is virtually the same I figured not a big deal to allow both.
> 
> >> @@ -539,7 +627,18 @@ int main (int argc, char ** argv)
> >> 			header_only++;
> >> 			break;
> >> 		case 'i':
> >> -			image_dump++;
> >> +			if (mmp_check)
> >> +				mmp_info++;
> >> +			else
> >> +				image_dump++;
> >> +			break;
> >> +		case 'm':
> >> +			mmp_check++;
> >> +			header_only++;
> >> +			if (image_dump) {
> >> +				mmp_info = image_dump;
> >> +				image_dump = 0;
> >> +			}
> > 
> > That's fugly, I hate it with passion. The whole idea of making a decision
> > based on the binary name changing the meaning of the parameters
> 
> Take a look at mke2fs for a similar example:
> 
>         if (argc && *argv) {
>                 program_name = get_progname(*argv);
> 
>                 /* If called as mkfs.ext3, create a journal inode */
>                 if (!strcmp(program_name, "mkfs.ext3") ||
>                     !strcmp(program_name, "mke3fs"))
>                         journal_size = -1;
>         }
> 
> and the same for tune2fs:
> 
> #ifdef CONFIG_BUILD_FINDFS
>         if (strcmp(get_progname(argv[0]), "findfs") == 0)
>                 do_findfs(argc, argv);
> #endif
>         if (strcmp(get_progname(argv[0]), "e2label") == 0)
>                 parse_e2label_options(argc, argv);
>         else
>                 parse_tune2fs_options(argc, argv);
> 
> and also other utilities, so this isn't wholly unusual:
> 
>     0 lrwxrwxrwx 1 root root          5 Jun 19  2011 bunzip2 -> bzip2*
>     0 lrwxrwxrwx 1 root root          5 Jun 19  2011 bzcat -> bzip2*
> 
> 
> I'd originally submitted e2mmpstatus as a standalone binary, but Ted
> asked me to implement it as part of dumpe2fs so that it would also get
> this useful functionality, and compatibility could still be kept with
> existing deployments.  Another alternative would be to include a one-line
> shell script named e2mmpstatus, I don't have a strong opinion, but I
> figured I'd stick with the same mechanism (argv[0] checking) used already:
> 
> #!/bin/sh
> dumpe2fs -m $*
> 
> > and on top of that changing the meanin of "-i" in case we specify "-m" as
> > well, looks like a horrible mess to me.
> 
> I agree that is a tiny bit messy, but '-i' was the argument to mmpstatus
> to dump MMP info rather than return a pass/fail check of the MMP block.
> I could take the route of e2label vs. tune2fs and have completely separate
> argument parsing based on argv[0] so that dumpe2fs options don't bleed
> over to e2mmpstatus, but I didn't think that was critical for just one
> argument which is unlikely to actually conflict in real life.
> 
> Ted, what is your preference here?
> 
> Cheers, Andreas

I understand, but I still disagree with it being implemented in this
way. the one-line shell script looks like a simpler more elegant
solution to me, that is if we really want to provide e2mmpstatus
mmpstatus executables.

But anyway the rest of the series apart from this one patch looks
good. You can add

Reviewed-by: Lukas Czerner <lczerner@...hat.com>

Thanks!
-Lukas

> 
> > Again, why do we need the mmpstatus alias ? why is dumpe2fs not enough ?
> > 
> > -Lukas
> > 
> >> 			break;
> >> 		case 'o':
> >> 			parse_extended_opts(optarg, &use_superblock,
> >> @@ -557,12 +656,12 @@ int main (int argc, char ** argv)
> >> 			usage();
> >> 		}
> >> 	}
> >> -	if (optind != argc - 1) {
> >> +	if (optind != argc - 1)
> >> 		usage();
> >> -		exit(1);
> >> -	}
> >> +
> >> 	device_name = argv[optind++];
> >> -	flags = EXT2_FLAG_JOURNAL_DEV_OK | EXT2_FLAG_SOFTSUPP_FEATURES | EXT2_FLAG_64BITS;
> >> +	flags = EXT2_FLAG_JOURNAL_DEV_OK | EXT2_FLAG_SOFTSUPP_FEATURES |
> >> +		EXT2_FLAG_64BITS;
> >> 	if (force)
> >> 		flags |= EXT2_FLAG_FORCE;
> >> 	if (image_dump)
> >> @@ -579,64 +678,87 @@ try_open_again:
> >> 			if (!retval)
> >> 				break;
> >> 		}
> >> -	} else
> >> -		retval = ext2fs_open (device_name, flags, use_superblock,
> >> -				      use_blocksize, unix_io_manager, &fs);
> >> -	if (retval && !(flags & EXT2_FLAG_IGNORE_CSUM_ERRORS)) {
> >> -		flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
> >> -		goto try_open_again;
> >> +	} else {
> >> +		retval = ext2fs_open(device_name, flags, use_superblock,
> >> +				     use_blocksize, unix_io_manager, &fs);
> >> 	}
> >> -	if (!retval && (fs->flags & EXT2_FLAG_IGNORE_CSUM_ERRORS))
> >> -		printf("%s", _("\n*** Checksum errors detected in filesystem!  Run e2fsck now!\n\n"));
> >> 	flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
> >> +	if (retval && !retval_csum) {
> >> +		retval_csum = retval;
> >> +		error_csum = _("while trying to open %s");
> >> +		goto try_open_again;
> >> +	}
> >> 	if (retval) {
> >> -		com_err (program_name, retval, _("while trying to open %s"),
> >> -			 device_name);
> >> +		com_err(program_name, retval, _("while trying to open %s"),
> >> +			device_name);
> >> 		printf("%s", _("Couldn't find valid filesystem superblock.\n"));
> >> 		if (retval == EXT2_ET_BAD_MAGIC)
> >> 			check_plausibility(device_name, CHECK_FS_EXIST, NULL);
> >> -		exit (1);
> >> +		goto out;
> >> 	}
> >> 	fs->default_bitmap_type = EXT2FS_BMAP64_RBTREE;
> >> 	if (ext2fs_has_feature_64bit(fs->super))
> >> 		blocks64 = 1;
> >> -	if (print_badblocks) {
> >> +	if (mmp_check) {
> >> +		if (ext2fs_has_feature_mmp(fs->super) &&
> >> +		    fs->super->s_mmp_block != 0) {
> >> +			if (mmp_info) {
> >> +				print_mmp_block(fs);
> >> +				printf("    mmp_block_number: ");
> >> +				print_number(fs->super->s_mmp_block);
> >> +				printf("\n");
> >> +			} else {
> >> +				retval = check_mmp(fs);
> >> +			}
> >> +			if (!retval && retval_csum)
> >> +				retval = 2;
> >> +		} else {
> >> +			fprintf(stderr, _("%s: MMP feature not enabled.\n"),
> >> +				program_name);
> >> +			retval = 2;
> >> +		}
> >> +	} else if (print_badblocks) {
> >> 		list_bad_blocks(fs, 1);
> >> 	} else {
> >> 		if (grp_only)
> >> 			goto just_descriptors;
> >> -		list_super (fs->super);
> >> +		list_super(fs->super);
> >> 		if (ext2fs_has_feature_journal_dev(fs->super)) {
> >> 			print_journal_information(fs);
> >> -			ext2fs_close_free(&fs);
> >> -			exit(0);
> >> +
> >> +			goto out_close;
> >> 		}
> >> 		if (ext2fs_has_feature_journal(fs->super) &&
> >> 		    (fs->super->s_journal_inum != 0))
> >> 			print_inline_journal_information(fs);
> >> +		if (ext2fs_has_feature_mmp(fs->super) &&
> >> +		    fs->super->s_mmp_block != 0)
> >> +			print_mmp_block(fs);
> >> 		list_bad_blocks(fs, 0);
> >> -		if (header_only) {
> >> -			ext2fs_close_free(&fs);
> >> -			exit (0);
> >> -		}
> >> +		if (header_only)
> >> +			goto out_close;
> >> +
> >> 		fs->flags &= ~EXT2_FLAG_IGNORE_CSUM_ERRORS;
> >> try_bitmaps_again:
> >> -		retval = ext2fs_read_bitmaps (fs);
> >> -		if (retval && !(fs->flags & EXT2_FLAG_IGNORE_CSUM_ERRORS)) {
> >> +		retval = ext2fs_read_bitmaps(fs);
> >> +		if (retval && !retval_csum) {
> >> 			fs->flags |= EXT2_FLAG_IGNORE_CSUM_ERRORS;
> >> +			retval_csum = retval;
> >> +			error_csum = _("while trying to read '%s' bitmaps\n");
> >> 			goto try_bitmaps_again;
> >> 		}
> >> -		if (!retval && (fs->flags & EXT2_FLAG_IGNORE_CSUM_ERRORS))
> >> -			printf("%s", _("\n*** Checksum errors detected in bitmaps!  Run e2fsck now!\n\n"));
> >> just_descriptors:
> >> 		list_desc(fs, grp_only);
> >> -		if (retval) {
> >> -			printf(_("\n%s: %s: error reading bitmaps: %s\n"),
> >> -			       program_name, device_name,
> >> -			       error_message(retval));
> >> -		}
> >> +	}
> >> +out_close:
> >> +	if (retval_csum) {
> >> +		com_err(program_name, retval_csum, error_csum, device_name);
> >> +		printf("%s", _("*** Run e2fsck now!\n\n"));
> >> +		if (!retval)
> >> +			retval = retval_csum;
> >> 	}
> >> 	ext2fs_close_free(&fs);
> >> 	remove_error_table(&et_ext2_error_table);
> >> -	exit (0);
> >> +out:
> >> +	return retval;
> >> }
> >> diff --git a/misc/e2mmpstatus.8.in b/misc/e2mmpstatus.8.in
> >> new file mode 100644
> >> index 0000000..f7d9557
> >> --- /dev/null
> >> +++ b/misc/e2mmpstatus.8.in
> >> @@ -0,0 +1,59 @@
> >> +.\" -*- nroff -*-
> >> +.\" This file may be copied under the terms of the GNU Public License.
> >> +.\"
> >> +.TH E2MMPSTATUS 8 "@E2FSPROGS_MONTH@ @E2FSPROGS_YEAR@" "E2fsprogs version @E2FSPROGS_VERSION@"
> >> +.SH NAME
> >> +e2mmpstatus \- Check MMP status of an ext4 filesystem
> >> +.SH SYNOPSIS
> >> +.BR e2mmpstatus " [" \-i ]
> >> +.RI < filesystem >
> >> +.SH OPTIONS
> >> +.TP
> >> +.B \-i
> >> +prints out the MMP information rather than check it.
> >> +.SH DESCRIPTION
> >> +.B e2mmpstatus
> >> +is used to check Multiple-Mount Protection (MMP) status of an ext4
> >> +filesystem with the
> >> +.B mmp
> >> +feature enabled.  The specified
> >> +.I filesystem
> >> +can be a device name (e.g.
> >> +.IR /dev/hdc1 ", " /dev/sdb2 ),
> >> +or an ext4 filesystem label or UUID, for example
> >> +.B UUID=8868abf6-88c5-4a83-98b8-bfc24057f7bd
> >> +or
> >> +.BR LABEL=root .
> >> +By default, the
> >> +.B e2mmpstatus
> >> +program checks whether it is safe to mount the filesystem without taking
> >> +the risk of mounting it more than once.
> >> +.PP
> >> +MMP (multiple-mount protection) is a feature that adds protection against
> >> +the filesystem being modified simultaneously by more than one node.
> >> +It is NOT safe to mount a filesystem when one of the following conditions
> >> +is true:
> >> +.br
> >> +\	1. e2fsck is running on the filesystem.
> >> +.br
> >> +\	2. the filesystem is in use by another node.
> >> +.br
> >> +\	3. The MMP block is corrupted or cannot be read for some reason.
> >> +.br
> >> +The
> >> +.B e2mmpstatus
> >> +program might wait for some time to see whether the MMP block is being
> >> +updated by any node during this period.  The time taken depends on how
> >> +frequently the MMP block is being written by the other node.
> >> +.SH EXIT CODE
> >> +The exit code returned by
> >> +.B e2mmpstatus
> >> +is 0 when it is safe to mount the filesystem, 1 when the MMP block shows
> >> +the filesystem is in use on another node and it is NOT safe to mount
> >> +the filesystem, and 2 if some other failure occurred that prevents the
> >> +check from properly detecting the current MMP status.
> >> +.SH SEE ALSO
> >> +.BR dumpe2fs (8),
> >> +.BR e2fsck (8),
> >> +.BR fstab (5),
> >> +.BR fsck (8),
> >> diff --git a/tests/f_mmp/script b/tests/f_mmp/script
> >> index 9ff16c9..07ae232 100644
> >> --- a/tests/f_mmp/script
> >> +++ b/tests/f_mmp/script
> >> @@ -43,6 +43,13 @@ rm -f $MARKFILE
> >> echo "kill debugfs abruptly (simulates e2fsck failure) ..." >> $test_name.log
> >> kill_debugfs
> >> 
> >> +$E2MMPSTATUS $TMPFILE > $test_name.log 2>&1
> >> +status=$?
> >> +if [ "$status" != 1 ] ; then
> >> +	echo "$E2MMPSTATUS with EXT2_MMP_SEQ_FSCK passed!" > $test_name.failed
> >> +	echo "$test_name: $test_description: failed"
> >> +	return 1
> >> +fi
> >> 
> >> echo "e2fsck (should fail mmp_seq = EXT2_MMP_SEQ_FSCK) ..." >> $test_name.log
> >> $FSCK $FSCK_OPT $TMPFILE >> $test_name.log 2>&1
> >> diff --git a/tests/m_mmp/expect.1 b/tests/m_mmp/expect.1
> >> index a1452e6..9d8a5a3 100644
> >> --- a/tests/m_mmp/expect.1
> >> +++ b/tests/m_mmp/expect.1
> >> @@ -46,6 +46,14 @@ Inode size:	          128
> >> Default directory hash:   half_md4
> >> MMP block number:         1049
> >> MMP update interval:      5
> >> +MMP_block:
> >> +    mmp_magic: 0x4d4d50
> >> +    mmp_check_interval: 5
> >> +    mmp_sequence: 0xff4d4d50
> >> +    mmp_update_date: test date
> >> +    mmp_update_time: test_time
> >> +    mmp_node_name: test_node
> >> +    mmp_device_name: test.img
> >> 
> >> 
> >> Group 0: (Blocks 0-32767)
> >> diff --git a/tests/m_mmp_bad_csum/expect b/tests/m_mmp_bad_csum/expect
> >> index e15e7b4..a0678ac 100644
> >> --- a/tests/m_mmp_bad_csum/expect
> >> +++ b/tests/m_mmp_bad_csum/expect
> >> @@ -1,4 +1,7 @@
> >> -Superblock MMP block checksum does not match MMP block.  Fix? yes
> >> +dumpe2fs: MMP block checksum does not match while trying to open test.img
> >> +dumpe2fs: MMP last updated by 'test_node' on test date
> >> +Exit status is 1
> >> +Superblock MMP block checksum does not match.  Fix? yes
> >> 
> >> Pass 1: Checking inodes, blocks, and sizes
> >> Pass 2: Checking directory structure
> >> @@ -7,3 +10,14 @@ Pass 4: Checking reference counts
> >> Pass 5: Checking group summary information
> >> test_filesys: 11/128 files (0.0% non-contiguous), 19/512 blocks
> >> Exit status is 0
> >> +dumpe2fs: it is safe to mount 'test.img', MMP is clean
> >> +Exit status is 0
> >> +MMP_block:
> >> +    mmp_magic: 0x4d4d50
> >> +    mmp_check_interval: 5
> >> +    mmp_sequence: 0xff4d4d50
> >> +    mmp_update_date: test date
> >> +    mmp_update_time: test_time
> >> +    mmp_node_name: test_node
> >> +    mmp_device_name: test.img
> >> +    mmp_block_number: 8
> >> diff --git a/tests/m_mmp_bad_csum/script b/tests/m_mmp_bad_csum/script
> >> index 09e870c..4c8fe16 100644
> >> --- a/tests/m_mmp_bad_csum/script
> >> +++ b/tests/m_mmp_bad_csum/script
> >> @@ -12,8 +12,15 @@ gzip -dc < $test_dir/image.gz > $TMPFILE
> >> 
> >> OUT=$test_name.log
> >> EXP=$test_dir/expect
> >> -$FSCK -fy $TMPFILE 2>&1 | sed -f $cmd_dir/filter.sed > $OUT
> >> +$E2MMPSTATUS $TMPFILE > $OUT 2>&1
> >> echo Exit status is $? >> $OUT
> >> +$FSCK -fy $TMPFILE >> $OUT 2>&1
> >> +echo Exit status is $? >> $OUT
> >> +$E2MMPSTATUS $TMPFILE >> $OUT 2>&1
> >> +echo Exit status is $? >> $OUT
> >> +$E2MMPSTATUS -i $TMPFILE >> $OUT 2>&1
> >> +sed -f $cmd_dir/filter.sed $OUT > $OUT.new
> >> +mv $OUT.new $OUT
> >> 
> >> rm -f $TMPFILE
> >> cmp -s $OUT $EXP
> >> diff --git a/tests/m_mmp_bad_magic/expect b/tests/m_mmp_bad_magic/expect
> >> index b5dfb89..d5fa98c 100644
> >> --- a/tests/m_mmp_bad_magic/expect
> >> +++ b/tests/m_mmp_bad_magic/expect
> >> @@ -1,3 +1,5 @@
> >> +dumpe2fs: MMP: invalid magic number while trying to open test.img
> >> +Exit status is 2
> >> Superblock has invalid MMP magic.  Fix? yes
> >> 
> >> Pass 1: Checking inodes, blocks, and sizes
> >> @@ -7,3 +9,14 @@ Pass 4: Checking reference counts
> >> Pass 5: Checking group summary information
> >> test_filesys: 11/128 files (0.0% non-contiguous), 19/512 blocks
> >> Exit status is 0
> >> +dumpe2fs: it is safe to mount 'test.img', MMP is clean
> >> +Exit status is 0
> >> +MMP_block:
> >> +    mmp_magic: 0x4d4d50
> >> +    mmp_check_interval: 5
> >> +    mmp_sequence: 0xff4d4d50
> >> +    mmp_update_date: test date
> >> +    mmp_update_time: test_time
> >> +    mmp_node_name: test_node
> >> +    mmp_device_name: test.img
> >> +    mmp_block_number: 8
> >> diff --git a/tests/m_mmp_bad_magic/script b/tests/m_mmp_bad_magic/script
> >> index 09e870c..4c8fe16 100644
> >> --- a/tests/m_mmp_bad_magic/script
> >> +++ b/tests/m_mmp_bad_magic/script
> >> @@ -12,8 +12,15 @@ gzip -dc < $test_dir/image.gz > $TMPFILE
> >> 
> >> OUT=$test_name.log
> >> EXP=$test_dir/expect
> >> -$FSCK -fy $TMPFILE 2>&1 | sed -f $cmd_dir/filter.sed > $OUT
> >> +$E2MMPSTATUS $TMPFILE > $OUT 2>&1
> >> echo Exit status is $? >> $OUT
> >> +$FSCK -fy $TMPFILE >> $OUT 2>&1
> >> +echo Exit status is $? >> $OUT
> >> +$E2MMPSTATUS $TMPFILE >> $OUT 2>&1
> >> +echo Exit status is $? >> $OUT
> >> +$E2MMPSTATUS -i $TMPFILE >> $OUT 2>&1
> >> +sed -f $cmd_dir/filter.sed $OUT > $OUT.new
> >> +mv $OUT.new $OUT
> >> 
> >> rm -f $TMPFILE
> >> cmp -s $OUT $EXP
> >> diff --git a/tests/test_config b/tests/test_config
> >> index cf9c79c..2aee6ff 100644
> >> --- a/tests/test_config
> >> +++ b/tests/test_config
> >> @@ -25,6 +25,7 @@ RESIZE2FS_EXE="../resize/resize2fs"
> >> RESIZE2FS="$USE_VALGRIND $RESIZE2FS_EXE"
> >> E2UNDO_EXE="../misc/e2undo"
> >> E2UNDO="$USE_VALGRIND $E2UNDO_EXE"
> >> +E2MMPSTATUS="$USE_VALGRIND ../misc/dumpe2fs -m"
> >> TEST_REL=../tests/progs/test_rel
> >> TEST_ICOUNT=../tests/progs/test_icount
> >> CRCSUM=../tests/progs/crcsum
> >> --
> >> 1.8.0
> >> 
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 

Powered by blists - more mailing lists