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 4/5] ext4: update extent status tree after an extent is zeroed out
Date: Fri, 8 Mar 2013 21:14:32 +0800 [thread overview]
Message-ID: <20130308131431.GC18986@gmail.com> (raw)
In-Reply-To: <87sj47gq71.fsf@openvz.org>
On Thu, Mar 07, 2013 at 07:55:14PM +0400, Dmitry Monakhov wrote:
> On Wed, 6 Mar 2013 22:17:14 +0800, Zheng Liu <gnehzuil.liu@gmail.com> wrote:
> > From: Zheng Liu <wenqing.lz@taobao.com>
> >
> > When we try to split an extent, this extent could be zeroed out and mark
> > as initialized. But we don't know this in ext4_map_blocks because it
> > only returns a length of allocated extent. Meanwhile we will mark this
> > extent as uninitialized because we only check m_flags.
> >
> > This commit update extent status tree when we try to split an unwritten
> > extent. We don't need to worry about the status of this extent because
> > we always mark it as initialized.
> >
> > 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 | 35 +++++++++++++++++++++++++++++++----
> > fs/ext4/extents_status.c | 17 +++++++++++++++++
> > fs/ext4/extents_status.h | 3 +++
> > fs/ext4/inode.c | 10 ++++++++++
> > 4 files changed, 61 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index 110e85a..7e37018 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -2925,7 +2925,7 @@ static int ext4_split_extent_at(handle_t *handle,
> > {
> > ext4_fsblk_t newblock;
> > ext4_lblk_t ee_block;
> > - struct ext4_extent *ex, newex, orig_ex;
> > + struct ext4_extent *ex, newex, orig_ex, zero_ex;
> > struct ext4_extent *ex2 = NULL;
> > unsigned int ee_len, depth;
> > int err = 0;
> > @@ -2996,12 +2996,26 @@ static int ext4_split_extent_at(handle_t *handle,
> > err = ext4_ext_insert_extent(handle, inode, path, &newex, flags);
> > if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
> > if (split_flag & (EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
> > - if (split_flag & EXT4_EXT_DATA_VALID1)
> > + if (split_flag & EXT4_EXT_DATA_VALID1) {
> > err = ext4_ext_zeroout(inode, ex2);
> > - else
> > + zero_ex.ee_block = ex2->ee_block;
> > + zero_ex.ee_len = ext4_ext_get_actual_len(ex2);
> > + ext4_ext_store_pblock(&zero_ex,
> > + ext4_ext_pblock(ex2));
> > + } else {
> > err = ext4_ext_zeroout(inode, ex);
> > - } else
> > + zero_ex.ee_block = ex->ee_block;
> > + zero_ex.ee_len = ext4_ext_get_actual_len(ex);
> > + ext4_ext_store_pblock(&zero_ex,
> > + ext4_ext_pblock(ex));
> > + }
> > + } else {
> > err = ext4_ext_zeroout(inode, &orig_ex);
> > + zero_ex.ee_block = orig_ex.ee_block;
> > + zero_ex.ee_len = ext4_ext_get_actual_len(&orig_ex);
> > + ext4_ext_store_pblock(&zero_ex,
> > + ext4_ext_pblock(&orig_ex));
> > + }
> > if (err)
> > goto fix_extent_len;
> > @@ -3009,6 +3023,12 @@ static int ext4_split_extent_at(handle_t *handle,
> > ex->ee_len = cpu_to_le16(ee_len);
> > ext4_ext_try_to_merge(handle, inode, path, ex);
> > err = ext4_ext_dirty(handle, inode, path + path->p_depth);
> > + if (err)
> > + goto fix_extent_len;
> > +
> > + /* update extent status tree */
> > + err = ext4_es_zeroout(inode, &zero_ex);
> > +
> Previous blocks are correct but too complex.
> At this point we know that extent "ex" becomes initialized so just
> manually update it like follows:
> err = ext4_es_insert_extent(inode, ee_block, ee_len,
> ext4_ext_pblock(ex),
> EXTENT_STATUS_WRITTEN);
Yeah, but maybe a inline function is better. As you said below, I need
to avoid to define a local variable that is used only once. So it look
like this:
inline int ext4_es_zeroout(struct inode *inode, struct ext4_extent *ex)
{
return ext4_es_insert_extent(inode, le32_to_cpu(ex->ee_block),
ext4_ext_get_actual_len(ex),
ext4_ext_pblock(ex),
EXTENT_STATUS_WRITTEN);
}
What do you think?
> BTW I'm wonder what happen if one of ext4_es_xxx functions failed with
> error. ASAIU this possible only incase of ENOMEM so it is very unlikely
> but allowed. If this happens then es_tree will be out of sinc with
> extent_tree which later result in corruption.
I don't think we need to worry about it because we always remove some
extents from status tree, and then try to insert some one. -ENOMEM will
be returned when we are trying to insert a extent. So when -ENOMEM is
returned, that means that no related extent is in status tree. So later
we won't lookup this extent in map_blocks().
> > goto out;
> > } else if (err)
> > goto fix_extent_len;
> > @@ -3150,6 +3170,7 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> > ee_block = le32_to_cpu(ex->ee_block);
> > ee_len = ext4_ext_get_actual_len(ex);
> > allocated = ee_len - (map->m_lblk - ee_block);
> > + zero_ex.ee_len = 0;
> >
> > trace_ext4_ext_convert_to_initialized_enter(inode, map, ex);
> >
> > @@ -3247,6 +3268,9 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> > err = ext4_ext_zeroout(inode, ex);
> > if (err)
> > goto out;
> > + zero_ex.ee_block = ex->ee_block;
> > + zero_ex.ee_len = ext4_ext_get_actual_len(ex);
> > + ext4_ext_store_pblock(&zero_ex, ext4_ext_pblock(ex));
> >
> > err = ext4_ext_get_access(handle, inode, path + depth);
> > if (err)
> > @@ -3305,6 +3329,9 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
> > err = allocated;
> >
> > out:
> > + /* If we have gotten a failure, don't zero out status tree */
> > + if (!err)
> > + err = ext4_es_zeroout(inode, &zero_ex);
> > return err ? err : allocated;
> > }
> >
> > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> > index a434f81..e7bebbc 100644
> > --- a/fs/ext4/extents_status.c
> > +++ b/fs/ext4/extents_status.c
> > @@ -854,6 +854,23 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> > return err;
> > }
> >
> > +int ext4_es_zeroout(struct inode *inode, struct ext4_extent *ex)
> > +{
> > + ext4_lblk_t ee_block;
> > + ext4_fsblk_t ee_pblock;
> > + unsigned int ee_len;
> > +
> > + ee_block = le32_to_cpu(ex->ee_block);
> > + ee_len = ext4_ext_get_actual_len(ex);
> > + ee_pblock = ext4_ext_pblock(ex);
> Andreas Dilger do not like local variables which used only once. Let's avoid that.
As I said above.
Regards,
- Zheng
next prev parent reply other threads:[~2013-03-08 12:59 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
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 [this message]
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=20130308131431.GC18986@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).