Patchwork Minimal configuration for e2fsprogs

login
register
mail settings
Submitter Tony Breeds
Date June 18, 2012, 5:58 a.m.
Message ID <20120618055836.GA30557@thor.bakeyournoodle.com>
Download mbox | patch
Permalink /patch/165385/
State Superseded
Headers show

Comments

Tony Breeds - June 18, 2012, 5:58 a.m.
On Fri, Jun 15, 2012 at 08:08:25PM -0400, Ted Ts'o wrote:

> So I think you need to break your list a bit more.  There are a number
> of functions on this list where, if yaboot doesn't actually call a
> particular function (for example, the progess bar functions, in
> lib/ext2fs/progress.c), the fact that they reference the stdio
> functions won't matter.  Something similar will be going on with
> test_io.c; if you don't use those functions, you won't need to provide
> stubs for any of the libc functions called by them.

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?

What have I misunderstood?

> There are some cases where we may need to create some configure
> options.  For example, there are a large number of the symbols that
> you have listed that got dragged in by the lib/ext2fs/mmp.c.  That's
> one where it may be simplest to add a set of #ifdef's that comment out
> multi-mount protection --- I assume you don't plan to support booting
> over fibre channel where you might need to care about having two
> systems trying to modify the same file system at the same time.  :-)

No we really don't need mmp :)  I can see creating --enable-mmp and
--disbale-mmp options for configure would be reasonable and not too
intrusive, and it gets rid of nearly 30% of the libc functions.

Something like this lightly tested patch:
---
---

Feedback very welcome but if it looks okay I'll add my signed off and
resubmit with a reasonable commit message.
 
> Which brings up another point --- are you only planning on opening the
> file system read-only, or do you expect to modify the file system from
> yaboot?  If you don't need to modify the file system, and so you don't
> need to load the bitmap manipulation functions, that will make a whole
> bunch more of the libc dependencies drop out.

Well currently we open it read-write but I cannot see a problem with
switching to read-only.  Based on my very limited understanding of
e2fsprogs it seems that disabling the bitmap functions is good for yaboot
it would result in a library that has pretty limited value to anyone
else.

A really ugly local hack to disbale the bitmap functions (along with the
patch above) results in calloc() being the only missing symbol.  Adding
calloc is probably a good idea anyway :)
 
> So the bottom line is yes, I think we can do somethings to help you;
> but depending on which parts of the libext2fs functionality yaboot
> actually needs, it may not be as bad as you think, especially if you
> are willing to limit which libext2fs functions you call.

We're a very simple user.  We only support ext* on local disks so no
fibre channel or anything too fancy.

For the record we currently only call:
	ext2fs_open()
	ext2fs_namei_follow()
	ext2fs_follow_link()
	ext2fs_read_inode()
	ext2fs_close()
	ext2fs_block_iterate()
	ext2fs_bmap() # Actually we may not use this anymore 

I'm also happy to rewrite the yaboot code for ext* if someone with
more knowledge can make a provide pointers.

I'd like to keep the headaches involved with using yaboot "small", so if
I can do a little extra work now to ensure that users not need to really
care what options (dir_index etc) the file-system is created with I'm
all for that.

Yours Tony
Theodore Ts'o - June 18, 2012, 5:12 p.m.
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.

> No we really don't need mmp :)  I can see creating --enable-mmp and
> --disbale-mmp options for configure would be reasonable and not too
> intrusive, and it gets rid of nearly 30% of the libc functions.

Yes, absolutely.  I'll look at your patch but adding --disable-mmp is
something I'm very comfortable adding to e2fsprogs.

> Feedback very welcome but if it looks okay I'll add my signed off and
> resubmit with a reasonable commit message.

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.

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.

> Well currently we open it read-write but I cannot see a problem with
> switching to read-only.  Based on my very limited understanding of
> e2fsprogs it seems that disabling the bitmap functions is good for yaboot
> it would result in a library that has pretty limited value to anyone
> else.

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.

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.

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.

> For the record we currently only call:
> 	ext2fs_open()
> 	ext2fs_namei_follow()
> 	ext2fs_follow_link()
> 	ext2fs_read_inode()
> 	ext2fs_close()
> 	ext2fs_block_iterate()
> 	ext2fs_bmap() # Actually we may not use this anymore 
> 
> I'm also happy to rewrite the yaboot code for ext* if someone with
> more knowledge can make a provide pointers.

OK, that's useful.  I'm a little busy at the moment, but in the next
couple of weeks as I have time I'll trace down what is getting dragged
in and see if I can minimize the number of library functions that are
pulled in via static library link, and where necessary add configure
options to disable stuff that you wouldn't need.

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.

Regards,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Breeds - June 26, 2012, 2:10 a.m.
On Mon, Jun 18, 2012 at 01:12:57PM -0400, Ted Ts'o wrote:

> 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. 

I'm ready to tackle the --disable-libext2fs-stats portion but I need to
make sure I'm looking in the rigth area.

I was thinking that this was the very sinmple matter of making
BMAP_STATS conditional rather that have it always #defined.  Is that
what you were thinking?

Also what's the correct way to get BMAP_STATS_OPS defined?

Yours Tony
Theodore Ts'o - June 26, 2012, 2:33 a.m.
On Tue, Jun 26, 2012 at 12:10:47PM +1000, Tony Breeds wrote:
> 
> I was thinking that this was the very sinmple matter of making
> BMAP_STATS conditional rather that have it always #defined.  Is that
> what you were thinking?
> 
> Also what's the correct way to get BMAP_STATS_OPS defined?

Yeah, I was thinking to simply rename BMAP_STATS and BMAP_STATS_OPS to
ENABLE_BMAP_STATS and ENABLE_BMAP_STATS_OPS, and then handle them via
an AC_ARG_ENABLE much like --enable-compression is handled (with the
first enabled by default, so --disable-bmap-stats to the configure
script would disable it, while the much more heavier weight from a
performance standpoint BMAP_STATS_OPS would be disabled by default, so
the user would have to specify --enable-bmap-stats-ops to the
configure script to enable it.)

Regards,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Breeds - June 26, 2012, 2:47 a.m.
On Mon, Jun 25, 2012 at 10:33:31PM -0400, Theodore Ts'o wrote:
> On Tue, Jun 26, 2012 at 12:10:47PM +1000, Tony Breeds wrote:
> > 
> > I was thinking that this was the very sinmple matter of making
> > BMAP_STATS conditional rather that have it always #defined.  Is that
> > what you were thinking?
> > 
> > Also what's the correct way to get BMAP_STATS_OPS defined?
> 
> Yeah, I was thinking to simply rename BMAP_STATS and BMAP_STATS_OPS to
> ENABLE_BMAP_STATS and ENABLE_BMAP_STATS_OPS, and then handle them via
> an AC_ARG_ENABLE much like --enable-compression is handled (with the
> first enabled by default, so --disable-bmap-stats to the configure
> script would disable it, while the much more heavier weight from a
> performance standpoint BMAP_STATS_OPS would be disabled by default, so
> the user would have to specify --enable-bmap-stats-ops to the
> configure script to enable it.)

Thanks for the clarification.  Should be doable today.

Yours Tony

Patch

diff --git a/MCONFIG.in b/MCONFIG.in
index fa2b03e..a0c828a 100644
--- a/MCONFIG.in
+++ b/MCONFIG.in
@@ -53,7 +53,7 @@  datadir = @datadir@
 CC = @CC@
 BUILD_CC = @BUILD_CC@
 CFLAGS = @CFLAGS@
-CPPFLAGS = @INCLUDES@
+CPPFLAGS = @INCLUDES@ $(ENABLE_MPP)
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 LDFLAGS = @LDFLAGS@
 ALL_LDFLAGS = $(LDFLAGS) @LDFLAG_DYNAMIC@
diff --git a/configure.in b/configure.in
index 7373e8e..f7562d3 100644
--- a/configure.in
+++ b/configure.in
@@ -746,6 +746,24 @@  AC_MSG_RESULT([Building uuidd by default])
 )
 AC_SUBST(UUIDD_CMT)
 dnl
+dnl handle --disable-mmp
+dnl
+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])
+	MMP_CMT=
+fi
+,
+AC_MSG_RESULT([Enabling mmp support by default])
+MMP_CMT=
+)
+AC_SUBST(MMP_CMT)
+dnl
 dnl
 dnl
 MAKEFILE_LIBRARY=$srcdir/lib/Makefile.library
diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
index 0d9ac21..021f3b0 100644
--- a/lib/ext2fs/Makefile.in
+++ b/lib/ext2fs/Makefile.in
@@ -14,9 +14,11 @@  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
+@MMP_CMT@ENABLE_MMP = -DENABLE_MMP
 
 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 +68,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/ext2fs.h b/lib/ext2fs/ext2fs.h
index ff088bb..cfa8822 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 ENABLE_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,22 @@  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);
+#else
+static errcode_t ext2fs_mmp_read(ext2_filsys fs, blk64_t mmp_blk, void *buf)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_write(ext2_filsys fs, blk64_t mmp_blk, void *buf)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_clear(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_init(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_start(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_update(ext2_filsys fs)
+{ return (errcode_t)0; }
+static errcode_t ext2fs_mmp_stop(ext2_filsys fs)
+{ return (unsigned)0; }
+#endif /* ENABLE_MMP */
 
 /* read_bb.c */
 extern errcode_t ext2fs_read_bb_inode(ext2_filsys fs,