From: yebin <yebin10@huawei.com>
To: Jan Kara <jack@suse.cz>
Cc: <tytso@mit.edu>, <adilger.kernel@dilger.ca>,
<linux-ext4@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed
Date: Thu, 6 May 2021 19:47:11 +0800 [thread overview]
Message-ID: <6093D73F.70909@huawei.com> (raw)
In-Reply-To: <20210506101915.GA22189@quack2.suse.cz>
On 2021/5/6 18:19, Jan Kara wrote:
> On Thu 06-05-21 16:26:24, yebin wrote:
>> Thanks for your suggesttion. If you have no objection to following
>> modification, i will send it as V4.
>>
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> index 77c84d6f1af6..f9cbd11e1eae 100644
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
>> @@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
>> ext4_ext_mark_unwritten(ex2);
>>
>> err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
>> - if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
>> + if (err != -ENOSPC && err != -EDQUOT)
>> + goto out;
>> +
>> + if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
> You need:
>
> if (err && (EXT4_EXT_MAY_ZEROOUT & split_flag))
>
> there, don't you? You don't want to zero-out if there's no error.
If (err != -ENOSPC && err != -EDQUOT) already goto out, so there is needn't
judge "err" again.
>
>> @@ -3232,22 +3235,23 @@ static int ext4_split_extent_at(handle_t *handle,
>> ext4_ext_pblock(&orig_ex));
>> }
>>
>> - if (err)
>> - goto fix_extent_len;
>> - /* update the extent length and mark as initialized */
>> - 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_zeroout_es(inode, &zero_ex);
>> -
>> - goto out;
>> - } else if (err)
>> - goto fix_extent_len;
>> -
>> + if (!err) {
>> + /* update the extent length and mark as initialized
>> */
>> + 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)
>> + /* update extent status tree */
>> + err = ext4_zeroout_es(inode, &zero_ex);
>> + /* At here, ext4_ext_try_to_merge maybe already
>> merge
>> + * extent, if fix origin extent length may lead to
>> + * overwritten.
>> + */
> I'd rephrase the comment as:
>
> /*
> * If we failed at this point, we don't know in which state the extent tree
> * exactly is so don't try to fix length of the original extent as it may do
> * even more damage.
> */
I will replace it with your comment.
>
>> + goto out;
>> + }
>> + }
>> + if (err)
>> + goto fix_extent_len;
> And you can move this if (err) before if (!err) above to make code easier
> to read and save one indentation level.
if (EXT4_EXT_MAY_ZEROOUT & split_flag) do zero-out, if failed, we
don't need fix extent length.
But if (!EXT4_EXT_MAY_ZEROOUT & split_flag) we need to fix extent
length. Maybe i can move
label "out" behind label "fix_extent_len", then this judement can be
removed.
Did i misunderstand what you meant earlier?
>> out:
>> ext4_ext_show_leaf(inode, path);
>> return err;
>>
>>
> Honza
The diff as following:
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 77c84d6f1af6..cbf37b2cf871 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3206,7 +3206,10 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_mark_unwritten(ex2);
err = ext4_ext_insert_extent(handle, inode, ppath, &newex, flags);
- if (err == -ENOSPC && (EXT4_EXT_MAY_ZEROOUT & split_flag)) {
+ if (err != -ENOSPC && err != -EDQUOT)
+ goto out;
+
+ if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
if (split_flag &
(EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
if (split_flag & EXT4_EXT_DATA_VALID1) {
err = ext4_ext_zeroout(inode, ex2);
@@ -3232,25 +3235,22 @@ static int ext4_split_extent_at(handle_t *handle,
ext4_ext_pblock(&orig_ex));
}
- if (err)
- goto fix_extent_len;
- /* update the extent length and mark as initialized */
- 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_zeroout_es(inode, &zero_ex);
-
- goto out;
- } else if (err)
- goto fix_extent_len;
-
-out:
- ext4_ext_show_leaf(inode, path);
- return err;
+ if (!err) {
+ /* update the extent length and mark as
initialized */
+ 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)
+ /* update extent status tree */
+ err = ext4_zeroout_es(inode, &zero_ex);
+ /* If we failed at this point, we don't know in
which
+ * state the extent tree exactly is so don't try
to fix
+ * length of the original extent as it may do
even more
+ * damage.
+ */
+ goto out;
+ }
+ }
fix_extent_len:
ex->ee_len = orig_ex.ee_len;
@@ -3260,6 +3260,9 @@ static int ext4_split_extent_at(handle_t *handle,
*/
ext4_ext_dirty(handle, inode, path + path->p_depth);
return err;
+out:
+ ext4_ext_show_leaf(inode, path);
+ return err;
}
The whole code as folowing:
ext4_split_extent_at:
.......
3208 err = ext4_ext_insert_extent(handle, inode, ppath, &newex,
flags);
3209 if (err != -ENOSPC && err != -EDQUOT)
3210 goto out;
3211
3212 if (EXT4_EXT_MAY_ZEROOUT & split_flag) {
3213 if (split_flag &
(EXT4_EXT_DATA_VALID1|EXT4_EXT_DATA_VALID2)) {
3214 if (split_flag & EXT4_EXT_DATA_VALID1) {
3215 err = ext4_ext_zeroout(inode, ex2);
3216 zero_ex.ee_block = ex2->ee_block;
3217 zero_ex.ee_len = cpu_to_le16(
3218 ext4_ext_get_actual_len(ex2));
3219 ext4_ext_store_pblock(&zero_ex,
3220 ext4_ext_pblock(ex2));
3221 } else {
3222 err = ext4_ext_zeroout(inode, ex);
3223 zero_ex.ee_block = ex->ee_block;
3224 zero_ex.ee_len = cpu_to_le16(
3225 ext4_ext_get_actual_len(ex));
3226 ext4_ext_store_pblock(&zero_ex,
3227 ext4_ext_pblock(ex));
3228 }
3229 } else {
3230 err = ext4_ext_zeroout(inode, &orig_ex);
3231 zero_ex.ee_block = orig_ex.ee_block;
3232 zero_ex.ee_len = cpu_to_le16(
3233 ext4_ext_get_actual_len(&orig_ex));
3234 ext4_ext_store_pblock(&zero_ex,
3235 ext4_ext_pblock(&orig_ex));
3236 }
3237
3238 if (!err) {
3239 /* update the extent length and mark as
initialized */
3240 ex->ee_len = cpu_to_le16(ee_len);
3241 ext4_ext_try_to_merge(handle, inode, path, ex);
3242 err = ext4_ext_dirty(handle, inode, path +
path->p_depth);
3243 if (!err)
3244 /* update extent status tree */
3245 err = ext4_zeroout_es(inode, &zero_ex);
3246 /* If we failed at this point, we don't
know in which
3247 * state the extent tree exactly is so
don't try to fix
3248 * length of the original extent as it may
do even more
3249 * damage.
3250 */
3251 goto out;
3252 }
3253 }
3254
3255 fix_extent_len:
3256 ex->ee_len = orig_ex.ee_len;
3257 /*
3258 * Ignore ext4_ext_dirty return value since we are already
in error path
3259 * and err is a non-zero error code.
3260 */
3261 ext4_ext_dirty(handle, inode, path + path->p_depth);
3262 return err;
3263 out:
3264 ext4_ext_show_leaf(inode, path);
3265 return err;
3266 }
next prev parent reply other threads:[~2021-05-06 11:47 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-28 8:51 [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed Ye Bin
2021-04-30 12:58 ` Jan Kara
2021-05-05 3:29 ` yebin
2021-05-05 10:41 ` Jan Kara
2021-05-06 8:26 ` yebin
2021-05-06 10:19 ` Jan Kara
2021-05-06 11:47 ` yebin [this message]
2021-05-06 12:15 ` Jan Kara
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=6093D73F.70909@huawei.com \
--to=yebin10@huawei.com \
--cc=adilger.kernel@dilger.ca \
--cc=jack@suse.cz \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tytso@mit.edu \
/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.