Something wrong with extent-based journal creation

I've been tearing my hair out all day on this and not getting anywhere yet, so punting to the list ;)

I have a user running mke2fs -t ext4 in a virt guest, and a mount immediately after that fails.

It fails because the journal inode has an invalide i_extra_isize.

I've narrowed it down to the commit (961306d3) which creates the journal with extent format; if that's commented out, it works fine.

I can't for the life of me see where the problem is coming from, though.

I thought we'd need a write_inode_new (emphasis on new here) for the journal inode, but switching to that doesn't help.

I can't reproduce this myself... any ideas or suggestions of where to look? :)

-Eric

Well, here's one problem. I'm waiting for the reporter to confirm whether it fixes the bug. I think it likely does but will report back tomorrow. Anyway patch follows, thanks valgrind!

libext2fs: write only core inode in update_path()

The ext2_extent_handle only has a struct ext2_inode allocated on it, and the same amount copied into it in that same function, but in update_path() we're possibly writing out more than that - for example 256 bytes, from that address. This causes uninitialized memory to get written to disk, overwriting the parts of the inode past the osd2 member (the end of the smaller structure).

---

index b7eb617..0dfee62 100644
@@ -547,7 +547,7 @@ static errcode_t update_path(ext2_extent_handle_t handle)
 
 	if (handle->level == 0) {
 		retval = ext2fs_write_inode_full(handle->fs, handle->ino,
-			   handle->inode, EXT2_INODE_SIZE(handle->fs->super));
+			   handle->inode, sizeof(struct ext2_inode));
 	} else {
 		ix = handle->path[handle->level - 1].curr;
 		blk = ext2fs_le32_to_cpu(ix->ei_leaf) +

Oh, I see. The bug was introduced by commit 84b239ae

libext2fs: add ext2fs_extent_open2

The patch below adds a function, ext2fs_extent_open2(), that behaves as ext2fs_extent_open(), but will use the user-supplied inode structure when opening an extent instead of reading the inode from disk. It also changes several of the calls to extent_open() to use this enhancement.

Which replaced ext2fs_read_inode_full with ext2fs_read_inode(); which is fine, extents.c doesn't need to use the full inode; but it didn't change ext2fs_write_inode_full() with ext2fs_write_inode().

		retval = ext2fs_write_inode(handle->fs, handle->ino,
					    handle->inode);

							- Ted

Sure, that makes more sense, revised below:

libext2fs: write only core inode in update_path()

The ext2_extent_handle only has a struct ext2_inode allocated on it, and the same amount copied into it in that same function, but in update_path() we're possibly writing out more than that - for example 256 bytes, from that address. This causes uninitialized memory to get written to disk, overwriting the parts of the inode past the osd2 member (the end of the smaller structure).

---

index b7eb617..15ce302 100644
@@ -546,8 +546,8 @@ static errcode_t update_path(ext2_extent_handle_t handle)
 	struct ext3_extent_idx		*ix;
 
 	if (handle->level == 0) {
-		retval = ext2fs_write_inode_full(handle->fs, handle->ino,
-			   handle->inode, EXT2_INODE_SIZE(handle->fs->super));
+		retval = ext2fs_write_inode(handle->fs, handle->ino,
+				   handle->inode);
 	} else {
 		ix = handle->path[handle->level - 1].curr;
 		blk = ext2fs_le32_to_cpu(ix->ei_leaf) +

This is what I ultimately checked in. It converts all calls of ext2fs_write_inode_full() to ext2fs_write_inode().

- Ted

commit 125a36780626cdb0fc4d62fd529486baa8bce54c

Author: Eric Sandeen <[email protected]>
Date:   Wed Jun 17 18:49:01 2009 -0400

libext2fs: write only core inode in update_path()

The ext2_extent_handle only has a struct ext2_inode allocated on it, and the same amount copied into it in that same function, but in update_path() we're possibly writing out more than that - for example 256 bytes, from that address. This causes uninitialized memory to get written to disk, overwriting the parts of the inode past the osd2 member (the end of the smaller structure).

index 2b88739..35b080e 100644
@@ -546,8 +546,8 @@ static errcode_t update_path(ext2_extent_handle_t handle)
 	struct ext3_extent_idx		*ix;
 
 	if (handle->level == 0) {
-		retval = ext2fs_write_inode_full(handle->fs, handle->ino,
-			   handle->inode, EXT2_INODE_SIZE(handle->fs->super));
+		retval = ext2fs_write_inode(handle->fs, handle->ino,
+					    handle->inode);
 	} else {
 		ix = handle->path[handle->level - 1].curr;
 		blk = ext2fs_le32_to_cpu(ix->ei_leaf) +

@@ -1011,8 +1011,8 @@ static errcode_t extent_node_split(ext2_extent_handle_t handle)
 
 	/* new node hooked in, so update inode block count (do this here?) */
 	handle->inode->i_blocks += handle->fs->blocksize / 512;
-	retval = ext2fs_write_inode_full(handle->fs, handle->ino,
-		handle->inode, EXT2_INODE_SIZE(handle->fs->super));
+	retval = ext2fs_write_inode(handle->fs, handle->ino,
+				    handle->inode);
 	if (retval)
 		goto done;
 

@@ -1370,9 +1370,8 @@ errcode_t ext2fs_extent_delete(ext2_extent_handle_t handle, int flags)
 
 			retval = ext2fs_extent_delete(handle, flags);
 			handle->inode->i_blocks -= handle->fs->blocksize / 512;
-			retval = ext2fs_write_inode_full(handle->fs,
-					handle->ino, handle->inode,
-					EXT2_INODE_SIZE(handle->fs->super));
+			retval = ext2fs_write_inode(handle->fs, handle->ino,
+						    handle->inode);
 			ext2fs_block_alloc_stats(handle->fs, extent.e_pblk, -1);
 		}
 	} else {

Hey thanks for finding those, it fixed the next resize bug I was looking at! ;)

Now, put down the laptop and pay attention to that talk, Ted ;)

-Eric

Hmm, this seems like it might be a troublesome feature... Are we sure that there is proper kernel/tool compatibility for an extent-mapped journal? At least none of the Lustre code has any support for this, though they can handle the INCOMPAT_EXTENTS feature, and I don't think this was present in older kernels either.

I haven't looked at the code for this yet, so it may be that it will "just work", but if this is a new feature I would urge caution w.r.t. compatibility before this is merged upstream (in either e2fsprogs or the kernel).

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

It's already upstream.... :)

But I found the problem I was looking for, at least, see the [PATCH] in reply to my original email. (which is actually a pretty bad bug, likely causing problems during e2fsck etc as well)

-Eric

Extent-based journals "just works", at least in the kernel; I didn't need to make any changes to the kernel before I added support to using an extent-mapped journal; if the kernel supports extents, it should expert extent-mapped journals without any difficults. The patch to add this was merged in before e2fsprogs 1.41.1 was released, so mke2fs has been creating extent-based journals for a while.

- Ted