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

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

On Tue, Oct 15, 2024 at 10:32:41PM -0400, Liam R. Howlett wrote:
>* Wei Yang <richard.weiyang@gmail.com> [241015 22:18]:
>> 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?
>
>No, it's not fine.  You are removing an optimisation.  The issue is the
>other side of things where a node is used to store a single range from 0
>to ULONX_MAX pointing to NULL in a 256B node.
>
>And, potentially, the debug dump of the tree should be more clear.
>
>> 
>> >
>> >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.
>
>Merge changes like this in the future, but the second patch in this
>series is wrong.
>
>> 
>> >
>> >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?
>
>Write a test to find out if the resulting 0 - ULONG_MAX store of NULL
>results in a node.  If it is in a node, then the test should fail.
>
>> 
>> >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?
>
>Yes, it's not a bug/problem - it's just inefficient use of space when
>someone tries to store 0 - ULONG_MAX, so there really isn't a reason to
>backport.
>

Ok, I see your preference.

So current behavior of this is not expected, right?

  mas_set_range(mas, 0, ULONG_MAX)
  mas_store(mas, NULL)

This operation to an empty tree will create a node now.

Looks like a problem?

>Thanks,
>Liam

-- 
Wei Yang
Help you, Help me