On Thu 06-05-21 19:47:11, yebin wrote:

On 2021/5/6 18:19, Jan Kara wrote:

quoted

On Thu 06-05-21 16:26:24, yebin wrote:

quoted

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.

Right, my fault. I was confused.

quoted

quoted

@@ -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.

quoted

quoted

+                        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?

Thanks for the update! The diff now looks good to me so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

for your next posting.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR