From: Zheng Liu <gnehzuil.liu@gmail.com>
To: Dmitry Monakhov <dmonakhov@openvz.org>
Cc: linux-ext4@vger.kernel.org, Zheng Liu <wenqing.lz@taobao.com>,
Theodore Ts'o <tytso@mit.edu>
Subject: Re: [PATCH v2 2/5] ext4: add self-testing infrastructure to do a sanity check
Date: Fri, 8 Mar 2013 21:01:02 +0800 [thread overview]
Message-ID: <20130308130102.GB18986@gmail.com> (raw)
In-Reply-To: <87y5dzgqum.fsf@openvz.org>
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
> >
next prev parent reply other threads:[~2013-03-08 12:45 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-06 14:17 [PATCH v2 0/5] ext4: try to fix up es regressions Zheng Liu
2013-03-06 14:17 ` [PATCH v2 1/5] ext4: improve ext4_es_can_be_merged() to avoid a potential overflow Zheng Liu
2013-03-11 0:43 ` Theodore Ts'o
2013-03-11 6:03 ` Zheng Liu
2013-03-06 14:17 ` [PATCH v2 2/5] ext4: add self-testing infrastructure to do a sanity check Zheng Liu
2013-03-07 15:41 ` Dmitry Monakhov
2013-03-08 13:01 ` Zheng Liu [this message]
2013-03-11 1:01 ` Theodore Ts'o
2013-03-06 14:17 ` [PATCH v2 3/5] ext4: fix wrong m_len value after unwritten extent conversion Zheng Liu
2013-03-07 15:42 ` Dmitry Monakhov
2013-03-11 1:07 ` Theodore Ts'o
2013-03-11 5:47 ` Zheng Liu
2013-03-13 1:57 ` Theodore Ts'o
2013-03-13 2:14 ` Theodore Ts'o
2013-03-13 8:53 ` Zheng Liu
2013-03-06 14:17 ` [PATCH v2 4/5] ext4: update extent status tree after an extent is zeroed out Zheng Liu
2013-03-07 15:55 ` Dmitry Monakhov
2013-03-08 13:14 ` Zheng Liu
2013-03-06 14:17 ` [PATCH v2 5/5] ext4: fix wrong the number of the allocted blocks in ext4_split_extent Zheng Liu
2013-03-06 22:58 ` Dev branch regressions Theodore Ts'o
2013-03-07 2:40 ` Zheng Liu
2013-03-07 6:47 ` Lukáš Czerner
2013-03-07 11:54 ` Zheng Liu
2013-03-07 16:08 ` [PATCH v2 0/5] ext4: try to fix up es regressions Dmitry Monakhov
2013-03-08 13:18 ` Zheng Liu
2013-03-11 2:11 ` Theodore Ts'o
2013-03-11 6:23 ` Zheng Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130308130102.GB18986@gmail.com \
--to=gnehzuil.liu@gmail.com \
--cc=dmonakhov@openvz.org \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=wenqing.lz@taobao.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.