Message ID | 1364118173-12354-1-git-send-email-wenqing.lz@taobao.com |
---|---|
State | Accepted, archived |
Headers | show |
On Sun, Mar 24, 2013 at 05:42:53PM +0800, Zheng Liu wrote: > From: Zheng Liu <wenqing.lz@taobao.com> > > In this commit (921f266b) a sanity check is added in map_blocks to make > sure 'retval == map->m_len'. But we need to define a macro to enable > it. This commit uses a WARN_ON to do the same thing. > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> You and Dmitry were the ones who using originally these these checks to fix the bugs here; if we think the code is clean enough that we don't need the debugging information with the inode number, length, etc., then sure, we could use the unconditionally defined WARN_ON(). If we wanted to be really paranoid and give ourselves the maximal amount of debugging information, we could of course do something like this: if (retval != map->m_len) { ext4_warning(inode->i_sb, "ES len assertation failed for inode: %lu retval %d != map->m_len %d\n", inode->i_ino, retval, map->m_len); WARN_ON(1); } This way, we get the stack dump, the file system device, and all of the debugging information. The tradeoff is we're bloating the code size a bit. The question is really how confident are we that we've found all of the potential bugs here. If we think that there's a chance we might trip this check in the future, sometimes it's good to print as much information as possible, especially if it's hard to create a reproduction on demand. What do you think? - 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 24, 2013 at 02:23:18PM -0400, Theodore Ts'o wrote: > On Sun, Mar 24, 2013 at 05:42:53PM +0800, Zheng Liu wrote: > > From: Zheng Liu <wenqing.lz@taobao.com> > > > > In this commit (921f266b) a sanity check is added in map_blocks to make > > sure 'retval == map->m_len'. But we need to define a macro to enable > > it. This commit uses a WARN_ON to do the same thing. > > > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> > > You and Dmitry were the ones who using originally these these checks > to fix the bugs here; Yes, I use this check to fix bug. > if we think the code is clean enough that we > don't need the debugging information with the inode number, length, > etc., then sure, we could use the unconditionally defined WARN_ON(). > > If we wanted to be really paranoid and give ourselves the maximal > amount of debugging information, we could of course do something like > this: > > if (retval != map->m_len) { > ext4_warning(inode->i_sb, "ES len assertation failed for inode: %lu retval %d != map->m_len %d\n", inode->i_ino, retval, > map->m_len); > WARN_ON(1); > } I think this is better. > > This way, we get the stack dump, the file system device, and all of > the debugging information. The tradeoff is we're bloating the code > size a bit. > > The question is really how confident are we that we've found all of > the potential bugs here. If we think that there's a chance we might > trip this check in the future, sometimes it's good to print as much > information as possible, especially if it's hard to create a > reproduction on demand. > > What do you think? In my sand box, after fixed the bug, I never see this warning again. But I do believe we'd better leave it here to give us an opportunity to fix some potential bugs. Thanks, - 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/inode.c b/fs/ext4/inode.c index 85e41a2..4513e9a 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -613,14 +613,7 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, int ret; unsigned long long status; -#ifdef ES_AGGRESSIVE_TEST - if (retval != map->m_len) { - printk("ES len assertation failed for inode: %lu " - "retval %d != map->m_len %d " - "in %s (lookup)\n", inode->i_ino, retval, - map->m_len, __func__); - } -#endif + WARN_ON(retval != map->m_len); status = map->m_flags & EXT4_MAP_UNWRITTEN ? EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; @@ -714,14 +707,7 @@ found: int ret; unsigned long long status; -#ifdef ES_AGGRESSIVE_TEST - if (retval != map->m_len) { - printk("ES len assertation failed for inode: %lu " - "retval %d != map->m_len %d " - "in %s (allocation)\n", inode->i_ino, retval, - map->m_len, __func__); - } -#endif + WARN_ON(retval != map->m_len); /* * If the extent has been zeroed out, we don't need to update @@ -2030,14 +2016,7 @@ add_delayed: int ret; unsigned long long status; -#ifdef ES_AGGRESSIVE_TEST - if (retval != map->m_len) { - printk("ES len assertation failed for inode: %lu " - "retval %d != map->m_len %d " - "in %s (lookup)\n", inode->i_ino, retval, - map->m_len, __func__); - } -#endif + WARN_ON(retval != map->m_len); status = map->m_flags & EXT4_MAP_UNWRITTEN ? EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;