From patchwork Tue Jun 19 05:48:10 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tony Breeds X-Patchwork-Id: 165647 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 37F32B701D for ; Tue, 19 Jun 2012 15:48:22 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752171Ab2FSFsU (ORCPT ); Tue, 19 Jun 2012 01:48:20 -0400 Received: from ozlabs.org ([203.10.76.45]:54503 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752223Ab2FSFsT (ORCPT ); Tue, 19 Jun 2012 01:48:19 -0400 Received: from thor.bakeyournoodle.com (ibmaus65.lnk.telstra.net [165.228.126.9]) (using TLSv1 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPSA id B26C5B7017; Tue, 19 Jun 2012 15:48:16 +1000 (EST) Date: Tue, 19 Jun 2012 15:48:10 +1000 From: Tony Breeds To: Ted Ts'o Cc: linux-ext4@vger.kernel.org Subject: Re: Minimal configuration for e2fsprogs Message-ID: <20120619054751.GA2660@thor.bakeyournoodle.com> Mail-Followup-To: Ted Ts'o , linux-ext4@vger.kernel.org References: <20120615042421.GA7021@thor.bakeyournoodle.com> <20120616000825.GE7363@thunk.org> <20120618055836.GA30557@thor.bakeyournoodle.com> <20120618171257.GA16017@thunk.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20120618171257.GA16017@thunk.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org On Mon, Jun 18, 2012 at 01:12:57PM -0400, Ted Ts'o wrote: > On Mon, Jun 18, 2012 at 03:58:42PM +1000, Tony Breeds wrote: > > > > I'm not certain we're on the same page. We're linking statically so > > that fact we don't call the progress functions doesn't matter. The > > code is in libext2fs.a and there must be a call path from (eg) > > ext2fs_open() to fwrite(stderr, ...). The fact we don't add the > > EXT2_FLAG_PRINT_PROGRESS doesn't come into it does it? > > Oh, I see, the problem is that ext2fs_closefs() is calling the > progress functions; I had forgotten about it. Yeah, that's something > I can fix so that the progress functions are only dragged in if mke2fs > explicitly requests it, via adding function which enables the progress > functions, instead of just setting the EXT2_FLAG_PRINT_PROGRESS. Okay that sounds good to me. If I get the other bits implemented before you get to it. I'm happy to take a run at it. > So a couple of things; it's a really bad to define static functions in > a header file such as ext2fs.h. Yes, gcc will normally optimize out > functions which aren't used, but it will also complain with warnings > if you enable them. Thanks for the pointers. I had meant to make them static inline I just forgot the inline. > Instead, what I would do is to simply take out the calls to the > ext2fs_mmp library functions in e2fsck and tune2fs, and remove the MMP > feature flag from the list of supported features. That way, if > someone tries to use the e2fsck binary compiled with --disable-mmp on > a file system with MMP enabled, e2fsck will refuse to touch the file > system saying that it doesn't support the feature. If you just make > the mmp functions silently succeed (by returning 0), that's really bad > since file system damage could result. Okay that all make sense I've had a try but I think it's pretty brutal. I'm sure I could finesse it a bit but I have a couple of questions: 1) The only way I could see to remove MMP from the supported features was with something like: #ifdef CONFIG_MMP #define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...|EXT4_FEATURE_INCOMPAT_MMP|...) #else #define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...) #endif But EXT2_LIB_FEATURE_INCOMPAT_SUPP is already #defined twice depending on the state of ENABLE_COMPRESSION. So going down the path above would mean #defining it 4 times which I'd think is starting to indicate there must be a better way. Firstly am I in the right ball park, secondly Would something like #ifdef ENABLE_COMPRESSION ... #define _EXT4_FEATURE_INCOMPAT_COMPRESSION EXT4_FEATURE_INCOMPAT_COMPRESSION #else #define _EXT4_FEATURE_INCOMPAT_COMPRESSION (0) #endif #ifdef ENABLE_MMP ... #define _EXT4_FEATURE_INCOMPAT_MMP EXT4_FEATURE_INCOMPAT_MMP #else #define _EXT4_FEATURE_INCOMPAT_MMP (0) #endif #define EXT2_LIB_FEATURE_INCOMPAT_SUPP (...\ _EXT2_FEATURE_INCOMPAT_COMPRESSION|\ ... \ _EXT4_FEATURE_INCOMPAT_MMP|\ ...) Be acceptable? 2) in debugfs you have a command table (debug_cmds.ct or similar) there doesn't seem to be a way to exclude commands based on the state of CONFIG_MMP. Is it a problem the expose a debugfs command that will do nothing when built with --disable-mmp, or should I look at extending the command table generator to support the conditionals? I'll add my current patch after my .signature > I already have things set up so that if you don't actually call the > functions to read in the bitmaps, the functions to read and write the > bitmaps don't get dragged into a static library link. That's what I'm > referring to. Ahh thanks for the clarification. > The functions to actually create in-memory bitmaps for various > housekeeping things, do need to get dragged in, but we don't > necessarily need to drag in all of the different bitmap back-ends. If > we assume that yaboot doesn't need to optimize for low-memory usage > for really, really big file systems (i.e., you're not going to try to > allocate files or blocks while opening a multi-terrabyte file system > using libext2fs on a system with only 512mb), then you wouldn't need > blkmap64_rb.c. Right, we're operating in a pretty tight memory environment (typically 128MB or 256MB) but we're also usually opening file-systems <100GB usually closer to 200MB. > And the bitmap statistics functions are sometihng we can use a > configure option to disable, which should remove the last bits of > stdio usage I believe. Okay. If I can get the disable-mmp patch into a state you like it I'll tackle that configure option as well. > If you'd like to try to clean up the --disable-mmp patch, and perhaps > try your hand at a --disable-libext2fs-stats patch which disable the > statistics options, that would be great. Otherwise, I'll try to get > to it in the next few weeks. I really appreciate you help and time, and hope that continually pointing me in the right direction isn't too taxing on you. Yours Tony --- configure.in | 21 +++++++++++++++++++++ debugfs/debugfs.c | 2 ++ debugfs/set_fields.c | 9 +++++++++ e2fsck/journal.c | 2 ++ e2fsck/unix.c | 4 ++++ e2fsck/util.c | 6 ++++++ lib/config.h.in | 3 +++ lib/ext2fs/Makefile.in | 4 ++-- lib/ext2fs/closefs.c | 2 ++ lib/ext2fs/ext2fs.h | 2 ++ lib/ext2fs/openfs.c | 2 ++ misc/mke2fs.c | 2 ++ misc/tune2fs.c | 6 ++++++ 13 files changed, 63 insertions(+), 2 deletions(-) diff --git a/configure.in b/configure.in index 7373e8e..4bacd1a 100644 --- a/configure.in +++ b/configure.in @@ -746,6 +746,27 @@ AC_MSG_RESULT([Building uuidd by default]) ) AC_SUBST(UUIDD_CMT) dnl +dnl handle --disable-mmp +dnl +AH_TEMPLATE([CONFIG_MMP], [Define to 1 to enable mmp support]) +AC_ARG_ENABLE([mmp], +[ --disable-mmp disable support mmp, Multi Mount Protection], +if test "$enableval" = "no" +then + AC_MSG_RESULT([Disabling mmp support]) + MMP_CMT="#" +else + AC_MSG_RESULT([Enabling mmp support]) + AC_DEFINE(CONFIG_MMP, 1) + MMP_CMT= +fi +, +AC_MSG_RESULT([Enabling mmp support by default]) +AC_DEFINE(CONFIG_MMP, 1) +MMP_CMT= +) +AC_SUBST(MMP_CMT) +dnl dnl dnl MAKEFILE_LIBRARY=$srcdir/lib/Makefile.library diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c index cf80bd0..5df915e 100644 --- a/debugfs/debugfs.c +++ b/debugfs/debugfs.c @@ -2194,6 +2194,7 @@ void do_punch(int argc, char *argv[]) void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[]) { +#ifdef CONFIG_MMP struct ext2_super_block *sb; struct mmp_struct *mmp_s; time_t t; @@ -2237,6 +2238,7 @@ void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[]) fprintf(stdout, "node_name: %s\n", mmp_s->mmp_nodename); fprintf(stdout, "device_name: %s\n", mmp_s->mmp_bdevname); fprintf(stdout, "magic: 0x%x\n", mmp_s->mmp_magic); +#endif } static int source_file(const char *cmd_file, int sci_idx) diff --git a/debugfs/set_fields.c b/debugfs/set_fields.c index 08bfd8d..77fbbc1 100644 --- a/debugfs/set_fields.c +++ b/debugfs/set_fields.c @@ -69,8 +69,10 @@ static errcode_t parse_hashalg(struct field_set_info *info, char *field, char *a static errcode_t parse_time(struct field_set_info *info, char *field, char *arg); static errcode_t parse_bmap(struct field_set_info *info, char *field, char *arg); static errcode_t parse_gd_csum(struct field_set_info *info, char *field, char *arg); +#ifdef CONFIG_MMP static errcode_t parse_mmp_clear(struct field_set_info *info, char *field, char *arg); +#endif static struct field_set_info super_fields[] = { { "inodes_count", &set_sb.s_inodes_count, NULL, 4, parse_uint }, @@ -246,6 +248,7 @@ static struct field_set_info ext4_bg_fields[] = { }; static struct field_set_info mmp_fields[] = { +#ifdef CONFIG_MMP { "clear", &set_mmp.mmp_magic, NULL, sizeof(set_mmp), parse_mmp_clear }, { "magic", &set_mmp.mmp_magic, NULL, 4, parse_uint }, { "seq", &set_mmp.mmp_seq, NULL, 4, parse_uint }, @@ -255,6 +258,8 @@ static struct field_set_info mmp_fields[] = { { "bdevname", &set_mmp.mmp_bdevname, NULL, sizeof(set_mmp.mmp_bdevname), parse_string }, { "check_interval", &set_mmp.mmp_check_interval, NULL, 2, parse_uint }, +#endif + { 0, 0, 0, 0 } }; static int check_suffix(const char *field) @@ -748,6 +753,7 @@ void do_set_block_group_descriptor(int argc, char *argv[]) } } +#ifdef CONFIG static errcode_t parse_mmp_clear(struct field_set_info *info, char *field EXT2FS_ATTR((unused)), char *arg EXT2FS_ATTR((unused))) @@ -762,9 +768,11 @@ static errcode_t parse_mmp_clear(struct field_set_info *info, return 1; /* we don't need the MMP block written again */ } +#endif void do_set_mmp_value(int argc, char *argv[]) { +#ifdef CONFIG_MMP const char *usage = " \n" "\t\"set_mmp_value -l\" will list the names of " "MMP fields\n\twhich can be set."; @@ -819,5 +827,6 @@ void do_set_mmp_value(int argc, char *argv[]) &set_mmp); *mmp_s = set_mmp; } +#endif } diff --git a/e2fsck/journal.c b/e2fsck/journal.c index 767ea10..e5ee4ed 100644 --- a/e2fsck/journal.c +++ b/e2fsck/journal.c @@ -889,7 +889,9 @@ int e2fsck_run_ext3_journal(e2fsck_t ctx) if (stats && stats->bytes_written) kbytes_written = stats->bytes_written >> 10; +#ifdef CONFIG_MMP ext2fs_mmp_stop(ctx->fs); +#endif ext2fs_free(ctx->fs); retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW, ctx->superblock, blocksize, io_ptr, diff --git a/e2fsck/unix.c b/e2fsck/unix.c index 94260bd..6cca9e0 100644 --- a/e2fsck/unix.c +++ b/e2fsck/unix.c @@ -1042,6 +1042,7 @@ static const char *my_ver_date = E2FSPROGS_DATE; static int e2fsck_check_mmp(ext2_filsys fs, e2fsck_t ctx) { +#ifdef CONFIG_MMP struct mmp_struct *mmp_s; unsigned int mmp_check_interval; errcode_t retval = 0; @@ -1120,6 +1121,9 @@ check_error: } } return retval; +#else + return 0; +#endif } int main (int argc, char *argv[]) diff --git a/e2fsck/util.c b/e2fsck/util.c index 7c4caab..5b9b154 100644 --- a/e2fsck/util.c +++ b/e2fsck/util.c @@ -56,7 +56,9 @@ void fatal_error(e2fsck_t ctx, const char *msg) if (!fs) goto out; if (fs->io) { +#ifdef CONFIG_MMP ext2fs_mmp_stop(ctx->fs); +#endif if (ctx->fs->io->magic == EXT2_ET_MAGIC_IO_CHANNEL) io_channel_flush(ctx->fs->io); else @@ -780,6 +782,7 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg) errcode_t e2fsck_mmp_update(ext2_filsys fs) { +#ifdef CONFIF_MMP errcode_t retval; retval = ext2fs_mmp_update(fs); @@ -789,6 +792,9 @@ errcode_t e2fsck_mmp_update(ext2_filsys fs) "being modified while fsck is running.\n")); return retval; +#else + return 0; +#endif } void e2fsck_set_bitmap_type(ext2_filsys fs, unsigned int default_type, diff --git a/lib/config.h.in b/lib/config.h.in index 90e9743..52c3897 100644 --- a/lib/config.h.in +++ b/lib/config.h.in @@ -12,6 +12,9 @@ /* Define to 1 if debugging ext3/4 journal code */ #undef CONFIG_JBD_DEBUG +/* Define to 1 to enable mmp support */ +#undef CONFIG_MMP + /* Define to 1 to enable quota support */ #undef CONFIG_QUOTA diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in index 0d9ac21..5b73958 100644 --- a/lib/ext2fs/Makefile.in +++ b/lib/ext2fs/Makefile.in @@ -14,9 +14,10 @@ MK_CMDS= _SS_DIR_OVERRIDE=../ss ../ss/mk_cmds @RESIZER_CMT@RESIZE_LIB_OBJS = dupfs.o @TEST_IO_CMT@TEST_IO_LIB_OBJS = test_io.o @IMAGER_CMT@E2IMAGE_LIB_OBJS = imager.o +@MMP_CMT@MMP_OBJS = mmp.o OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \ - $(TEST_IO_LIB_OBJS) \ + $(TEST_IO_LIB_OBJS) $(MMP_OBJS) \ ext2_err.o \ alloc.o \ alloc_sb.o \ @@ -66,7 +67,6 @@ OBJS= $(DEBUGFS_LIB_OBJS) $(RESIZE_LIB_OBJS) $(E2IMAGE_LIB_OBJS) \ lookup.o \ mkdir.o \ mkjournal.o \ - mmp.o \ namei.o \ native.o \ newdir.o \ diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c index 973c2a2..abf77cc 100644 --- a/lib/ext2fs/closefs.c +++ b/lib/ext2fs/closefs.c @@ -464,9 +464,11 @@ errcode_t ext2fs_close2(ext2_filsys fs, int flags) return retval; } +#ifdef CONFIG_MMP retval = ext2fs_mmp_stop(fs); if (retval) return retval; +#endif ext2fs_free(fs); return 0; diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h index ff088bb..fc83973 100644 --- a/lib/ext2fs/ext2fs.h +++ b/lib/ext2fs/ext2fs.h @@ -1366,6 +1366,7 @@ errcode_t ext2fs_unlink(ext2_filsys fs, ext2_ino_t dir, const char *name, ext2_ino_t ino, int flags); /* mmp.c */ +#ifdef CONFIG_MMP errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf); errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf); errcode_t ext2fs_mmp_clear(ext2_filsys fs); @@ -1374,6 +1375,7 @@ errcode_t ext2fs_mmp_start(ext2_filsys fs); errcode_t ext2fs_mmp_update(ext2_filsys fs); errcode_t ext2fs_mmp_stop(ext2_filsys fs); unsigned ext2fs_mmp_new_seq(void); +#endif /* CONFIG_MMP */ /* read_bb.c */ extern errcode_t ext2fs_read_bb_inode(ext2_filsys fs, diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c index 482e4ab..88fdd4d 100644 --- a/lib/ext2fs/openfs.c +++ b/lib/ext2fs/openfs.c @@ -391,6 +391,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options, fs->flags &= ~EXT2_FLAG_NOFREE_ON_ERROR; *ret_fs = fs; +#ifdef CONFIG_MMP if ((fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_MMP) && !(flags & EXT2_FLAG_SKIP_MMP) && (flags & (EXT2_FLAG_RW | EXT2_FLAG_EXCLUSIVE))) { @@ -401,6 +402,7 @@ errcode_t ext2fs_open2(const char *name, const char *io_options, goto cleanup; } } +#endif return 0; cleanup: diff --git a/misc/mke2fs.c b/misc/mke2fs.c index 7ec8cc2..e9b4c1f 100644 --- a/misc/mke2fs.c +++ b/misc/mke2fs.c @@ -2557,6 +2557,7 @@ int main (int argc, char *argv[]) printf(_("done\n")); } no_journal: +#ifdef CONFIG_MMP if (!super_only && fs->super->s_feature_incompat & EXT4_FEATURE_INCOMPAT_MMP) { retval = ext2fs_mmp_init(fs); @@ -2570,6 +2571,7 @@ no_journal: "with update interval %d seconds.\n"), fs->super->s_mmp_update_interval); } +#endif if (EXT2_HAS_RO_COMPAT_FEATURE(&fs_param, EXT4_FEATURE_RO_COMPAT_BIGALLOC)) diff --git a/misc/tune2fs.c b/misc/tune2fs.c index f1f0bcf..2c4b20b 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -430,6 +430,7 @@ static int update_feature_set(ext2_filsys fs, char *features) return 1; } } +#ifdef CONFIG_MMP if (FEATURE_ON(E2P_FEATURE_INCOMPAT, EXT4_FEATURE_INCOMPAT_MMP)) { int error; @@ -495,6 +496,7 @@ mmp_error: sb->s_mmp_block = 0; sb->s_mmp_update_interval = 0; } +#endif if (FEATURE_ON(E2P_FEATURE_COMPAT, EXT3_FEATURE_COMPAT_HAS_JOURNAL)) { /* @@ -2105,10 +2107,12 @@ retry_open: goto closefs; } } +#ifdef CONFIG_MMP if (clear_mmp) { rc = ext2fs_mmp_clear(fs); goto closefs; } +#endif if (journal_size || journal_device) { rc = add_journal(fs); if (rc) @@ -2219,7 +2223,9 @@ retry_open: closefs: if (rc) { +#ifdef CONFIG_MMP ext2fs_mmp_stop(fs); +#endif exit(1); }