On Fri, 1 Mar 2013 02:26:41 +0800, Zheng Liu <gnehzuil.liu@xxxxxxxxx> wrote:
> From: Dmitry Monakhov <dmonakhov@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Zheng Liu <wenqing.lz@xxxxxxxxxx>
> Cc: "Theodore Ts'o" <tytso@xxxxxxx>
> ---
> fs/ext4/extents_status.c | 64 ++++++++++++++++++++++++++++++++
> fs/ext4/extents_status.h | 6 +++
> fs/ext4/inode.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 165 insertions(+)
>
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index a769485..02df536 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -401,6 +401,66 @@ 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_check(struct inode *inode,
> + struct extent_status *es)
Agree, this is very good place for sanity check.
> +{
> + 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, ex_status, es_status;
> +
> + /*
> + * Delayed and hole haven't been allocated in disk. We
> + * don't need to check it.
> + */
Why? We still can perform ext4_ext_find_extent and check that:
/* If extent is delayed only if ex == NULL */
if (!ex == ext4_es_is_written(es))
WARN_ON();
if (ext4_es_is_unwritten(es) != ext4_ext_is_uninitialized(ex))
WARN_ON();
> + if (!ext4_es_is_written(es) && !ext4_es_is_unwritten(es))
> + return;
> +
> + /*
> + * We don't need to worry about the race condition because
> + * caller takes i_data_sem locking.
Yes, and it is good place to assertion that we hold it.
> + */
> + path = ext4_ext_find_extent(inode, es->es_lblk, NULL);
> + if (IS_ERR(path))
> + return;
> +
> + depth = ext_depth(inode);
> +
> + /* We should always find an extent at given logical block */
> + ex = path[depth].p_ext;
> + if ((ex != NULL ) ^ ext4_es_is_written(es) {
> + printk("ES insert assertation failed to inode: %lu "
> + "can not find the extent at block %d\n",
> + inode->i_ino, es->es_lblk);
> + goto out;
> + }
> +
> +> +
> + ee_block = le32_to_cpu(ex->ee_block);
> + ee_start = ext4_ext_pblock(ex);
> + ee_len = ext4_ext_get_actual_len(ex);
> +
> + ex_status = ext4_ext_is_uninitialized(ex) ? 1 : 0;
> + es_status = ext4_es_is_unwritten(es) ? 1 : 0;
> + if (ex_status ^ es_status) {
Why status only? we can also check pblock, and lblk.
> + 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, ex_status ? 'u' : 'w',
> + es->es_lblk, es->es_len, ext4_es_pblock(es),
> + es_status ? 'u' : 'w');
> + }
> +out:
> + if (path) {
> + ext4_ext_drop_refs(path);
> + kfree(path);
> + }
> +
> +}
> +#endif
> +
> static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
> {
> struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
> @@ -483,6 +543,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..8d22170 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -482,6 +482,57 @@ 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 flag because it shouldn't be marked
> + * in es_map->m_flags.
> + */
> + map->m_flags &= ~EXT4_MAP_FROM_CLUSTER;
> +
> + /*
> + * 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]\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);
> + }
> +}
> +#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 +560,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 +587,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 +611,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 +712,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 +1846,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 +1892,9 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
> else
> BUG_ON(1);
>
> +#ifdef ES_AGRESSIVE_TEST
> + ext4_map_blocks_es_recheck(NULL, inode, map, &orig_map, flags);
> +#endif
> return retval;
> }
>
> @@ -1873,6 +1959,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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
