diff mbox

mk2fs lazy_journal_init option

Message ID 127958.57045.qm@web43516.mail.sp1.yahoo.com
State Rejected, archived
Headers show

Commit Message

number9652 Feb. 19, 2010, 7:53 p.m. UTC
I hope you decide to continue unconditionally zeroing the journal at mkfs time.

In this case, block_iterate2 was being used to create a file, but when the file became big enough to have an extent leaf, ext2fs_get_extent was returning an extent when it shouldn't have been.  That caused ext2fs_block_iterate2 to go wrong, eventually calling mkjournal_proc millions of times more than it needed to (during which it would immediately return).  A patch which resolves this is below.  It shows normal mkfs times for me with a 512 MB journal and passes make check.

Signed-off-by Nic Case <number9652@yahoo.com>





      

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

Comments

Andreas Dilger Feb. 24, 2010, 5:30 a.m. UTC | #1
On 2010-02-19, at 12:53, number9652 wrote:
> I hope you decide to continue unconditionally zeroing the journal at  
> mkfs time.

That will continue to be the default for the time being, unless we can  
be sure that there is no possibility of error.  That would be possible  
in conjunction with the journal checksum patch, if the checksum  
function is changed slightly to include the journal UUID.

> In this case, block_iterate2 was being used to create a file, but  
> when the file became big enough to have an extent leaf,  
> ext2fs_get_extent was returning an extent when it shouldn't have  
> been.  That caused ext2fs_block_iterate2 to go wrong, eventually  
> calling mkjournal_proc millions of times more than it needed to  
> (during which it would immediately return).  A patch which resolves  
> this is below.  It shows normal mkfs times for me with a 512 MB  
> journal and passes make check.
>
> Signed-off-by Nic Case <number9652@yahoo.com>

I haven't looked at this from a logic POV, but I tested this with a  
1GB journal and it ran the same time with/without specifying "-O  
extents" (10s vs. 650s without this patch).  I also ran our additional  
e2fsck extents tests without errors, and on a couple of my extent- 
based filesystems.

Tested-by: Andreas Dilger <adilger@sun.com>

> --- lib/ext2fs/extent-orig.c    2010-02-05 08:58:41.000000000 -0600
> +++ lib/ext2fs/extent.c 2010-02-19 13:37:32.000000000 -0600
> @@ -307,16 +307,12 @@
>                                op = EXT2_EXTENT_DOWN;
>                        } else if (path->left > 0)
>                                op = EXT2_EXTENT_NEXT_SIB;
> -                       else if (handle->level > 0)
> -                               op = EXT2_EXTENT_UP;
>                        else
>                                return EXT2_ET_EXTENT_NO_NEXT;
>                } else {
>                        /* leaf node */
>                        if (path->left > 0)
>                                op = EXT2_EXTENT_NEXT_SIB;
> -                       else if (handle->level > 0)
> -                               op = EXT2_EXTENT_UP;
>                        else
>                                return EXT2_ET_EXTENT_NO_NEXT;
>                }
>
>
>
>
>
>


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

--
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
number9652 March 2, 2010, 7:36 p.m. UTC | #2
--- On Tue, 2/23/10, Andreas Dilger wrote:
> I haven't looked at this from a logic POV, but I tested
> this with a 1GB journal and it ran the same time
> with/without specifying "-O extents" (10s vs. 650s without
> this patch).  I also ran our additional e2fsck extents
> tests without errors, and on a couple of my extent-based
> filesystems.

Thanks for testing this patch, but after looking at it again,
it is clear it is not the right thing to do.  If there are at
least two levels of the extent tree with more than one entry,
iterating through with EXTENT_NEXT will return NO_EXTENT before
reaching the end of the file.  I sent another patch to the ext4
mailing list (with ext2fs_extent_get in the title) that I believe
is the right way to fix the problem.

Nic



      

--
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
diff mbox

Patch

--- lib/ext2fs/extent-orig.c    2010-02-05 08:58:41.000000000 -0600
+++ lib/ext2fs/extent.c 2010-02-19 13:37:32.000000000 -0600
@@ -307,16 +307,12 @@ 
                                op = EXT2_EXTENT_DOWN;
                        } else if (path->left > 0)
                                op = EXT2_EXTENT_NEXT_SIB;
-                       else if (handle->level > 0)
-                               op = EXT2_EXTENT_UP;
                        else
                                return EXT2_ET_EXTENT_NO_NEXT;
                } else {
                        /* leaf node */
                        if (path->left > 0)
                                op = EXT2_EXTENT_NEXT_SIB;
-                       else if (handle->level > 0)
-                               op = EXT2_EXTENT_UP;
                        else
                                return EXT2_ET_EXTENT_NO_NEXT;
                }