Re: [PATCH v3] ext4: Fix bug on in ext4_es_cache_extent as ext4_split_extent_at failed
From: yebin
Date: Thu May 06 2021 - 07:47:20 EST
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 }