Message-ID: <20080423115500.GA7003@mit.edu>
Date:	Wed, 23 Apr 2008 07:55:00 -0400
From:	Theodore Tso <tytso@....edu>
To:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>
Cc:	linux-ext4@...r.kernel.org
Subject: Re: What's cooking in e2fsprogs.git (topics)

On Wed, Apr 23, 2008 at 01:02:31PM +0530, Aneesh Kumar K.V wrote:
> > 	Unfortunately, there appears to be some kind of data
> > 	corruption bug if I force a tdb_data_size of 32768, so I'm not
> > 	entirely sure I trust the undo manager to be working
> > 	correctly.  The undo_manager code itself needs a pretty
> > 	serious auditing and checking to make sure it's doing the
> > 	right thing with large tdb_data_sizes.
> > 
> 
> undo-mgr is using the first blocks size used to write. This was done as
> per the discussion at
> 
> http://thread.gmane.org/gmane.comp.file-systems.ext4/1942
> http://thread.gmane.org/gmane.comp.file-systems.ext4/2056
> 
> I am not sure how you are forcing larger tdb_data_size. Can you send me
> the changes so that I can test it and see what is wrong.

There's a reason why I publish the "What's Cooking" messages, so
people can see what's in the master, next, and pu branches, and can
examine them to see what's in there.  I find the easiest way to look
at what's gone into the git repository is by using the gitk command,
i.e., "gitk pu", or "gitk maint", etc.  If you like terminal based
approaches, you can also use "git log pu", or "git log -p pu" if you
want to see the patches along side the changelog commit.

(BTW, it's good to browse the Linux kernel tree using git in this way
so you can get a good feel for the level of verbosity generally
favored by kernel maintainers, and match this in the ext4 patch
queue....)

------ 

   [This begins a git tutorial for those who don't know how to dig out
    the changes out of the 'pu' branch in the e2fsprogs git tree.
    Given that you asked me to send me the changes without realizing I
    had already pushed them out, I'm going to assume that you didn't
    know how to do this.  My apologies if you already did, and please
    skip to the next section; maybe someone else will find this mini
    git tutorial useful.]

I don't normally push out the heads of the individual components which
make up the 'pu' branch (mainly to avoid confusion), but it's normally
pretty easy to isolate the beginning of the ak/undo-mgr branch simply
by looking at the gitk output, or by doing something like this: "git
log --abbrev-commit --pretty=oneline --parents pu", which will show
you something like this:

0f83612... 2934064... 61639ff... Merge branch 'tt/64bit-bitmaps' into pu
2934064... 1140583... 9ba4000... Merge branch 'tt/flex-bg' into pu
1140583... d11736c... 625d7bf... Merge branch 'ak/undo-mgr' into pu
9ba4000... 494a1da... mke2fs: New bitmap and inode table allocation for FLEX_BG
494a1da... d11736c... Basic flexible block group support
d11736c... 0f2794c... ext2fs_open_inode_scan: Handle an non-zero bg_itable_used
0f2794c... 4476105... mke2fs/libext2fs: Fix lazy inode table initialization
625d7bf... 954b213... Add test cases for undoe2fs: u_undoe2fs_mke2fs and u_undo
954b213... cd81b90... Fix the resize inode test case
cd81b90... eadc218... tune2fs: Support for large inode migration.
eadc218... 5af269b... mke2fs: Add support for the undo I/O manager.
5af269b... 31db6f7... Add undoe2fs command
31db6f7... 4476105... libext2fs: Add undo I/O manager
4476105... 5711ed2... 748d071... Merge branch 'maint'

Either by looking at the "Merge branch 'ak/undo-mgr' into pu" line, or
by matching the subject lines from the "What's Cooking" e-mail, you
can easily determine that the head of the ak/undo-mgr branch is
625d7bf.  So you can, if you like set a branch point for ak/undo-mgr
in your own git repository and change to it, like this:

	git branch -f ak/undo-mgr 625d7bf
	git checkout ak/undo-mgr

You can then rebase all of the patches on that branch to be against
the latest master branch, like this:

	git rebase master

You can also look at the patches as ascii text via a command like this:

	git format-patch --subject-prefix="E2FSPROGS" -o /tmp/patches master

Or, you can edit individual patches, like this:

	git rebase --interactive master

This will throw up an editor window with the individual patches,
between the head of the branch and "master"; if you reorder the
patches by cutting and pasting lines, the patches will be reordered in
the branch.  If you delete a line, the patch will be dropped from the
branch.  If you change the first word in the editor window from "pick"
to "edit", during the rebasing process, it will stop, and you can look
at the tree right after the patch was applied, modify files, and then
use "git commit --amend -a" to ammend the current patch, and then type
"git rebase --continue" to continue the rebasing process.  It is
possible to edit multiple patches in a branch this way; DO NOT do this
to try to modify commits that have already been pushed out on the
'master' or the 'next' branches.  One of the reasons why we use the
'pu' branch is specifically for patches that need refinement in this
way, since commits between master/next and 'pu' aren't guaranteed to
be invariant.  It is therefore a bad idea to base patches off of the
'pu' branch, and this is why in general all of the topics branches
shown in the 'What's Cooking' reports are based off of 'master'.

And finally, when you want to send out a patch series for people to
see, you can use the git-format-patch command above (make sure you
clear out the /tmp/patches directory first, or use a new, non-existent
directory name), and then use the command:

	  git send-email --to linux-ext4@...r.kernel.org /tmp/patches

You can use the --compose option if you want to compose an
introductory message describing the patchset first.  Or use --dry-run
if you're not sure it will do what you want, first.  See the man pages
for all of these commands for more details.

-----------

Anyway, getting back to the subject at hand, how I am forcing the
larger tdb_data_size is in the modified ak/undo-mgr patches which are
in the 'pu' branch.  Most of the key bits are in the undo_io manager;
note especially:

	* tdb_data_size is now stored in the private data
	  section of the structure, instead of being a global static 
	  variable.  (Why this is so much better than a static variable 
	  is left as an exercise to the reader.)
	* setting the tdb_data_size has been moved from undo_write_tdb()
	  to undo_set_blksize().
	* new support added for the tdb_data_size option in undo_set_option()

The other key bit is in misc/mke2fs.c, in the main() function:

#if 0	      	  /* XXX bug in undo_io.c if we set this? */
    io_channel_set_options(fs->io, "tdb_data_size=32768");
#endif

Since ext2fs_initialize() calls io_channel_set_blksize(), this forces
tdb_data_size to be fs->blocksize, or 4096.  The call to
io_channel_set_options(), above forces the tdb_data_size to another
value.  If you set it to be 512, you will get the old, horrible
performance of the small tdb_data_size.  If you set it to 32768, then
when you use undoe2fs to replay the undo log, you get back a different
md5sum checksum, so the u_undoe2fs_mke2fs test ends up failing.

       		    			       - Ted

P.S.  Unfortunately, ext2fs_open() first sets the blocksize to 1024,
in order to read the superblock.  This means we need to either add an
io_channel_set_options(io, "tdb_data_size=<fs->blocksize>") to callers
of ext2fs_open(), or to ext2fs_open() itself.  I've held off on that
because for the tune2fs -I case, it may make more sense to set the
tdb_data_size to something really big, like 32768, once we figure out
why that isn't working in the mke2fs case.







--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists