Something wrong with extent-based journal creation

Something wrong with extent-based journal creation

[Eric Sandeen]

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

[PATCH] libext2fs: write only core inode in update_path()

[Eric Sandeen]

Eric Sandeen wrote:

> I've been tearing my hair out all day on this and not getting anywhere
> yet, so punting to the list ;)
>
>
>   
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).

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index b7eb617..0dfee62 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -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) +

Re: [PATCH] libext2fs: write only core inode in update_path()

[Theodore Tso]

On Tue, Jun 16, 2009 at 07:57:59PM -0500, Eric Sandeen wrote:
> 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).

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.
    
    Signed-off-by: Nic Case <number9652@yahoo.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

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

> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index b7eb617..0dfee62 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -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));

Probably it would be better/simpler to replace this with:

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

							- Ted

Re: [PATCH] libext2fs: write only core inode in update_path()

[Eric Sandeen]

Theodore Tso wrote:

>
> Probably it would be better/simpler to replace this with:
>
> 		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).

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index b7eb617..15ce302 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -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) +

Re: [PATCH] libext2fs: write only core inode in update_path()

[Theodore Tso]

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 <sandeen@redhat.com>
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).
    
    Signed-off-by: Eric Sandeen <sandeen@redhat.com>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
index 2b88739..35b080e 100644
--- a/lib/ext2fs/extent.c
+++ b/lib/ext2fs/extent.c
@@ -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 {

Re: [PATCH] libext2fs: write only core inode in update_path()

[Eric Sandeen]

Theodore Tso wrote:
> This is what I ultimately checked in.  It converts all calls of
> ext2fs_write_inode_full() to ext2fs_write_inode().

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

> 							- Ted
> 
> commit 125a36780626cdb0fc4d62fd529486baa8bce54c
> Author: Eric Sandeen <sandeen@redhat.com>
> 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).
>     
>     Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> diff --git a/lib/ext2fs/extent.c b/lib/ext2fs/extent.c
> index 2b88739..35b080e 100644
> --- a/lib/ext2fs/extent.c
> +++ b/lib/ext2fs/extent.c
> @@ -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 {

Re: Something wrong with extent-based journal creation

[Andreas Dilger]

On Jun 16, 2009  17:38 -0500, Eric Sandeen wrote:
> I've narrowed it down to the commit (961306d3) which creates the journal
> with extent format; if that's commented out, it works fine.

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.

Re: Something wrong with extent-based journal creation

[Eric Sandeen]

Andreas Dilger wrote:
> On Jun 16, 2009  17:38 -0500, Eric Sandeen wrote:
>> I've narrowed it down to the commit (961306d3) which creates the journal
>> with extent format; if that's commented out, it works fine.
> 
> 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).

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

Re: Something wrong with extent-based journal creation

[Theodore Tso]

On Wed, Jun 17, 2009 at 02:04:32AM -0600, Andreas Dilger wrote:
> On Jun 16, 2009  17:38 -0500, Eric Sandeen wrote:
> > I've narrowed it down to the commit (961306d3) which creates the journal
> > with extent format; if that's commented out, it works fine.
> 
> 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).

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