Message ID | 1362579435-6333-4-git-send-email-wenqing.lz@taobao.com |
---|---|
State | Accepted, archived |
Headers | show |
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
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
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
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
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
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 --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 */