Re: [PATCH 5/6] btrfs: simplify btrfs_select_ref_head and cleanup some local variables

[Nikolay Borisov <nborisov@suse.com>]

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