diff mbox

[v2,3/5] ext4: fix wrong m_len value after unwritten extent conversion

Message ID 1362579435-6333-4-git-send-email-wenqing.lz@taobao.com
State Accepted, archived
Headers show

Commit Message

Zheng Liu March 6, 2013, 2:17 p.m. UTC 
From: Zheng Liu <wenqing.lz@taobao.com>

We always assume that the return value of ext4_ext_map_blocks is equal
to map->m_len but when we try to convert an unwritten extent 'm_len'
value will break this assumption.  It is harmless until we use status
tree as a extent cache because we need to update status tree according
to 'm_len' value.

Meanwhile this commit marks EXT4_MAP_MAPPED flag after unwritten extent
conversion.  It shouldn't cause a bug because we update status tree
according to checking EXT4_MAP_UNWRITTEN flag.  But it should be fixed.

After applied this commit, the following error message from self-testing
infrastructure disappears.

    ...
    kernel: ES len assertation failed for inode: 230 retval 1 !=
    map->m_len 3 in ext4_map_blocks (allocation)
    ...

Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/extents.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dmitry Monakhov March 7, 2013, 3:42 p.m. UTC | #1 
On Wed,  6 Mar 2013 22:17:13 +0800, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> We always assume that the return value of ext4_ext_map_blocks is equal
> to map->m_len but when we try to convert an unwritten extent 'm_len'
> value will break this assumption.  It is harmless until we use status
> tree as a extent cache because we need to update status tree according
> to 'm_len' value.
> 
> Meanwhile this commit marks EXT4_MAP_MAPPED flag after unwritten extent
> conversion.  It shouldn't cause a bug because we update status tree
> according to checking EXT4_MAP_UNWRITTEN flag.  But it should be fixed.
> 
> After applied this commit, the following error message from self-testing
> infrastructure disappears.
> 
>     ...
>     kernel: ES len assertation failed for inode: 230 retval 1 !=
>     map->m_len 3 in ext4_map_blocks (allocation)
>     ...
Ok. fill free to add Reviewed-by: Dmitry Monakhov <dmonakhov@openvz.org>
> 
> Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
> Cc: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/extents.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 25c86aa..110e85a 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3650,6 +3650,10 @@ ext4_ext_handle_uninitialized_extents(handle_t *handle, struct inode *inode,
>  						 path, map->m_len);
>  		} else
>  			err = ret;
> +		map->m_flags |= EXT4_MAP_MAPPED;
> +		if (allocated > map->m_len)
> +			allocated = map->m_len;
> +		map->m_len = allocated;
>  		goto out2;
>  	}
>  	/* buffered IO case */
> -- 
> 1.7.12.rc2.18.g61b472e
> 
> --
> 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
Theodore Ts'o March 11, 2013, 1:07 a.m. UTC | #2 
On Wed, Mar 06, 2013 at 10:17:13PM +0800, Zheng Liu wrote:
> From: Zheng Liu <wenqing.lz@taobao.com>
> 
> We always assume that the return value of ext4_ext_map_blocks is equal
> to map->m_len

Note that in general, this is _never_ safe to assume.  There are a
number of times when the number of blocks mapped is less than what the
caller originally requested, both when allocating blocks (and there
isn't the requestd number of contiguous blocks available), and when
EXT4_GET_BLOCKS_CREATE is not set.

					- 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
Zheng Liu March 11, 2013, 5:47 a.m. UTC | #3 
On Sun, Mar 10, 2013 at 09:07:18PM -0400, Theodore Ts'o wrote:
> On Wed, Mar 06, 2013 at 10:17:13PM +0800, Zheng Liu wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> > 
> > We always assume that the return value of ext4_ext_map_blocks is equal
> > to map->m_len
> 
> Note that in general, this is _never_ safe to assume.  There are a
> number of times when the number of blocks mapped is less than what the
> caller originally requested, both when allocating blocks (and there
> isn't the requestd number of contiguous blocks available), and when
> EXT4_GET_BLOCKS_CREATE is not set.

Yes, When EXT4_GET_BLOCKS_CREATE is not set, it could be 0 because there
is no block mapping, and we don't create it.  Meanwhile when we want to
allocate some blocks, it could be less than the number of block we
requested.  But IMHO at least when we try to allocate some blocks, m_len
should be changed according to the number of allocated blocks in order
to make them equal if the number of allocated blocks is less than the
number of blocks we requested.  Namely, when the return value (retval)
is greater than 0, this assumption will be right.  Because we will use
m_len value after map_blocks function returns.  We need to let upper
level know it, such as write_begin, DIO, etc...  Am I miss something?

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
Theodore Ts'o March 13, 2013, 1:57 a.m. UTC | #4 
On Mon, Mar 11, 2013 at 01:47:07PM +0800, Zheng Liu wrote:
> On Sun, Mar 10, 2013 at 09:07:18PM -0400, Theodore Ts'o wrote:
> > On Wed, Mar 06, 2013 at 10:17:13PM +0800, Zheng Liu wrote:
> > > From: Zheng Liu <wenqing.lz@taobao.com>
> > > 
> > > We always assume that the return value of ext4_ext_map_blocks is equal
> > > to map->m_len
> > 
> > Note that in general, this is _never_ safe to assume.  There are a
> > number of times when the number of blocks mapped is less than what the
> > caller originally requested, both when allocating blocks (and there
> > isn't the requestd number of contiguous blocks available), and when
> > EXT4_GET_BLOCKS_CREATE is not set.
> 
> Yes, When EXT4_GET_BLOCKS_CREATE is not set, it could be 0 because there
> is no block mapping, and we don't create it.  Meanwhile when we want to
> allocate some blocks, it could be less than the number of block we
> requested.  But IMHO at least when we try to allocate some blocks, m_len
> should be changed according to the number of allocated blocks in order
> to make them equal if the number of allocated blocks is less than the
> number of blocks we requested.  Namely, when the return value (retval)
> is greater than 0, this assumption will be right.  Because we will use
> m_len value after map_blocks function returns.  We need to let upper
> level know it, such as write_begin, DIO, etc...  Am I miss something?

No, you didn't miss anything.  I just wanted to say point out that any
assumption that ext4_ext_map_blocks() is equial to map->m_len was
always wrong, and not something that recently changed.  I updated the
commit description lightly to make this clear.

Regards,

						- 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
Theodore Ts'o March 13, 2013, 2:14 a.m. UTC | #5 
Oh, now I see my confusion.  You're talking about retval being
different from the output of map->m_len.  I was talking about retval
being often different from the *input* value of map->m_len.

I agree that the output of ext4_ext_map_blocks() and
ext4_ind_map_blocks() should be the same as map->m_len, and having a
BUG_ON there would make sense.  But we can save those changes for
after -rc3 as a cleanup....

					- 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
Zheng Liu March 13, 2013, 8:53 a.m. UTC | #6 
On Tue, Mar 12, 2013 at 10:14:20PM -0400, Theodore Ts'o wrote:
> Oh, now I see my confusion.  You're talking about retval being
> different from the output of map->m_len.  I was talking about retval
> being often different from the *input* value of map->m_len.

Ah, sorry for my bad expression. :-(

> 
> I agree that the output of ext4_ext_map_blocks() and
> ext4_ind_map_blocks() should be the same as map->m_len, and having a
> BUG_ON there would make sense.  But we can save those changes for
> after -rc3 as a cleanup....

Actually, in my tree I use a WARN_ON() because I don't want to throw a
kernel panic for user.  I think it is too unfriendly.  I will send a
patch to change it after -rc3.

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

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 25c86aa..110e85a 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3650,6 +3650,10 @@  ext4_ext_handle_uninitialized_extents(handle_t *handle, struct inode *inode,
 						 path, map->m_len);
 		} else
 			err = ret;
+		map->m_flags |= EXT4_MAP_MAPPED;
+		if (allocated > map->m_len)
+			allocated = map->m_len;
+		map->m_len = allocated;
 		goto out2;
 	}
 	/* buffered IO case */