Patchwork ext4: ext4_split_extent shoult take care about extent zeroout v2

login
register
mail settings
Submitter Dmitri Monakho
Date Jan. 31, 2013, 2:48 p.m.
Message ID <1359643738-22435-1-git-send-email-dmonakhov@openvz.org>
Download mbox | patch
Permalink /patch/217196/
State Superseded
Headers show

Comments

Dmitri Monakho - Jan. 31, 2013, 2:48 p.m.
When ext4_split_extent_at() ends up doing zeroout & conversion to
initialized instead of split & conversion, ext4_split_extent() gets
confused and can wrongly mark the extent back as uninitialized resulting in
end IO code getting confused from large unwritten extents and may result in
data loss.

The example of problematic behavior is:
			    lblk len              lblk len
  ext4_split_extent() (ex=[1000,30,uninit], map=[1010,10])
    ext4_split_extent_at() (split [1000,30,uninit] at 1020)
      ext4_ext_insert_extent() -> ENOSPC
      ext4_ext_zeroout()
	 -> extent [1000,30] is now initialized
    ext4_split_extent_at() (split [1000,30,init] at 1010,
			     MARK_UNINIT1 | MARK_UNINIT2)
      -> extent is split and parts marked as uninitialized

Fix the problem by rechecking extent type after the first
ext4_split_extent_at() returns. None of split_flags can not be applied to
initialized extent so this patch also add BUG_ON to prevent similar issues
in future.

TESTCASE: https://github.com/dmonakhov/xfstests/commit/1a1c4f337d4d198803436c63a56625b1a78d8a5e

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)
Jan Kara - Jan. 31, 2013, 4:12 p.m.
On Thu 31-01-13 18:48:58, Dmitry Monakhov wrote:
> When ext4_split_extent_at() ends up doing zeroout & conversion to
> initialized instead of split & conversion, ext4_split_extent() gets
> confused and can wrongly mark the extent back as uninitialized resulting in
> end IO code getting confused from large unwritten extents and may result in
> data loss.
> 
> The example of problematic behavior is:
> 			    lblk len              lblk len
>   ext4_split_extent() (ex=[1000,30,uninit], map=[1010,10])
>     ext4_split_extent_at() (split [1000,30,uninit] at 1020)
>       ext4_ext_insert_extent() -> ENOSPC
>       ext4_ext_zeroout()
> 	 -> extent [1000,30] is now initialized
>     ext4_split_extent_at() (split [1000,30,init] at 1010,
> 			     MARK_UNINIT1 | MARK_UNINIT2)
>       -> extent is split and parts marked as uninitialized
> 
> Fix the problem by rechecking extent type after the first
> ext4_split_extent_at() returns. None of split_flags can not be applied to
> initialized extent so this patch also add BUG_ON to prevent similar issues
> in future.
> 
> TESTCASE: https://github.com/dmonakhov/xfstests/commit/1a1c4f337d4d198803436c63a56625b1a78d8a5e
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index fd51469..05222d5 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2982,6 +2982,10 @@ static int ext4_split_extent_at(handle_t *handle,
>  	newblock = split - ee_block + ext4_ext_pblock(ex);
>  
>  	BUG_ON(split < ee_block || split >= (ee_block + ee_len));
> +	BUG_ON(!ext4_ext_is_uninitialized(ex) &&
> +	       split_flag & (EXT4_EXT_MAY_ZEROOUT |
> +			     EXT4_EXT_MARK_UNINIT1 |
> +			     EXT4_EXT_MARK_UNINIT2));
>  
>  	err = ext4_ext_get_access(handle, inode, path + depth);
>  	if (err)
> @@ -3091,18 +3095,24 @@ static int ext4_split_extent(handle_t *handle,
>  		if (err)
>  			goto out;
>  	}
> -
> +	/*
> +	 * Update path is required because previous ext4_split_extent_at() may
> +	 * result in split of original leaf or extent zeroout.
> +	 */
>  	ext4_ext_drop_refs(path);
>  	path = ext4_ext_find_extent(inode, map->m_lblk, path);
>  	if (IS_ERR(path))
>  		return PTR_ERR(path);
> +	depth = ext_depth(inode);
> +	ex = path[depth].p_ext;
> +	uninitialized = ext4_ext_is_uninitialized(ex);
> +	split_flag1 = 0;
>  
>  	if (map->m_lblk >= ee_block) {
> -		split_flag1 = split_flag & EXT4_EXT_MAY_ZEROOUT;
> -		if (uninitialized)
> -			split_flag1 |= EXT4_EXT_MARK_UNINIT1;
> -		if (split_flag & EXT4_EXT_MARK_UNINIT2)
> -			split_flag1 |= EXT4_EXT_MARK_UNINIT2;
> +		if (uninitialized) {
> +			split_flag1 = EXT4_EXT_MARK_UNINIT1;
> +			split_flag1 |= split_flag & (EXT4_EXT_MAY_ZEROOUT | EXT4_EXT_MARK_UNINIT2);
> +		}
>  		err = ext4_split_extent_at(handle, inode, path,
>  				map->m_lblk, split_flag1, flags);
>  		if (err)
> -- 
> 1.7.1
> 
> --
> 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
Eric Sandeen - Feb. 1, 2013, 4:09 p.m.
On 1/31/13 8:48 AM, Dmitry Monakhov wrote:
> When ext4_split_extent_at() ends up doing zeroout & conversion to
> initialized instead of split & conversion, ext4_split_extent() gets
> confused and can wrongly mark the extent back as uninitialized resulting in
> end IO code getting confused from large unwritten extents and may result in
> data loss.
> 
> The example of problematic behavior is:
> 			    lblk len              lblk len
>   ext4_split_extent() (ex=[1000,30,uninit], map=[1010,10])
>     ext4_split_extent_at() (split [1000,30,uninit] at 1020)
>       ext4_ext_insert_extent() -> ENOSPC
>       ext4_ext_zeroout()
> 	 -> extent [1000,30] is now initialized
>     ext4_split_extent_at() (split [1000,30,init] at 1010,
> 			     MARK_UNINIT1 | MARK_UNINIT2)
>       -> extent is split and parts marked as uninitialized
> 
> Fix the problem by rechecking extent type after the first
> ext4_split_extent_at() returns. None of split_flags can not be applied to
> initialized extent so this patch also add BUG_ON to prevent similar issues
> in future.
> 
> TESTCASE: https://github.com/dmonakhov/xfstests/commit/1a1c4f337d4d198803436c63a56625b1a78d8a5e

Dmitry, thanks - want to send that testcase (and any others you've collected
that aren't in the sgi repo) to xfs@oss.sgi.com?

Thanks,
-Eric

> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/extents.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index fd51469..05222d5 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2982,6 +2982,10 @@ static int ext4_split_extent_at(handle_t *handle,
>  	newblock = split - ee_block + ext4_ext_pblock(ex);
>  
>  	BUG_ON(split < ee_block || split >= (ee_block + ee_len));
> +	BUG_ON(!ext4_ext_is_uninitialized(ex) &&
> +	       split_flag & (EXT4_EXT_MAY_ZEROOUT |
> +			     EXT4_EXT_MARK_UNINIT1 |
> +			     EXT4_EXT_MARK_UNINIT2));
>  
>  	err = ext4_ext_get_access(handle, inode, path + depth);
>  	if (err)
> @@ -3091,18 +3095,24 @@ static int ext4_split_extent(handle_t *handle,
>  		if (err)
>  			goto out;
>  	}
> -
> +	/*
> +	 * Update path is required because previous ext4_split_extent_at() may
> +	 * result in split of original leaf or extent zeroout.
> +	 */
>  	ext4_ext_drop_refs(path);
>  	path = ext4_ext_find_extent(inode, map->m_lblk, path);
>  	if (IS_ERR(path))
>  		return PTR_ERR(path);
> +	depth = ext_depth(inode);
> +	ex = path[depth].p_ext;
> +	uninitialized = ext4_ext_is_uninitialized(ex);
> +	split_flag1 = 0;
>  
>  	if (map->m_lblk >= ee_block) {
> -		split_flag1 = split_flag & EXT4_EXT_MAY_ZEROOUT;
> -		if (uninitialized)
> -			split_flag1 |= EXT4_EXT_MARK_UNINIT1;
> -		if (split_flag & EXT4_EXT_MARK_UNINIT2)
> -			split_flag1 |= EXT4_EXT_MARK_UNINIT2;
> +		if (uninitialized) {
> +			split_flag1 = EXT4_EXT_MARK_UNINIT1;
> +			split_flag1 |= split_flag & (EXT4_EXT_MAY_ZEROOUT | EXT4_EXT_MARK_UNINIT2);
> +		}
>  		err = ext4_split_extent_at(handle, inode, path,
>  				map->m_lblk, split_flag1, flags);
>  		if (err)
> 

--
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 - Feb. 4, 2013, 7:37 a.m.
On Fri, 01 Feb 2013 10:09:34 -0600, Eric Sandeen <sandeen@redhat.com> wrote:
> On 1/31/13 8:48 AM, Dmitry Monakhov wrote:
> > When ext4_split_extent_at() ends up doing zeroout & conversion to
> > initialized instead of split & conversion, ext4_split_extent() gets
> > confused and can wrongly mark the extent back as uninitialized resulting in
> > end IO code getting confused from large unwritten extents and may result in
> > data loss.
> > 
> > The example of problematic behavior is:
> > 			    lblk len              lblk len
> >   ext4_split_extent() (ex=[1000,30,uninit], map=[1010,10])
> >     ext4_split_extent_at() (split [1000,30,uninit] at 1020)
> >       ext4_ext_insert_extent() -> ENOSPC
> >       ext4_ext_zeroout()
> > 	 -> extent [1000,30] is now initialized
> >     ext4_split_extent_at() (split [1000,30,init] at 1010,
> > 			     MARK_UNINIT1 | MARK_UNINIT2)
> >       -> extent is split and parts marked as uninitialized
> > 
> > Fix the problem by rechecking extent type after the first
> > ext4_split_extent_at() returns. None of split_flags can not be applied to
> > initialized extent so this patch also add BUG_ON to prevent similar issues
> > in future.
> > 
> > TESTCASE: https://github.com/dmonakhov/xfstests/commit/1a1c4f337d4d198803436c63a56625b1a78d8a5e
> 
> Dmitry, thanks - want to send that testcase (and any others you've collected
> that aren't in the sgi repo) to xfs@oss.sgi.com?

Off course. I'll a bunch of my tests this week.
> 
> Thanks,
> -Eric
> 
> > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> > ---
> >  fs/ext4/extents.c |   22 ++++++++++++++++------
> >  1 files changed, 16 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index fd51469..05222d5 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -2982,6 +2982,10 @@ static int ext4_split_extent_at(handle_t *handle,
> >  	newblock = split - ee_block + ext4_ext_pblock(ex);
> >  
> >  	BUG_ON(split < ee_block || split >= (ee_block + ee_len));
> > +	BUG_ON(!ext4_ext_is_uninitialized(ex) &&
> > +	       split_flag & (EXT4_EXT_MAY_ZEROOUT |
> > +			     EXT4_EXT_MARK_UNINIT1 |
> > +			     EXT4_EXT_MARK_UNINIT2));
> >  
> >  	err = ext4_ext_get_access(handle, inode, path + depth);
> >  	if (err)
> > @@ -3091,18 +3095,24 @@ static int ext4_split_extent(handle_t *handle,
> >  		if (err)
> >  			goto out;
> >  	}
> > -
> > +	/*
> > +	 * Update path is required because previous ext4_split_extent_at() may
> > +	 * result in split of original leaf or extent zeroout.
> > +	 */
> >  	ext4_ext_drop_refs(path);
> >  	path = ext4_ext_find_extent(inode, map->m_lblk, path);
> >  	if (IS_ERR(path))
> >  		return PTR_ERR(path);
> > +	depth = ext_depth(inode);
> > +	ex = path[depth].p_ext;
> > +	uninitialized = ext4_ext_is_uninitialized(ex);
> > +	split_flag1 = 0;
> >  
> >  	if (map->m_lblk >= ee_block) {
> > -		split_flag1 = split_flag & EXT4_EXT_MAY_ZEROOUT;
> > -		if (uninitialized)
> > -			split_flag1 |= EXT4_EXT_MARK_UNINIT1;
> > -		if (split_flag & EXT4_EXT_MARK_UNINIT2)
> > -			split_flag1 |= EXT4_EXT_MARK_UNINIT2;
> > +		if (uninitialized) {
> > +			split_flag1 = EXT4_EXT_MARK_UNINIT1;
> > +			split_flag1 |= split_flag & (EXT4_EXT_MAY_ZEROOUT | EXT4_EXT_MARK_UNINIT2);
> > +		}
> >  		err = ext4_split_extent_at(handle, inode, path,
> >  				map->m_lblk, split_flag1, flags);
> >  		if (err)
> > 
> 
> --
> 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

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index fd51469..05222d5 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2982,6 +2982,10 @@  static int ext4_split_extent_at(handle_t *handle,
 	newblock = split - ee_block + ext4_ext_pblock(ex);
 
 	BUG_ON(split < ee_block || split >= (ee_block + ee_len));
+	BUG_ON(!ext4_ext_is_uninitialized(ex) &&
+	       split_flag & (EXT4_EXT_MAY_ZEROOUT |
+			     EXT4_EXT_MARK_UNINIT1 |
+			     EXT4_EXT_MARK_UNINIT2));
 
 	err = ext4_ext_get_access(handle, inode, path + depth);
 	if (err)
@@ -3091,18 +3095,24 @@  static int ext4_split_extent(handle_t *handle,
 		if (err)
 			goto out;
 	}
-
+	/*
+	 * Update path is required because previous ext4_split_extent_at() may
+	 * result in split of original leaf or extent zeroout.
+	 */
 	ext4_ext_drop_refs(path);
 	path = ext4_ext_find_extent(inode, map->m_lblk, path);
 	if (IS_ERR(path))
 		return PTR_ERR(path);
+	depth = ext_depth(inode);
+	ex = path[depth].p_ext;
+	uninitialized = ext4_ext_is_uninitialized(ex);
+	split_flag1 = 0;
 
 	if (map->m_lblk >= ee_block) {
-		split_flag1 = split_flag & EXT4_EXT_MAY_ZEROOUT;
-		if (uninitialized)
-			split_flag1 |= EXT4_EXT_MARK_UNINIT1;
-		if (split_flag & EXT4_EXT_MARK_UNINIT2)
-			split_flag1 |= EXT4_EXT_MARK_UNINIT2;
+		if (uninitialized) {
+			split_flag1 = EXT4_EXT_MARK_UNINIT1;
+			split_flag1 |= split_flag & (EXT4_EXT_MAY_ZEROOUT | EXT4_EXT_MARK_UNINIT2);
+		}
 		err = ext4_split_extent_at(handle, inode, path,
 				map->m_lblk, split_flag1, flags);
 		if (err)