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

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

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

Thanks,
Liam