On 11.10.2018 08:40, Lu Fengqi wrote:
> If the return value of find_ref_head() is NULL, the only possibility is
> that delayed_refs' head ref rbtree is empty. Hence, the second
> find_ref_head() is pointless.
> > Besides, the local variables loop and start are unnecessary, just remove
> them.
So the objective of that function is to get a reference to the first
delayed head which is not processed. This is done by essentially keeping
track of the last range that was processed in
delayed_refs->run_delayed_start
>
> Signed-off-by: Lu Fengqi <lufq.f...@cn.fujitsu.com>
> ---
> fs/btrfs/delayed-ref.c | 17 +++--------------
> 1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 885581852bea..2726d2fb4bbe 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -354,20 +354,11 @@ struct btrfs_delayed_ref_head *
> btrfs_select_ref_head(struct btrfs_delayed_ref_root *delayed_refs)
> {
> struct btrfs_delayed_ref_head *head;
> - u64 start;
> - bool loop = false;
>
> again:
> - start = delayed_refs->run_delayed_start;
> - head = find_ref_head(delayed_refs, start, 1);
> - if (!head && !loop) {
> + head = find_ref_head(delayed_refs, delayed_refs->run_delayed_start, 1);
> + if (!head) {
> delayed_refs->run_delayed_start = 0;
> - start = 0;
> - loop = true;
> - head = find_ref_head(delayed_refs, start, 1);
> - if (!head)
> - return NULL;
> - } else if (!head && loop) {
I believe this will have a negative impact since it actually will
prevent finding a head which was added BEFORE the last processed head.
So when a ref head is selected in btrfs_obtain_ref_head then the
delayed_refs->lock is dropped and the given head is locked and
delayed_refs->run_delayed_start points to the end of the selected range
that the head represents. At this point it's possible that another
thread modifies a different range which is before the one we have
selected so graphically it will be something like:
---[HEAD2]----->[HEAD1]------
0 N
Where HEAD1 is the head returned from first invocation of
btrfs_obtain_ref_head. Once btrfs_obtain_ref_head is called the 2nd
time it will not find HEAD2 so will just reset run_delayed_start to 0
and return. So it will be up to another run of the delayed refs to
actually find head2. Essentially you made btrfs_obtain_ref_head less
greedy. Have you characterized what kind of performance impact this have?
> return NULL;
> }
>
> @@ -376,11 +367,9 @@ btrfs_select_ref_head(struct btrfs_delayed_ref_root
> *delayed_refs)
>
> node = rb_next(&head->href_node);
> if (!node) {
> - if (loop)
> + if (delayed_refs->run_delayed_start == 0)
> return NULL;
> delayed_refs->run_delayed_start = 0;
> - start = 0;
> - loop = true;
> goto again;
> }
> head = rb_entry(node, struct btrfs_delayed_ref_head,
>
