[PATCH] Btrfs: cleanup for __merge_refs

[PATCH] Btrfs: cleanup for __merge_refs

[Liu Bo]

Parents must be same after going through ref_for_same_block.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/backref.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index f318793..9aaa38e6 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -469,11 +469,6 @@ static int __merge_refs(struct list_head *head, int mode)
 			if (mode == 1) {
 				if (!ref_for_same_block(ref1, ref2))
 					continue;
-				if (!ref1->parent && ref2->parent) {
-					xchg = ref1;
-					ref1 = ref2;
-					ref2 = xchg;
-				}
 				ref1->count += ref2->count;
 			} else {
 				if (ref1->parent != ref2->parent)
@@ -483,7 +478,6 @@ static int __merge_refs(struct list_head *head, int mode)
 			list_del(&ref2->list);
 			kfree(ref2);
 		}
-
 	}
 	return 0;
 }
-- 
1.7.7.6

Re: [PATCH] Btrfs: cleanup for __merge_refs

[Jan Schmidt]

On Thu, October 18, 2012 at 09:10 (+0200), Liu Bo wrote:
> Parents must be same after going through ref_for_same_block.

Well. We could kill that for the moment. However, ref_for_same_block wasn't
meant to check for the parent. We should do ...

--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -400,8 +400,6 @@ static inline int ref_for_same_block(struct __prelim_ref *ref1,
                return 0;
        if (ref1->key_for_search.offset != ref2->key_for_search.offset)
                return 0;
-       if (ref1->parent != ref2->parent)
-               return 0;

        return 1;
 }

Those two lines were originally added while debugging. The idea was to merge
indirect (parent == 0) and shared backrefs (parent != 0) to avoid the expensive
tree search for indirect backrefs when possible.

We would need extensive testing to remove those two lines from the current
version, though...

-Jan

Re: [PATCH] Btrfs: cleanup for __merge_refs

[Liu Bo]

On 10/18/2012 03:31 PM, Jan Schmidt wrote:
> On Thu, October 18, 2012 at 09:10 (+0200), Liu Bo wrote:
>> Parents must be same after going through ref_for_same_block.
> 
> Well. We could kill that for the moment. However, ref_for_same_block wasn't
> meant to check for the parent. We should do ...
> 
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -400,8 +400,6 @@ static inline int ref_for_same_block(struct __prelim_ref *ref1,
>                 return 0;
>         if (ref1->key_for_search.offset != ref2->key_for_search.offset)
>                 return 0;
> -       if (ref1->parent != ref2->parent)
> -               return 0;
> 
>         return 1;
>  }
> 
> Those two lines were originally added while debugging. The idea was to merge
> indirect (parent == 0) and shared backrefs (parent != 0) to avoid the expensive
> tree search for indirect backrefs when possible.
> 
> We would need extensive testing to remove those two lines from the current
> version, though...
> 

Oh, all right, would you make this as a patch by yourself or I can send a V2 for you?

thanks,
liubo

> -Jan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Btrfs: cleanup for __merge_refs

[Jan Schmidt]

On Thu, October 18, 2012 at 09:36 (+0200), Liu Bo wrote:
> On 10/18/2012 03:31 PM, Jan Schmidt wrote:
>> On Thu, October 18, 2012 at 09:10 (+0200), Liu Bo wrote:
>>> Parents must be same after going through ref_for_same_block.
>>
>> Well. We could kill that for the moment. However, ref_for_same_block wasn't
>> meant to check for the parent. We should do ...
>>
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -400,8 +400,6 @@ static inline int ref_for_same_block(struct __prelim_ref *ref1,
>>                 return 0;
>>         if (ref1->key_for_search.offset != ref2->key_for_search.offset)
>>                 return 0;
>> -       if (ref1->parent != ref2->parent)
>> -               return 0;
>>
>>         return 1;
>>  }
>>
>> Those two lines were originally added while debugging. The idea was to merge
>> indirect (parent == 0) and shared backrefs (parent != 0) to avoid the expensive
>> tree search for indirect backrefs when possible.
>>
>> We would need extensive testing to remove those two lines from the current
>> version, though...
>>
> 
> Oh, all right, would you make this as a patch by yourself or I can send a V2 for you?

If you want to make sure nothing would break with that patch, feel free to send
it :-) It's quite a few things to think of, and a lot to test, though. Currently
I don't want to put that much effort into backref walking performance. I might
do so later if you decide not to do it now.

I'm also okay with applying your original patch for now, although those lines
are very likely come back in the future.

Thanks,
-Jan

Re: [PATCH] Btrfs: cleanup for __merge_refs

[Liu Bo]

On 10/18/2012 03:42 PM, Jan Schmidt wrote:
> On Thu, October 18, 2012 at 09:36 (+0200), Liu Bo wrote:
>> On 10/18/2012 03:31 PM, Jan Schmidt wrote:
>>> On Thu, October 18, 2012 at 09:10 (+0200), Liu Bo wrote:
>>>> Parents must be same after going through ref_for_same_block.
>>>
>>> Well. We could kill that for the moment. However, ref_for_same_block wasn't
>>> meant to check for the parent. We should do ...
>>>
>>> --- a/fs/btrfs/backref.c
>>> +++ b/fs/btrfs/backref.c
>>> @@ -400,8 +400,6 @@ static inline int ref_for_same_block(struct __prelim_ref *ref1,
>>>                 return 0;
>>>         if (ref1->key_for_search.offset != ref2->key_for_search.offset)
>>>                 return 0;
>>> -       if (ref1->parent != ref2->parent)
>>> -               return 0;
>>>
>>>         return 1;
>>>  }
>>>
>>> Those two lines were originally added while debugging. The idea was to merge
>>> indirect (parent == 0) and shared backrefs (parent != 0) to avoid the expensive
>>> tree search for indirect backrefs when possible.
>>>
>>> We would need extensive testing to remove those two lines from the current
>>> version, though...
>>>
>>
>> Oh, all right, would you make this as a patch by yourself or I can send a V2 for you?
> 
> If you want to make sure nothing would break with that patch, feel free to send
> it :-) It's quite a few things to think of, and a lot to test, though. Currently
> I don't want to put that much effort into backref walking performance. I might
> do so later if you decide not to do it now.
> 

Well, I can make the change locally and do some testing to see if it brings any bugs.

> I'm also okay with applying your original patch for now, although those lines
> are very likely come back in the future.
> 

So just leave it alone :)

thanks,
liubo

> Thanks,
> -Jan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>