Re: [BUG] rbtree bug with mmotm 2009-04-14-17-24

[Jens Axboe <jens.axboe@oracle.com>]

On Wed, Apr 22 2009, Steve Wise wrote:
> Steve Wise wrote:
>> Jens Axboe wrote:
>>
>>  
>>
>>>> Still crashes with this variant:
>>>>     
>>>
>>> OK, so please try and revert a36e71f996e25d6213f57951f7ae1874086ec57e
>>> and see if that works?
>>>
>>> Or, better yet, please try and revert
>>> 55a63998b8967615a15e2211ba0ff3a84a565824 first. If that doesn't work,
>>> try the above revert.
>>>
>>>   
>>
>> Reverting 55a63998b8967615a15e2211ba0ff3a84a565824 did the trick.
>>
>>
>> Steve.
>>
>>
>
> <snipit from 55a63998b8967615a15e2211ba0ff3a84a565824>
>
> @@ -200,17 +197,14 @@ static void __rb_erase_color(struct rb_node *node,  
> struct rb_node *parent,
>                        {
>                                if (!other->rb_left ||  
> rb_is_black(other->rb_left))
>                                {
> -                                       register struct rb_node *o_right;
> -                                       if ((o_right = other->rb_right))
> -                                               rb_set_black(o_right);
> +                                       rb_set_black(other->rb_right);
>                                        rb_set_red(other);
>                                        __rb_rotate_left(other, root);
>                                        other = parent->rb_left;
>                                }
>                                rb_set_color(other, rb_color(parent));
>                                rb_set_black(parent);
> -                               if (other->rb_left)
> -                                       rb_set_black(other->rb_left);
> +                               rb_set_black(other->rb_left);
>                                __rb_rotate_right(parent, root);
>                                node = root->rb_node;
>                                break;
>
>
> I don't know this code, but isn't the 'if (other->rb_left)' really  
> needed?  Or is it always true that if '!other->rb_left' is true entering  
> this snipit, then after executing the first 'if' block, then  
> 'other->rb_left' must be a valid ptr? (how's that for confusing english? 
> :)

Heh, not sure. What is sure is that the commit in question is
problematic. Either because it itself has a bug, or because it exposes a
bug elsewhere in the rbtree code. So I'd suggest we just revert that
commit ASAP.

-- 
Jens Axboe