[5/6] btrfs: simplify btrfs_select_ref_head and cleanup some local variables
diff mbox series

Message ID 20181011054038.5428-6-lufq.fnst@cn.fujitsu.com
State New
Headers show
Series
  • Some trivail cleanup about dealyed-refs
Related show

Commit Message

Lu Fengqi Oct. 11, 2018, 5:40 a.m. UTC
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.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 fs/btrfs/delayed-ref.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

Comments

Nikolay Borisov Oct. 11, 2018, 6:40 a.m. UTC | #1
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,
>
Lu Fengqi Oct. 11, 2018, 12:15 p.m. UTC | #2
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.

>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.
Nikolay Borisov Oct. 11, 2018, 12:28 p.m. UTC | #3
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.
David Sterba Oct. 11, 2018, 12:45 p.m. UTC | #4
On Thu, Oct 11, 2018 at 03:28:15PM +0300, Nikolay Borisov wrote:
> > 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.

Agreed, the way the scrambling is supposed to be used does not align
very well with the typical testing workflow so adding to ti the
BTRFS_DEBUG set is ok, unless there are severe performance problems.

The part in btrfs_run_delayed_refs would be better hidden in a function
similar to btrfs_debug_check_extent_io_range or btrfs_leak_debug_check.
Lu Fengqi Oct. 15, 2018, 2:09 a.m. UTC | #5
On Thu, Oct 11, 2018 at 03:28:15PM +0300, Nikolay Borisov wrote:
>
>
>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

This is also exactly what I want. The patch is on the way.

>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.
>

Make sense.
Lu Fengqi Oct. 15, 2018, 2:32 a.m. UTC | #6
On Thu, Oct 11, 2018 at 02:45:04PM +0200, David Sterba wrote:
>On Thu, Oct 11, 2018 at 03:28:15PM +0300, Nikolay Borisov wrote:
>> > 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.
>
>Agreed, the way the scrambling is supposed to be used does not align
>very well with the typical testing workflow so adding to ti the
>BTRFS_DEBUG set is ok, unless there are severe performance problems.

I will add it to the BTRFS_DEBUG set, and test if it has severe
performance problems.

>
>The part in btrfs_run_delayed_refs would be better hidden in a function
>similar to btrfs_debug_check_extent_io_range or btrfs_leak_debug_check.

Got it.

Patch
diff mbox series

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) {
 		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,