Re: [PATCH] Btrfs: incremental send, fix infinite loop when apply children dir moves

From: Filipe Manana <fdmanana@gmail.com>
To: Robbie Ko <robbieko@synology.com>
Cc: linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH] Btrfs: incremental send, fix infinite loop when apply children dir moves
Date: Tue, 30 Oct 2018 11:36:59 +0000	[thread overview]
Message-ID: <CAL3q7H4p_fa=5UhfBkSs3GU5EydKQ0J76D5RMShMY-2HQY3F+Q@mail.gmail.com> (raw)
In-Reply-To: <1540882702-21104-1-git-send-email-robbieko@synology.com>

On Tue, Oct 30, 2018 at 7:00 AM robbieko <robbieko@synology.com> wrote:
>
> From: Robbie Ko <robbieko@synology.com>
>
> In apply_children_dir_moves, we first create an empty list (stack),
> then we get an entry from pending_dir_moves and add it to the stack,
> but we didn't delete the entry from rb_tree.
>
> So, in add_pending_dir_move, we create a new entry and then use the
> parent_ino in the current rb_tree to find the corresponding entry,
> and if so, add the new entry to the corresponding list.
>
> However, the entry may have been added to the stack, causing new
> entries to be added to the stack as well.
>
> Finally, each time we take the first entry from the stack and start
> processing, it ends up with an infinite loop.
>
> Fix this problem by remove node from pending_dir_moves,
> avoid add new pending_dir_move to error list.

I can't parse that explanation.
Can you give a concrete example (reproducer) or did this came out of thin air?

Thanks.

>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
>  fs/btrfs/send.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 094cc144..5be83b5 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -3340,7 +3340,8 @@ static void free_pending_move(struct send_ctx *sctx, struct pending_dir_move *m)
>         kfree(m);
>  }
>
> -static void tail_append_pending_moves(struct pending_dir_move *moves,
> +static void tail_append_pending_moves(struct send_ctx *sctx,
> +                                     struct pending_dir_move *moves,
>                                       struct list_head *stack)
>  {
>         if (list_empty(&moves->list)) {
> @@ -3351,6 +3352,10 @@ static void tail_append_pending_moves(struct pending_dir_move *moves,
>                 list_add_tail(&moves->list, stack);
>                 list_splice_tail(&list, stack);
>         }
> +       if (!RB_EMPTY_NODE(&moves->node)) {
> +               rb_erase(&moves->node, &sctx->pending_dir_moves);
> +               RB_CLEAR_NODE(&moves->node);
> +       }
>  }
>
>  static int apply_children_dir_moves(struct send_ctx *sctx)
> @@ -3365,7 +3370,7 @@ static int apply_children_dir_moves(struct send_ctx *sctx)
>                 return 0;
>
>         INIT_LIST_HEAD(&stack);
> -       tail_append_pending_moves(pm, &stack);
> +       tail_append_pending_moves(sctx, pm, &stack);
>
>         while (!list_empty(&stack)) {
>                 pm = list_first_entry(&stack, struct pending_dir_move, list);
> @@ -3376,7 +3381,7 @@ static int apply_children_dir_moves(struct send_ctx *sctx)
>                         goto out;
>                 pm = get_pending_dir_moves(sctx, parent_ino);
>                 if (pm)
> -                       tail_append_pending_moves(pm, &stack);
> +                       tail_append_pending_moves(sctx, pm, &stack);
>         }
>         return 0;
>
> --
> 1.9.1
>

-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”