Patchwork [04/12] ext4: Disable merging of uninitialized extents

login
register
mail settings
Submitter Theodore Ts'o
Date Jan. 28, 2013, 3:38 p.m.
Message ID <20130128153836.GH22711@thunk.org>
Download mbox | patch
Permalink /patch/216230/
State Superseded
Headers show

Comments

Theodore Ts'o - Jan. 28, 2013, 3:38 p.m.
On Mon, Jan 28, 2013 at 07:02:55PM +0400, Dmitry Monakhov wrote:
> Actually this patch consists of two peaces
> 1) disable merging of uninitialized extents. (1 line change) I'm
> absolutely agree with it.

To be clear, that's this patch chunk (one line change not including
comments :-), right?


The one thing I'm a bit worried about is how much worse will extent
fragmentation be once we do this, but it's clear we need to strive for
correctness first.

							- 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
Dmitri Monakho - Jan. 29, 2013, 7:41 a.m.
On Mon, 28 Jan 2013 10:38:36 -0500, Theodore Ts'o <tytso@mit.edu> wrote:
> On Mon, Jan 28, 2013 at 07:02:55PM +0400, Dmitry Monakhov wrote:
> > Actually this patch consists of two peaces
> > 1) disable merging of uninitialized extents. (1 line change) I'm
> > absolutely agree with it.
> 
> To be clear, that's this patch chunk (one line change not including
> comments :-), right?
Off course.
> 
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1579,11 +1576,13 @@ int
>  ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
>  				struct ext4_extent *ex2)
>  {
>  
>  	/*
> -	 * Make sure that either both extents are uninitialized, or
> -	 * both are _not_.
> +	 * Make sure that both extents are initialized. We don't merge
> +	 * uninitialized extents so that we can be sure that end_io code has
> +	 * the extent that was written properly split out and conversion to
> +	 * initialized is trivial.
>  	 */
> -	if (ext4_ext_is_uninitialized(ex1) ^ ext4_ext_is_uninitialized(ex2))
> +	if (ext4_ext_is_uninitialized(ex1) || ext4_ext_is_uninitialized(ex2))
>  		return 0;
>  
> 
> The one thing I'm a bit worried about is how much worse will extent
> fragmentation be once we do this, but it's clear we need to strive for
> correctness first.
This change should not affect fragmentation because
1) Most people call fallocate(2) on big chunks (>4M)
2) Once uninitialized extent filled with data and converted to
initialized extents  will be merged immediately.
3) Most people use fallocate(2) for preallocation before write(2)
   so effectively calls are interleaved so merging works as expected.

The only case where fragmentation will increase is when someone
performs many fallocate(2) calls for small chunks (4k) w/o writes.
As result leaf block will consist of 256 extents 4k each.
Later writes can't help us because we can not merge extents from two
leaf blocks. But I still think that this use case it inconvenient.

BTW why do we not try to merge extents from two leaf blocs?
I do not see any technical difficulties. If two adjacent leaf blocks
are covered by common index block merging is possible (but we need +1
journal block).  
> 
> 							- 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
--
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
Zheng Liu - Jan. 29, 2013, 8:37 a.m.
On Tue, Jan 29, 2013 at 11:41:13AM +0400, Dmitry Monakhov wrote:
> On Mon, 28 Jan 2013 10:38:36 -0500, Theodore Ts'o <tytso@mit.edu> wrote:
> > On Mon, Jan 28, 2013 at 07:02:55PM +0400, Dmitry Monakhov wrote:
> > > Actually this patch consists of two peaces
> > > 1) disable merging of uninitialized extents. (1 line change) I'm
> > > absolutely agree with it.
> > 
> > To be clear, that's this patch chunk (one line change not including
> > comments :-), right?
> Off course.
> > 
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -1579,11 +1576,13 @@ int
> >  ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
> >  				struct ext4_extent *ex2)
> >  {
> >  
> >  	/*
> > -	 * Make sure that either both extents are uninitialized, or
> > -	 * both are _not_.
> > +	 * Make sure that both extents are initialized. We don't merge
> > +	 * uninitialized extents so that we can be sure that end_io code has
> > +	 * the extent that was written properly split out and conversion to
> > +	 * initialized is trivial.
> >  	 */
> > -	if (ext4_ext_is_uninitialized(ex1) ^ ext4_ext_is_uninitialized(ex2))
> > +	if (ext4_ext_is_uninitialized(ex1) || ext4_ext_is_uninitialized(ex2))
> >  		return 0;
> >  
> > 
> > The one thing I'm a bit worried about is how much worse will extent
> > fragmentation be once we do this, but it's clear we need to strive for
> > correctness first.
> This change should not affect fragmentation because
> 1) Most people call fallocate(2) on big chunks (>4M)
> 2) Once uninitialized extent filled with data and converted to
> initialized extents  will be merged immediately.
> 3) Most people use fallocate(2) for preallocation before write(2)
>    so effectively calls are interleaved so merging works as expected.
> 
> The only case where fragmentation will increase is when someone
> performs many fallocate(2) calls for small chunks (4k) w/o writes.
> As result leaf block will consist of 256 extents 4k each.
> Later writes can't help us because we can not merge extents from two
> leaf blocks. But I still think that this use case it inconvenient.

Yes, I doubt that no one does like this because it can not brings any
benefit.  We usually call fallocate(2) to preallocate some sequential
spaces.  Obviously preallocating a small chunk is useless.

> 
> BTW why do we not try to merge extents from two leaf blocs?
> I do not see any technical difficulties. If two adjacent leaf blocks
> are covered by common index block merging is possible (but we need +1
> journal block).

Yep, I also notice this.  I don't think there is a technical difficulty.
That would be great if two adjacent leaf blocks could be merged.

Regards,
                                                - Zheng
--
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

Patch

--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1579,11 +1576,13 @@  int
 ext4_can_extents_be_merged(struct inode *inode, struct ext4_extent *ex1,
 				struct ext4_extent *ex2)
 {
 
 	/*
-	 * Make sure that either both extents are uninitialized, or
-	 * both are _not_.
+	 * Make sure that both extents are initialized. We don't merge
+	 * uninitialized extents so that we can be sure that end_io code has
+	 * the extent that was written properly split out and conversion to
+	 * initialized is trivial.
 	 */
-	if (ext4_ext_is_uninitialized(ex1) ^ ext4_ext_is_uninitialized(ex2))
+	if (ext4_ext_is_uninitialized(ex1) || ext4_ext_is_uninitialized(ex2))
 		return 0;