Re: [PATCH 0/2] fix mas_new_root()

[Wei Yang <richard.weiyang@gmail.com>]

On Tue, Oct 15, 2024 at 09:25:19PM -0400, Liam R. Howlett wrote:
>* Liam R. Howlett <Liam.Howlett@oracle.com> [241015 20:42]:
>> * Wei Yang <richard.weiyang@gmail.com> [241015 19:39]:
>> > When overwriting the whole range with NULL, current behavior is not correct.
>> > 
>> 
>> This is really strange.  You have changed the code to be wrong then
>> removed it..  The second patch removes what you changed in the first.
>> 
>> It doesn't look right today but what you have done is also not right.
>
>Looking at this again, the code that you have changed is correct.
>
>I actually think the bug is the other way around.  If we are
>represnenting 0 - ULONG_MAX => NULL, then it's an empty tree and we
>don't need a node to store that, and shouldn't.
>
>It's also not really a bug, but a missed optimisation.  The ranges are
>stored correctly, we just use too much memory in one case.
>
>The dump isn't clear, but since we merge NULL entries, if there is a 0-0
>-> NULL and 1-ULONG_MAX => NULL, then they will be one and the same.
>You could change the dump code as part of your fix.
>
>It's like the init of a tree (tree->ma_root = NULL).

Agree with your above statement, this depends how we want to handle this. The
change here is to make the behavior consistent.

Want to confirm with you: the fix in this patch is fine with your, right?

>
>Please don't submit multiple patches to fix the same thing like this, it
>makes it look like you are trying to pad your patch count.  I'm guessing
>you did this to keep them logically separate, but when you completely
>drop the entire block of code that was changed in the second patch it
>becomes a bit much (and hard to follow, I was trying to figure out what
>branch you were working off because it didn't look like the patch would
>apply to my branch).

Sure, will merge it.

>
>Please submit a testcase with any suspected bugs. If it is not possible
>to do the fix first, then do them at the same time.  I often write the
>fix for a bug, then recreate the bug in a testcase and ensure that it
>fails without my fix.
>

Since user won't detect the difference, so a case to see whether the root is a
node looks good to you?

>I am not sure the fixes tag is correct in the patch either, since so
>much has changed around this.  You could test the older code to see once
>you write a testcase.  But the bug is using a node to store 0-ULONG_MAX
>=> NULL.
>

So I should drop the fix tag?

>> 
>> 
>> > Wei Yang (2):
>> >   maple_tree: not necessary to check index/last again
>> >   maple_tree: one single entry couldn't represent the whole range
>> > 
>> >  lib/maple_tree.c | 9 ---------
>> >  1 file changed, 9 deletions(-)
>> > 
>> > -- 
>> > 2.34.1
>> > 
>> > 

-- 
Wei Yang
Help you, Help me