Re: [PATCH v2] ext4: fix hole punch failure when depth is greaterthan 0

[Ashish Sangwan]

On Wed, Jul 4, 2012 at 11:03 PM, LukÃÅ Czerner <lczerner@xxxxxxxxxx> wrote:
> So I've finally has some time to look at the patch and reproduce the
> problem. Thanks for noticing the problem, the patch seems good,
> though I have one question. Is the p_block setting really necessary
> ? I do not think so, but I might be missing something. Here is
> updated patch I've tested, bellow.
AFAICS, p_block setting is necessary. As mentioned in the patch description,
whether to continue removing extents or not is decided by the return value
of function ext4_ext_more_to_rm() which checks 2 conditions:
a) if there are no more indexes to process.
b) if the number of entries are decreased in the header of "depth -1".
The p_block setting is important for condition b).

In function ext4_ext_more_to_rm, there is this second check:
if (le16_to_cpu(path->p_hdr->eh_entries) == path->p_block)
    return 0;
If the value of p_block would not be correct, the above mentioned
condition could become
true while there are still blocks left to be removed.
>
> Note: there are some indent problems in your patch, like for example
> this:
>
> +                       path[k].p_block =
> +                       le16_to_cpu(path[k].p_hdr->eh_entries)+1;
>
>
Before submitting the patch, I run checkpatch.pl with --strict option.
It did'nt show any error or warning. Should I re-submit
the patch with an extra tab before the second line? The call is yours.

> Anyway, what do you think about the modification ?
>
Also there is 1 modification missing from your patch.
    ext4_ext_drop_refs(path);
        kfree(path);
+       path = NULL;
        if (err == -EAGAIN)
                goto again;
If path is not set to NULL, it will crash in xfstest #13. Ted has
already reported it.

Thanks,
Ashish

> Thanks!
> -Lukas
>
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index b3300eb..94bc1bd 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2570,7 +2570,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>  {
>         struct super_block *sb = inode->i_sb;
>         int depth = ext_depth(inode);
> -       struct ext4_ext_path *path;
> +       struct ext4_ext_path *path = NULL;
>         ext4_fsblk_t partial_cluster = 0;
>         handle_t *handle;
>         int i, err;
> @@ -2606,8 +2606,12 @@ again:
>                 }
>                 depth = ext_depth(inode);
>                 ex = path[depth].p_ext;
> -               if (!ex)
> +               if (!ex) {
> +                       ext4_ext_drop_refs(path);
> +                       kfree(path);
> +                       path = NULL;
>                         goto cont;
> +               }
>
>                 ee_block = le32_to_cpu(ex->ee_block);
>
> @@ -2637,8 +2641,6 @@ again:
>                         if (err < 0)
>                                 goto out;
>                 }
> -               ext4_ext_drop_refs(path);
> -               kfree(path);
>         }
>  cont:
>
> @@ -2646,20 +2648,26 @@ cont:
>          * We start scanning from right side, freeing all the blocks
>          * after i_size and walking into the tree depth-wise.
>          */
> -       depth = ext_depth(inode);
> -       path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS);
> -       if (path == NULL) {
> -               ext4_journal_stop(handle);
> -               return -ENOMEM;
> -       }
> -       path[0].p_depth = depth;
> -       path[0].p_hdr = ext_inode_hdr(inode);
> +       if (path)
> +               i = depth;
> +       else {
> +               depth = ext_depth(inode);
> +               path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1),
> +                              GFP_NOFS);
> +               if (path == NULL) {
> +                       ext4_journal_stop(handle);
> +                       return -ENOMEM;
> +               }
> +               path[0].p_depth = depth;
> +               path[0].p_hdr = ext_inode_hdr(inode);
>
> -       if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
> -               err = -EIO;
> -               goto out;
> +               if (ext4_ext_check(inode, path[0].p_hdr, depth)) {
> +                       err = -EIO;
> +                       goto out;
> +               }
> +               i = 0;
>         }
> -       i = err = 0;
> +       err = 0;
>
>         while (i >= 0 && err == 0) {
>                 if (i == depth) {
>
> On Mon, 2 Jul 2012, Ashish Sangwan wrote:
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/