| Message ID | 1362579435-6333-3-git-send-email-wenqing.lz@taobao.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wed, 6 Mar 2013 22:17:12 +0800, Zheng Liu <gnehzuil.liu@gmail.com> wrote: > From: Dmitry Monakhov <dmonakhov@openvz.org> Looks good with small comments (see below) > > This commit adds a self-testing infrastructure like extent tree does to > do a sanity check for extent status tree. After status tree is as a > extent cache, we'd better to make sure that it caches right result. > > After applied this commit, we will get a lot of messages when we run > xfstests as below. > > ... > kernel: ES len assertation failed for inode: 230 retval 1 != map->m_len > 3 in ext4_map_blocks (allocation) > ... > kernel: ES cache assertation failed for inode: 230 es_cached ex > [974/2/4781/20] != found ex [974/1/4781/1000] > ... > kernel: ES insert assertation failed for inode: 635 ex_status > [0/45/21388/w] != es_status [44/1/21432/u] > ... > > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> > Cc: "Theodore Ts'o" <tytso@mit.edu> > --- > fs/ext4/extents_status.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > fs/ext4/extents_status.h | 6 ++ > fs/ext4/inode.c | 96 +++++++++++++++++++++++++++ > 3 files changed, 271 insertions(+) > > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c > index dc4e4c5..a434f81 100644 > --- a/fs/ext4/extents_status.c > +++ b/fs/ext4/extents_status.c > @@ -405,6 +405,171 @@ ext4_es_try_to_merge_right(struct inode *inode, struct extent_status *es) > return es; > } > > +#ifdef ES_AGGRESSIVE_TEST > +static void ext4_es_insert_extent_ext_check(struct inode *inode, > + struct extent_status *es) > +{ > + struct ext4_ext_path *path = NULL; > + struct ext4_extent *ex; > + ext4_lblk_t ee_block; > + ext4_fsblk_t ee_start; > + unsigned short ee_len; > + int depth, ee_status, es_status; > + > + path = ext4_ext_find_extent(inode, es->es_lblk, NULL); > + if (IS_ERR(path)) > + return; > + > + depth = ext_depth(inode); > + ex = path[depth].p_ext; > + > + if (ex) { > + > + ee_block = le32_to_cpu(ex->ee_block); > + ee_start = ext4_ext_pblock(ex); > + ee_len = ext4_ext_get_actual_len(ex); > + > + ee_status = ext4_ext_is_uninitialized(ex) ? 1 : 0; > + es_status = ext4_es_is_unwritten(es) ? 1 : 0; > + > + /* > + * Make sure ex and es are not overlap when we try to insert > + * a delayed/hole extent. > + */ > + if (!ext4_es_is_written(es) && !ext4_es_is_unwritten(es)) { > + if (in_range(es->es_lblk, ee_block, ee_len)) { > + printk("ES insert assertation failed for inode: %lu " > + "we can find an extent at block " > + "[%d/%d/%llu/%c], but we want to add an " > + "delayed/hole extent [%d/%d/%llu/%llx]\n", > + inode->i_ino, ee_block, ee_len, ee_start, > + ee_status ? 'u' : 'w', es->es_lblk, es->es_len, > + ext4_es_pblock(es), ext4_es_status(es)); > + } > + goto out; > + } > + > + /* > + * We don't check ee_block == es->es_lblk, etc. because es > + * might be a part of whole extent, vice versa. > + */ > + if (es->es_lblk < ee_block || > + ext4_es_pblock(es) != ee_start + es->es_lblk - ee_block) { > + printk("ES insert assertation failed for inode: %lu " > + "ex_status [%d/%d/%llu/%c] != " > + "es_status [%d/%d/%llu/%c]\n", inode->i_ino, > + ee_block, ee_len, ee_start, ee_status ? 'u' : 'w', > + es->es_lblk, es->es_len, ext4_es_pblock(es), > + es_status ? 'u' : 'w'); > + goto out; > + } > + > + if (ee_status ^ es_status) { > + printk("ES insert assertation failed for inode: %lu " > + "ex_status [%d/%d/%llu/%c] != " > + "es_status [%d/%d/%llu/%c]\n", inode->i_ino, > + ee_block, ee_len, ee_start, ee_status ? 'u' : 'w', > + es->es_lblk, es->es_len, ext4_es_pblock(es), > + es_status ? 'u' : 'w'); > + } > + } else { > + /* > + * We can't find an extent on disk. So we need to make sure > + * that we don't want to add an written/unwritten extent. > + */ > + if (!ext4_es_is_delayed(es) && !ext4_es_is_hole(es)) { > + printk("ES insert assertation failed for inode: %lu " > + "can't find an extent at block %d but we want " > + "to add an written/unwritten extent " > + "[%d/%d/%llu/%llx]\n", inode->i_ino, > + es->es_lblk, es->es_lblk, es->es_len, > + ext4_es_pblock(es), ext4_es_status(es)); > + } > + } > +out: > + if (path) { > + ext4_ext_drop_refs(path); > + kfree(path); > + } > +} > + > +static void ext4_es_insert_extent_ind_check(struct inode *inode, > + struct extent_status *es) > +{ > + struct ext4_map_blocks map; > + int retval; > + > + /* > + * Here we call ext4_ind_map_blocks to lookup a block mapping because > + * 'Indirect' structure is defined in indirect.c. So we couldn't > + * access direct/indirect tree from outside. It is too dirty to define > + * this function in indirect.c file. > + */ > + > + map.m_lblk = es->es_lblk; > + map.m_len = es->es_len; > + > + retval = ext4_ind_map_blocks(NULL, inode, &map, 0); > + if (retval > 0) { > + if (ext4_es_is_delayed(es) || ext4_es_is_hole(es)) { > + /* > + * We want to add a delayed/hole extent but this > + * block has been allocated. > + */ > + printk("ES insert assertation failed for inode: %lu " > + "We can find blocks but we want to add a " > + "delayed/hole extent [%d/%d/%llu/%llx]\n", > + inode->i_ino, es->es_lblk, es->es_len, > + ext4_es_pblock(es), ext4_es_status(es)); > + return; > + } else if (ext4_es_is_written(es)) { > + if (retval != es->es_len) { > + printk("ES insert assertation failed for inode: " > + "%lu retval %d != es_len %d\n", > + inode->i_ino, retval, es->es_len); > + return; > + } > + if (map.m_pblk != ext4_es_pblock(es)) { > + printk("ES insert assertation failed for inode: " > + "%lu m_pblk %llu != es_pblk %llu\n", > + inode->i_ino, map.m_pblk, > + ext4_es_pblock(es)); > + return; > + } > + } else { > + /* > + * We don't need to check unwritten extent because > + * indirect-based file doesn't have it. > + */ > + BUG_ON(1); > + } > + } else if (retval == 0) { > + if (ext4_es_is_written(es)) { > + printk("ES insert assertation failed for inode: %lu " > + "We can't find the block but we want to add " > + "an written extent [%d/%d/%llu/%llx]\n", > + inode->i_ino, es->es_lblk, es->es_len, > + ext4_es_pblock(es), ext4_es_status(es)); > + return; > + } > + } > +} > + > +static inline void ext4_es_insert_extent_check(struct inode *inode, > + struct extent_status *es) > +{ > + /* > + * We don't need to worry about the race condition because > + * caller takes i_data_sem locking. > + */ > + BUG_ON(!rwsem_is_locked(&EXT4_I(inode)->i_data_sem)); > + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) > + ext4_es_insert_extent_ext_check(inode, es); > + else > + ext4_es_insert_extent_ind_check(inode, es); > +} > +#endif > + > static int __es_insert_extent(struct inode *inode, struct extent_status *newes) > { > struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree; > @@ -487,6 +652,10 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, > ext4_es_store_status(&newes, status); > trace_ext4_es_insert_extent(inode, &newes); > > +#ifdef ES_AGGRESSIVE_TEST > + ext4_es_insert_extent_check(inode, &newes); > +#endif We can avoid #ifdef here simply by defining ext4_es_insert_extent_check like follows: #ifdef ES_AGGRESSIVE_TEST void ext4_es_insert_extent_check(inode, &newes) { ... our stuff here } #elseif #define ext4_es_insert_extent_check(a, b) do {} while(0) #endif > +#endif > + > write_lock(&EXT4_I(inode)->i_es_lock); > err = __es_remove_extent(inode, lblk, end); > if (err != 0) > diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h > index f190dfe..56140ad 100644 > --- a/fs/ext4/extents_status.h > +++ b/fs/ext4/extents_status.h > @@ -21,6 +21,12 @@ > #endif > > /* > + * With ES_AGGRESSIVE_TEST defined, the result of es caching will be > + * checked with old map_block's result. > + */ > +#define ES_AGGRESSIVE_TEST__ > + > +/* > * These flags live in the high bits of extent_status.es_pblk > */ > #define EXTENT_STATUS_WRITTEN (1ULL << 63) > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 95a0c62..3186a43 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -482,6 +482,58 @@ static pgoff_t ext4_num_dirty_pages(struct inode *inode, pgoff_t idx, > return num; > } > > +#ifdef ES_AGGRESSIVE_TEST > +static void ext4_map_blocks_es_recheck(handle_t *handle, > + struct inode *inode, > + struct ext4_map_blocks *es_map, > + struct ext4_map_blocks *map, > + int flags) > +{ > + int retval; > + > + map->m_flags = 0; > + /* > + * There is a race window that the result is not the same. > + * e.g. xfstests #223 when dioread_nolock enables. The reason > + * is that we lookup a block mapping in extent status tree with > + * out taking i_data_sem. So at the time the unwritten extent > + * could be converted. > + */ > + if (!(flags & EXT4_GET_BLOCKS_NO_LOCK)) > + down_read((&EXT4_I(inode)->i_data_sem)); > + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { > + retval = ext4_ext_map_blocks(handle, inode, map, flags & > + EXT4_GET_BLOCKS_KEEP_SIZE); > + } else { > + retval = ext4_ind_map_blocks(handle, inode, map, flags & > + EXT4_GET_BLOCKS_KEEP_SIZE); > + } > + if (!(flags & EXT4_GET_BLOCKS_NO_LOCK)) > + up_read((&EXT4_I(inode)->i_data_sem)); > + /* > + * Clear EXT4_MAP_FROM_CLUSTER and EXT4_MAP_BOUNDARY flag > + * because it shouldn't be marked in es_map->m_flags. > + */ > + map->m_flags &= ~(EXT4_MAP_FROM_CLUSTER | EXT4_MAP_BOUNDARY); > + > + /* > + * We don't check m_len because extent will be collpased in status > + * tree. So the m_len might not equal. > + */ > + if (es_map->m_lblk != map->m_lblk || > + es_map->m_flags != map->m_flags || > + es_map->m_pblk != map->m_pblk) { > + printk("ES cache assertation failed for inode: %lu " > + "es_cached ex [%d/%d/%llu/%x] != " > + "found ex [%d/%d/%llu/%x] retval %d flags %x\n", > + inode->i_ino, es_map->m_lblk, es_map->m_len, > + es_map->m_pblk, es_map->m_flags, map->m_lblk, > + map->m_len, map->m_pblk, map->m_flags, > + retval, flags); > + } > +} > +#endif /* ES_AGGRESSIVE_TEST */ > + > /* > * The ext4_map_blocks() function tries to look up the requested blocks, > * and returns if the blocks are already mapped. > @@ -509,6 +561,11 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, > { > struct extent_status es; > int retval; > +#ifdef ES_AGGRESSIVE_TEST > + struct ext4_map_blocks orig_map; > + > + memcpy(&orig_map, map, sizeof(*map)); > +#endif > > map->m_flags = 0; > ext_debug("ext4_map_blocks(): inode %lu, flag %d, max_blocks %u," > @@ -531,6 +588,10 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, > } else { > BUG_ON(1); > } > +#ifdef ES_AGGRESSIVE_TEST > + ext4_map_blocks_es_recheck(handle, inode, map, > + &orig_map, flags); > +#endif > goto found; > } > > @@ -551,6 +612,15 @@ 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 It is reasonable to replace this by one-line check BUG_ON(retval != map->m_len) > + > status = map->m_flags & EXT4_MAP_UNWRITTEN ? > EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; > if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) && > @@ -643,6 +713,15 @@ 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 Also one line check BUG_ON(retval != map->m_len) > + > status = map->m_flags & EXT4_MAP_UNWRITTEN ? > EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; > if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) && > @@ -1768,6 +1847,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, > struct extent_status es; > int retval; > sector_t invalid_block = ~((sector_t) 0xffff); > +#ifdef ES_AGGRESSIVE_TEST > + struct ext4_map_blocks orig_map; > + > + memcpy(&orig_map, map, sizeof(*map)); > +#endif > > if (invalid_block < ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es)) > invalid_block = ~0; > @@ -1809,6 +1893,9 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, > else > BUG_ON(1); > > +#ifdef ES_AGGRESSIVE_TEST > + ext4_map_blocks_es_recheck(NULL, inode, map, &orig_map, 0); > +#endif > return retval; > } > > @@ -1873,6 +1960,15 @@ 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 > + > status = map->m_flags & EXT4_MAP_UNWRITTEN ? > EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; > ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len, > -- > 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
On Thu, Mar 07, 2013 at 07:41:05PM +0400, Dmitry Monakhov wrote: > On Wed, 6 Mar 2013 22:17:12 +0800, Zheng Liu <gnehzuil.liu@gmail.com> wrote: > > From: Dmitry Monakhov <dmonakhov@openvz.org> > Looks good with small comments (see below) Yes, I will fix them in next version. Thanks for your comments. Regards, - Zheng > > > > This commit adds a self-testing infrastructure like extent tree does to > > do a sanity check for extent status tree. After status tree is as a > > extent cache, we'd better to make sure that it caches right result. > > > > After applied this commit, we will get a lot of messages when we run > > xfstests as below. > > > > ... > > kernel: ES len assertation failed for inode: 230 retval 1 != map->m_len > > 3 in ext4_map_blocks (allocation) > > ... > > kernel: ES cache assertation failed for inode: 230 es_cached ex > > [974/2/4781/20] != found ex [974/1/4781/1000] > > ... > > kernel: ES insert assertation failed for inode: 635 ex_status > > [0/45/21388/w] != es_status [44/1/21432/u] > > ... > > > > Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org> > > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> > > Cc: "Theodore Ts'o" <tytso@mit.edu> > > --- > > fs/ext4/extents_status.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++ > > fs/ext4/extents_status.h | 6 ++ > > fs/ext4/inode.c | 96 +++++++++++++++++++++++++++ > > 3 files changed, 271 insertions(+) > > > > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c > > index dc4e4c5..a434f81 100644 > > --- a/fs/ext4/extents_status.c > > +++ b/fs/ext4/extents_status.c > > @@ -405,6 +405,171 @@ ext4_es_try_to_merge_right(struct inode *inode, struct extent_status *es) > > return es; > > } > > > > +#ifdef ES_AGGRESSIVE_TEST > > +static void ext4_es_insert_extent_ext_check(struct inode *inode, > > + struct extent_status *es) > > +{ > > + struct ext4_ext_path *path = NULL; > > + struct ext4_extent *ex; > > + ext4_lblk_t ee_block; > > + ext4_fsblk_t ee_start; > > + unsigned short ee_len; > > + int depth, ee_status, es_status; > > + > > + path = ext4_ext_find_extent(inode, es->es_lblk, NULL); > > + if (IS_ERR(path)) > > + return; > > + > > + depth = ext_depth(inode); > > + ex = path[depth].p_ext; > > + > > + if (ex) { > > + > > + ee_block = le32_to_cpu(ex->ee_block); > > + ee_start = ext4_ext_pblock(ex); > > + ee_len = ext4_ext_get_actual_len(ex); > > + > > + ee_status = ext4_ext_is_uninitialized(ex) ? 1 : 0; > > + es_status = ext4_es_is_unwritten(es) ? 1 : 0; > > + > > + /* > > + * Make sure ex and es are not overlap when we try to insert > > + * a delayed/hole extent. > > + */ > > + if (!ext4_es_is_written(es) && !ext4_es_is_unwritten(es)) { > > + if (in_range(es->es_lblk, ee_block, ee_len)) { > > + printk("ES insert assertation failed for inode: %lu " > > + "we can find an extent at block " > > + "[%d/%d/%llu/%c], but we want to add an " > > + "delayed/hole extent [%d/%d/%llu/%llx]\n", > > + inode->i_ino, ee_block, ee_len, ee_start, > > + ee_status ? 'u' : 'w', es->es_lblk, es->es_len, > > + ext4_es_pblock(es), ext4_es_status(es)); > > + } > > + goto out; > > + } > > + > > + /* > > + * We don't check ee_block == es->es_lblk, etc. because es > > + * might be a part of whole extent, vice versa. > > + */ > > + if (es->es_lblk < ee_block || > > + ext4_es_pblock(es) != ee_start + es->es_lblk - ee_block) { > > + printk("ES insert assertation failed for inode: %lu " > > + "ex_status [%d/%d/%llu/%c] != " > > + "es_status [%d/%d/%llu/%c]\n", inode->i_ino, > > + ee_block, ee_len, ee_start, ee_status ? 'u' : 'w', > > + es->es_lblk, es->es_len, ext4_es_pblock(es), > > + es_status ? 'u' : 'w'); > > + goto out; > > + } > > + > > + if (ee_status ^ es_status) { > > + printk("ES insert assertation failed for inode: %lu " > > + "ex_status [%d/%d/%llu/%c] != " > > + "es_status [%d/%d/%llu/%c]\n", inode->i_ino, > > + ee_block, ee_len, ee_start, ee_status ? 'u' : 'w', > > + es->es_lblk, es->es_len, ext4_es_pblock(es), > > + es_status ? 'u' : 'w'); > > + } > > + } else { > > + /* > > + * We can't find an extent on disk. So we need to make sure > > + * that we don't want to add an written/unwritten extent. > > + */ > > + if (!ext4_es_is_delayed(es) && !ext4_es_is_hole(es)) { > > + printk("ES insert assertation failed for inode: %lu " > > + "can't find an extent at block %d but we want " > > + "to add an written/unwritten extent " > > + "[%d/%d/%llu/%llx]\n", inode->i_ino, > > + es->es_lblk, es->es_lblk, es->es_len, > > + ext4_es_pblock(es), ext4_es_status(es)); > > + } > > + } > > +out: > > + if (path) { > > + ext4_ext_drop_refs(path); > > + kfree(path); > > + } > > +} > > + > > +static void ext4_es_insert_extent_ind_check(struct inode *inode, > > + struct extent_status *es) > > +{ > > + struct ext4_map_blocks map; > > + int retval; > > + > > + /* > > + * Here we call ext4_ind_map_blocks to lookup a block mapping because > > + * 'Indirect' structure is defined in indirect.c. So we couldn't > > + * access direct/indirect tree from outside. It is too dirty to define > > + * this function in indirect.c file. > > + */ > > + > > + map.m_lblk = es->es_lblk; > > + map.m_len = es->es_len; > > + > > + retval = ext4_ind_map_blocks(NULL, inode, &map, 0); > > + if (retval > 0) { > > + if (ext4_es_is_delayed(es) || ext4_es_is_hole(es)) { > > + /* > > + * We want to add a delayed/hole extent but this > > + * block has been allocated. > > + */ > > + printk("ES insert assertation failed for inode: %lu " > > + "We can find blocks but we want to add a " > > + "delayed/hole extent [%d/%d/%llu/%llx]\n", > > + inode->i_ino, es->es_lblk, es->es_len, > > + ext4_es_pblock(es), ext4_es_status(es)); > > + return; > > + } else if (ext4_es_is_written(es)) { > > + if (retval != es->es_len) { > > + printk("ES insert assertation failed for inode: " > > + "%lu retval %d != es_len %d\n", > > + inode->i_ino, retval, es->es_len); > > + return; > > + } > > + if (map.m_pblk != ext4_es_pblock(es)) { > > + printk("ES insert assertation failed for inode: " > > + "%lu m_pblk %llu != es_pblk %llu\n", > > + inode->i_ino, map.m_pblk, > > + ext4_es_pblock(es)); > > + return; > > + } > > + } else { > > + /* > > + * We don't need to check unwritten extent because > > + * indirect-based file doesn't have it. > > + */ > > + BUG_ON(1); > > + } > > + } else if (retval == 0) { > > + if (ext4_es_is_written(es)) { > > + printk("ES insert assertation failed for inode: %lu " > > + "We can't find the block but we want to add " > > + "an written extent [%d/%d/%llu/%llx]\n", > > + inode->i_ino, es->es_lblk, es->es_len, > > + ext4_es_pblock(es), ext4_es_status(es)); > > + return; > > + } > > + } > > +} > > + > > +static inline void ext4_es_insert_extent_check(struct inode *inode, > > + struct extent_status *es) > > +{ > > + /* > > + * We don't need to worry about the race condition because > > + * caller takes i_data_sem locking. > > + */ > > + BUG_ON(!rwsem_is_locked(&EXT4_I(inode)->i_data_sem)); > > + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) > > + ext4_es_insert_extent_ext_check(inode, es); > > + else > > + ext4_es_insert_extent_ind_check(inode, es); > > +} > > +#endif > > + > > static int __es_insert_extent(struct inode *inode, struct extent_status *newes) > > { > > struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree; > > @@ -487,6 +652,10 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, > > ext4_es_store_status(&newes, status); > > trace_ext4_es_insert_extent(inode, &newes); > > > > +#ifdef ES_AGGRESSIVE_TEST > > + ext4_es_insert_extent_check(inode, &newes); > > +#endif > We can avoid #ifdef here simply by defining ext4_es_insert_extent_check > like follows: > #ifdef ES_AGGRESSIVE_TEST > void ext4_es_insert_extent_check(inode, &newes) { > ... our stuff here > } > #elseif > #define ext4_es_insert_extent_check(a, b) do {} while(0) > #endif > > > +#endif > > + > > write_lock(&EXT4_I(inode)->i_es_lock); > > err = __es_remove_extent(inode, lblk, end); > > if (err != 0) > > diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h > > index f190dfe..56140ad 100644 > > --- a/fs/ext4/extents_status.h > > +++ b/fs/ext4/extents_status.h > > @@ -21,6 +21,12 @@ > > #endif > > > > /* > > + * With ES_AGGRESSIVE_TEST defined, the result of es caching will be > > + * checked with old map_block's result. > > + */ > > +#define ES_AGGRESSIVE_TEST__ > > + > > +/* > > * These flags live in the high bits of extent_status.es_pblk > > */ > > #define EXTENT_STATUS_WRITTEN (1ULL << 63) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > > index 95a0c62..3186a43 100644 > > --- a/fs/ext4/inode.c > > +++ b/fs/ext4/inode.c > > @@ -482,6 +482,58 @@ static pgoff_t ext4_num_dirty_pages(struct inode *inode, pgoff_t idx, > > return num; > > } > > > > +#ifdef ES_AGGRESSIVE_TEST > > +static void ext4_map_blocks_es_recheck(handle_t *handle, > > + struct inode *inode, > > + struct ext4_map_blocks *es_map, > > + struct ext4_map_blocks *map, > > + int flags) > > +{ > > + int retval; > > + > > + map->m_flags = 0; > > + /* > > + * There is a race window that the result is not the same. > > + * e.g. xfstests #223 when dioread_nolock enables. The reason > > + * is that we lookup a block mapping in extent status tree with > > + * out taking i_data_sem. So at the time the unwritten extent > > + * could be converted. > > + */ > > + if (!(flags & EXT4_GET_BLOCKS_NO_LOCK)) > > + down_read((&EXT4_I(inode)->i_data_sem)); > > + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { > > + retval = ext4_ext_map_blocks(handle, inode, map, flags & > > + EXT4_GET_BLOCKS_KEEP_SIZE); > > + } else { > > + retval = ext4_ind_map_blocks(handle, inode, map, flags & > > + EXT4_GET_BLOCKS_KEEP_SIZE); > > + } > > + if (!(flags & EXT4_GET_BLOCKS_NO_LOCK)) > > + up_read((&EXT4_I(inode)->i_data_sem)); > > + /* > > + * Clear EXT4_MAP_FROM_CLUSTER and EXT4_MAP_BOUNDARY flag > > + * because it shouldn't be marked in es_map->m_flags. > > + */ > > + map->m_flags &= ~(EXT4_MAP_FROM_CLUSTER | EXT4_MAP_BOUNDARY); > > + > > + /* > > + * We don't check m_len because extent will be collpased in status > > + * tree. So the m_len might not equal. > > + */ > > + if (es_map->m_lblk != map->m_lblk || > > + es_map->m_flags != map->m_flags || > > + es_map->m_pblk != map->m_pblk) { > > + printk("ES cache assertation failed for inode: %lu " > > + "es_cached ex [%d/%d/%llu/%x] != " > > + "found ex [%d/%d/%llu/%x] retval %d flags %x\n", > > + inode->i_ino, es_map->m_lblk, es_map->m_len, > > + es_map->m_pblk, es_map->m_flags, map->m_lblk, > > + map->m_len, map->m_pblk, map->m_flags, > > + retval, flags); > > + } > > +} > > +#endif /* ES_AGGRESSIVE_TEST */ > > + > > /* > > * The ext4_map_blocks() function tries to look up the requested blocks, > > * and returns if the blocks are already mapped. > > @@ -509,6 +561,11 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, > > { > > struct extent_status es; > > int retval; > > +#ifdef ES_AGGRESSIVE_TEST > > + struct ext4_map_blocks orig_map; > > + > > + memcpy(&orig_map, map, sizeof(*map)); > > +#endif > > > > map->m_flags = 0; > > ext_debug("ext4_map_blocks(): inode %lu, flag %d, max_blocks %u," > > @@ -531,6 +588,10 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, > > } else { > > BUG_ON(1); > > } > > +#ifdef ES_AGGRESSIVE_TEST > > + ext4_map_blocks_es_recheck(handle, inode, map, > > + &orig_map, flags); > > +#endif > > goto found; > > } > > > > @@ -551,6 +612,15 @@ 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 > It is reasonable to replace this by one-line check > BUG_ON(retval != map->m_len) > > + > > status = map->m_flags & EXT4_MAP_UNWRITTEN ? > > EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; > > if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) && > > @@ -643,6 +713,15 @@ 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 > Also one line check BUG_ON(retval != map->m_len) > > + > > status = map->m_flags & EXT4_MAP_UNWRITTEN ? > > EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; > > if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) && > > @@ -1768,6 +1847,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, > > struct extent_status es; > > int retval; > > sector_t invalid_block = ~((sector_t) 0xffff); > > +#ifdef ES_AGGRESSIVE_TEST > > + struct ext4_map_blocks orig_map; > > + > > + memcpy(&orig_map, map, sizeof(*map)); > > +#endif > > > > if (invalid_block < ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es)) > > invalid_block = ~0; > > @@ -1809,6 +1893,9 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, > > else > > BUG_ON(1); > > > > +#ifdef ES_AGGRESSIVE_TEST > > + ext4_map_blocks_es_recheck(NULL, inode, map, &orig_map, 0); > > +#endif > > return retval; > > } > > > > @@ -1873,6 +1960,15 @@ 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 > > + > > status = map->m_flags & EXT4_MAP_UNWRITTEN ? > > EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; > > ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len, > > -- > > 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
On Wed, Mar 06, 2013 at 10:17:12PM +0800, Zheng Liu wrote: > + printk("ES insert assertation failed for inode: %lu " > + "we can find an extent at block " FYI, I changed all of the printk() messages to pr_warn(). - 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
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c index dc4e4c5..a434f81 100644 --- a/fs/ext4/extents_status.c +++ b/fs/ext4/extents_status.c @@ -405,6 +405,171 @@ ext4_es_try_to_merge_right(struct inode *inode, struct extent_status *es) return es; } +#ifdef ES_AGGRESSIVE_TEST +static void ext4_es_insert_extent_ext_check(struct inode *inode, + struct extent_status *es) +{ + struct ext4_ext_path *path = NULL; + struct ext4_extent *ex; + ext4_lblk_t ee_block; + ext4_fsblk_t ee_start; + unsigned short ee_len; + int depth, ee_status, es_status; + + path = ext4_ext_find_extent(inode, es->es_lblk, NULL); + if (IS_ERR(path)) + return; + + depth = ext_depth(inode); + ex = path[depth].p_ext; + + if (ex) { + + ee_block = le32_to_cpu(ex->ee_block); + ee_start = ext4_ext_pblock(ex); + ee_len = ext4_ext_get_actual_len(ex); + + ee_status = ext4_ext_is_uninitialized(ex) ? 1 : 0; + es_status = ext4_es_is_unwritten(es) ? 1 : 0; + + /* + * Make sure ex and es are not overlap when we try to insert + * a delayed/hole extent. + */ + if (!ext4_es_is_written(es) && !ext4_es_is_unwritten(es)) { + if (in_range(es->es_lblk, ee_block, ee_len)) { + printk("ES insert assertation failed for inode: %lu " + "we can find an extent at block " + "[%d/%d/%llu/%c], but we want to add an " + "delayed/hole extent [%d/%d/%llu/%llx]\n", + inode->i_ino, ee_block, ee_len, ee_start, + ee_status ? 'u' : 'w', es->es_lblk, es->es_len, + ext4_es_pblock(es), ext4_es_status(es)); + } + goto out; + } + + /* + * We don't check ee_block == es->es_lblk, etc. because es + * might be a part of whole extent, vice versa. + */ + if (es->es_lblk < ee_block || + ext4_es_pblock(es) != ee_start + es->es_lblk - ee_block) { + printk("ES insert assertation failed for inode: %lu " + "ex_status [%d/%d/%llu/%c] != " + "es_status [%d/%d/%llu/%c]\n", inode->i_ino, + ee_block, ee_len, ee_start, ee_status ? 'u' : 'w', + es->es_lblk, es->es_len, ext4_es_pblock(es), + es_status ? 'u' : 'w'); + goto out; + } + + if (ee_status ^ es_status) { + printk("ES insert assertation failed for inode: %lu " + "ex_status [%d/%d/%llu/%c] != " + "es_status [%d/%d/%llu/%c]\n", inode->i_ino, + ee_block, ee_len, ee_start, ee_status ? 'u' : 'w', + es->es_lblk, es->es_len, ext4_es_pblock(es), + es_status ? 'u' : 'w'); + } + } else { + /* + * We can't find an extent on disk. So we need to make sure + * that we don't want to add an written/unwritten extent. + */ + if (!ext4_es_is_delayed(es) && !ext4_es_is_hole(es)) { + printk("ES insert assertation failed for inode: %lu " + "can't find an extent at block %d but we want " + "to add an written/unwritten extent " + "[%d/%d/%llu/%llx]\n", inode->i_ino, + es->es_lblk, es->es_lblk, es->es_len, + ext4_es_pblock(es), ext4_es_status(es)); + } + } +out: + if (path) { + ext4_ext_drop_refs(path); + kfree(path); + } +} + +static void ext4_es_insert_extent_ind_check(struct inode *inode, + struct extent_status *es) +{ + struct ext4_map_blocks map; + int retval; + + /* + * Here we call ext4_ind_map_blocks to lookup a block mapping because + * 'Indirect' structure is defined in indirect.c. So we couldn't + * access direct/indirect tree from outside. It is too dirty to define + * this function in indirect.c file. + */ + + map.m_lblk = es->es_lblk; + map.m_len = es->es_len; + + retval = ext4_ind_map_blocks(NULL, inode, &map, 0); + if (retval > 0) { + if (ext4_es_is_delayed(es) || ext4_es_is_hole(es)) { + /* + * We want to add a delayed/hole extent but this + * block has been allocated. + */ + printk("ES insert assertation failed for inode: %lu " + "We can find blocks but we want to add a " + "delayed/hole extent [%d/%d/%llu/%llx]\n", + inode->i_ino, es->es_lblk, es->es_len, + ext4_es_pblock(es), ext4_es_status(es)); + return; + } else if (ext4_es_is_written(es)) { + if (retval != es->es_len) { + printk("ES insert assertation failed for inode: " + "%lu retval %d != es_len %d\n", + inode->i_ino, retval, es->es_len); + return; + } + if (map.m_pblk != ext4_es_pblock(es)) { + printk("ES insert assertation failed for inode: " + "%lu m_pblk %llu != es_pblk %llu\n", + inode->i_ino, map.m_pblk, + ext4_es_pblock(es)); + return; + } + } else { + /* + * We don't need to check unwritten extent because + * indirect-based file doesn't have it. + */ + BUG_ON(1); + } + } else if (retval == 0) { + if (ext4_es_is_written(es)) { + printk("ES insert assertation failed for inode: %lu " + "We can't find the block but we want to add " + "an written extent [%d/%d/%llu/%llx]\n", + inode->i_ino, es->es_lblk, es->es_len, + ext4_es_pblock(es), ext4_es_status(es)); + return; + } + } +} + +static inline void ext4_es_insert_extent_check(struct inode *inode, + struct extent_status *es) +{ + /* + * We don't need to worry about the race condition because + * caller takes i_data_sem locking. + */ + BUG_ON(!rwsem_is_locked(&EXT4_I(inode)->i_data_sem)); + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) + ext4_es_insert_extent_ext_check(inode, es); + else + ext4_es_insert_extent_ind_check(inode, es); +} +#endif + static int __es_insert_extent(struct inode *inode, struct extent_status *newes) { struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree; @@ -487,6 +652,10 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk, ext4_es_store_status(&newes, status); trace_ext4_es_insert_extent(inode, &newes); +#ifdef ES_AGGRESSIVE_TEST + ext4_es_insert_extent_check(inode, &newes); +#endif + write_lock(&EXT4_I(inode)->i_es_lock); err = __es_remove_extent(inode, lblk, end); if (err != 0) diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h index f190dfe..56140ad 100644 --- a/fs/ext4/extents_status.h +++ b/fs/ext4/extents_status.h @@ -21,6 +21,12 @@ #endif /* + * With ES_AGGRESSIVE_TEST defined, the result of es caching will be + * checked with old map_block's result. + */ +#define ES_AGGRESSIVE_TEST__ + +/* * These flags live in the high bits of extent_status.es_pblk */ #define EXTENT_STATUS_WRITTEN (1ULL << 63) diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 95a0c62..3186a43 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -482,6 +482,58 @@ static pgoff_t ext4_num_dirty_pages(struct inode *inode, pgoff_t idx, return num; } +#ifdef ES_AGGRESSIVE_TEST +static void ext4_map_blocks_es_recheck(handle_t *handle, + struct inode *inode, + struct ext4_map_blocks *es_map, + struct ext4_map_blocks *map, + int flags) +{ + int retval; + + map->m_flags = 0; + /* + * There is a race window that the result is not the same. + * e.g. xfstests #223 when dioread_nolock enables. The reason + * is that we lookup a block mapping in extent status tree with + * out taking i_data_sem. So at the time the unwritten extent + * could be converted. + */ + if (!(flags & EXT4_GET_BLOCKS_NO_LOCK)) + down_read((&EXT4_I(inode)->i_data_sem)); + if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)) { + retval = ext4_ext_map_blocks(handle, inode, map, flags & + EXT4_GET_BLOCKS_KEEP_SIZE); + } else { + retval = ext4_ind_map_blocks(handle, inode, map, flags & + EXT4_GET_BLOCKS_KEEP_SIZE); + } + if (!(flags & EXT4_GET_BLOCKS_NO_LOCK)) + up_read((&EXT4_I(inode)->i_data_sem)); + /* + * Clear EXT4_MAP_FROM_CLUSTER and EXT4_MAP_BOUNDARY flag + * because it shouldn't be marked in es_map->m_flags. + */ + map->m_flags &= ~(EXT4_MAP_FROM_CLUSTER | EXT4_MAP_BOUNDARY); + + /* + * We don't check m_len because extent will be collpased in status + * tree. So the m_len might not equal. + */ + if (es_map->m_lblk != map->m_lblk || + es_map->m_flags != map->m_flags || + es_map->m_pblk != map->m_pblk) { + printk("ES cache assertation failed for inode: %lu " + "es_cached ex [%d/%d/%llu/%x] != " + "found ex [%d/%d/%llu/%x] retval %d flags %x\n", + inode->i_ino, es_map->m_lblk, es_map->m_len, + es_map->m_pblk, es_map->m_flags, map->m_lblk, + map->m_len, map->m_pblk, map->m_flags, + retval, flags); + } +} +#endif /* ES_AGGRESSIVE_TEST */ + /* * The ext4_map_blocks() function tries to look up the requested blocks, * and returns if the blocks are already mapped. @@ -509,6 +561,11 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, { struct extent_status es; int retval; +#ifdef ES_AGGRESSIVE_TEST + struct ext4_map_blocks orig_map; + + memcpy(&orig_map, map, sizeof(*map)); +#endif map->m_flags = 0; ext_debug("ext4_map_blocks(): inode %lu, flag %d, max_blocks %u," @@ -531,6 +588,10 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode, } else { BUG_ON(1); } +#ifdef ES_AGGRESSIVE_TEST + ext4_map_blocks_es_recheck(handle, inode, map, + &orig_map, flags); +#endif goto found; } @@ -551,6 +612,15 @@ 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 + status = map->m_flags & EXT4_MAP_UNWRITTEN ? EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) && @@ -643,6 +713,15 @@ 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 + status = map->m_flags & EXT4_MAP_UNWRITTEN ? EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; if (!(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) && @@ -1768,6 +1847,11 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, struct extent_status es; int retval; sector_t invalid_block = ~((sector_t) 0xffff); +#ifdef ES_AGGRESSIVE_TEST + struct ext4_map_blocks orig_map; + + memcpy(&orig_map, map, sizeof(*map)); +#endif if (invalid_block < ext4_blocks_count(EXT4_SB(inode->i_sb)->s_es)) invalid_block = ~0; @@ -1809,6 +1893,9 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock, else BUG_ON(1); +#ifdef ES_AGGRESSIVE_TEST + ext4_map_blocks_es_recheck(NULL, inode, map, &orig_map, 0); +#endif return retval; } @@ -1873,6 +1960,15 @@ 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 + status = map->m_flags & EXT4_MAP_UNWRITTEN ? EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN; ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len,