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

[Nikolay Borisov <nborisov@suse.com>]

On 11.10.2018 15:15, Lu Fengqi wrote:
> On Thu, Oct 11, 2018 at 09:40:52AM +0300, Nikolay Borisov wrote:
>>
>>
>> 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
> 
> Not exactly. In fact, find_ref_head hides such a logic. When
> return_bigger is set, if there is no larger entry to return, the first
> entry will be returned. Please see the comment I add in the PATCH 6.
> 
> Hence, the 2nd invocation of btrfs_obtain_ref_head still will return
> HEAD2. There is no functional change here.
> 
> However, your question makes me consider whether such hidden logic
> should be extracted from find_ref_head to btrfs_select_ref_head.

Right I agree with your. As it stands I will expect that if
return_bigger is true to specifically return a bigger entry or if
nothing is found to return null. IMO this behavior is higher level and
belongs to btrfs_delayed_ref_head.

> 
>> greedy. Have you characterized what kind of performance impact this have?
> 
> I noticed that there is a macro called SCRAMBLE_DELAYED_REFS in the
> extent-tree.c. I am a bit curious whether it has been forgotten by
> everyone, I have not found any test results about its performance impact.

I guess it was used during testing but nothing currently sets it. I.e it
might make sense to enable it if BTRFS_DEBUG is set.