On Mon, Nov 5, 2018 at 4:10 AM robbieko <robbi...@synology.com> wrote:
>
> Filipe Manana 於 2018-10-30 19:36 寫到:
> > On Tue, Oct 30, 2018 at 7:00 AM robbieko <robbi...@synology.com> wrote:
> >>
> >> From: Robbie Ko <robbi...@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.
> >
>
> I am sorry that I replied so late.
>
> I have no way to give a simple example.
> But I can provide a btrfs image file
> You can restore the Image via btrfs-image
> Then directly command "btrfs send -e -p parent send -f dump_file"
> Infinite loop will occur.
> I use ubuntu 16.04, kernel 4.15.0.36-generic can be stable reproduce
You have been occasionally submitting fixes for send/receive for a few
years now, and you know already
that I always ask for a changelog that describes well the problem and
an example/reproducer.
Why did you do this?
What I can read from your answer is that you were too lazy to extract
a reproducer from that image.
Just made some change that fixes the infinite loop and because it
apparently works you're done with it,
Without an example at least, I don't think you or anyone can fully
understand the problem, and if what
you have (despite somewhat making theoretical sense) is really a good
solution or just a workaround for
the cause of the problem - after all if you can't give an example, you
can't explain how in practice such loop
of dependencies between directories happens. This, as with most
send/receive problems, is a pure sequential
and deterministic problem so there's really no excuse for not getting
a reproducer.
Without an example, an explanation how it happens in the real world,
does one know that your change is
fixing the problem is the right place and not introducing other
problems? Like the receiver not getting some
changes (missing directories, files, or renames, etc).
Tests are not just to prove a change is correct, they exist to catch
and prevent regressions in the future too.
You can do better than that.
>
> Image file, please refer to the attachment.
>
> Thanks.
>
>
> >>
> >> Signed-off-by: Robbie Ko <robbi...@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.”
