[WIP | DO NOT MERGE] zstd restructuring #9673

c0d3z3r0 · 2019-12-04T08:18:05Z

This is the WIP PR / branch where restructuring of #8941 and all related work happens (main parts of the code from @BrainSlayer and @allanjude).

Main goals:

keep zstd clean and untouched to make updates easy
clean up the code (drop unneeded/unused code)
add more documentation
...
get this thing ready for final review (we will open a separate, clean PR for final review, so that nobody has to read our 9 000 comments :P)

PrivatePuffin · 2019-12-04T08:25:57Z

@c0d3z3r0 Thanks for seperating those two branches! :)

PrivatePuffin · 2019-12-04T08:56:32Z

Ubuntu 16 failure of zloop is unrelated.
http://build.zfsonlinux.org/builders/Ubuntu%2016.04%20x86_64%20%28TEST%29/builds/9661/steps/shell_6/logs/1.ztest.out

@BrainSlayer @c0d3z3r0 besides the coverage tests zloop (lets away those)... no mutex errors appeared.

PrivatePuffin · 2019-12-04T10:04:03Z

@c0d3z3r0 Coverage test also completed zloop without mutex related errors.

PrivatePuffin · 2019-12-04T13:42:29Z

@c0d3z3r0 That would also do.

if we get the mutex_exit error here now, we know this was the cause.
if we don't we have a nice, functioning branch to fall back to that passes most tests and wil could just add the fixes from brainslayer just to be sure.

PrivatePuffin · 2019-12-04T14:13:32Z

@c0d3z3r0 Everything clear here except coverage test
Which is nice, because it means at least your build-in fix is fine! :)

c0d3z3r0 · 2019-12-04T14:14:23Z

ok, then let's merge in BrainSlayers patches now

PrivatePuffin · 2019-12-04T14:15:03Z

@c0d3z3r0 In that one thats fine... There is no reasonable expectation for a mutex_exit error here atm.

c0d3z3r0 · 2019-12-04T14:26:13Z

applied all... normally they get all tested

PrivatePuffin · 2019-12-04T14:27:36Z

@c0d3z3r0
No issues merging those in?

c0d3z3r0 · 2019-12-04T14:28:30Z

No issues merging those in?

no, this branch was missing my refactoring

PrivatePuffin · 2019-12-04T14:38:01Z

@c0d3z3r0 When this works, I suggest closing this draft PR and continue the work on the other one...

c0d3z3r0 · 2019-12-04T14:40:34Z

@c0d3z3r0 When this works, I suggest closing this draft PR and continue the work on the other one...

I don't want to do too much spam... I'd close the other, continue work here and just open a new PR for final review

PrivatePuffin · 2019-12-04T14:42:51Z

Lets do that indeed...
Its too much spam to click through already.

so Ack.

PrivatePuffin · 2019-12-04T16:19:28Z

@c0d3z3r0 Two failures in zloop currently.
None mutex related.

BrainSlayer · 2019-12-04T19:58:52Z

no. it was also no crash. looks like a irregular regular exit due io error

PrivatePuffin · 2019-12-04T20:09:25Z

@BrainSlayer Thats what I just said smartass.

PrivatePuffin · 2019-12-04T20:09:45Z

@c0d3z3r0 Tests finished, we can continue safely :)

BrainSlayer · 2019-12-04T21:06:02Z

@c0d3z3r0 not yet. you missed a patch from my branch a967990. so ensure that really all is included. for me it looks only this is missing. this patch will trigger a assert if the memory header is not properly initalized. this is better than raising a crashdump.

PrivatePuffin · 2019-12-04T21:39:43Z

Okey...
@c0d3z3r0
As discussed, the tests are both clean... so we can continue on this branch....

Before we start implementing your refactor one-by-one, 2 things needs to be done:

rebase to include my test refactor (just been merged into master)
implementation of @BrainSlayer his a967990 commit.

After those are merged we can readd the refactor points one by one...

c0d3z3r0 · 2019-12-04T23:17:38Z

are you kidding me? ofc I have merged that commit.... have a close look again

c0d3z3r0 · 2019-12-05T08:09:49Z

@BrainSlayer ping

PrivatePuffin · 2019-12-05T08:13:17Z

Morning @c0d3z3r0
Lets get this rebased and the refactor im shallwe? :)

c0d3z3r0 · 2019-12-05T08:56:42Z

Morning @c0d3z3r0
Lets get this rebased and the refactor im shallwe? :)

sure, but a bit later; day job ;)

BrainSlayer · 2019-12-05T09:16:07Z

@c0d3z3r0 dont bother. i just thought it was missing since zstd_free still had zstd_kmem_unknown as case. at the end i found out that i missed it. this is why i added a new patch (the one which solves the memleak) last evening.

BrainSlayer · 2019-12-05T09:18:11Z

and yes. now we may refactor everything. i can also do some work in advance. for instance i want to get rid of the real_zstd_decompress functions etc. i see no reason to split it into 2 sub functions here. could be merged etc. but not that important. its just for easier reading

PrivatePuffin · 2019-12-05T10:04:49Z

@BrainSlayer
I think you should holdoff on refactoring yourself.
@c0d3z3r0 is just going to port (parts) of his previous refactor and it would be easier if you add any changes on top of that instead of changing the source of that refactor (your repo) even more.

BrainSlayer · 2019-12-05T10:08:29Z

since the majority of the refactoring process he did is just style based it could be done from scratch. especially if i rewrite functions from the ground. just some thoughts

c0d3z3r0 · 2019-12-05T10:46:39Z

since the majority of the refactoring process he did is just style based it could be done from scratch. especially if i rewrite functions from the ground. just some thoughts

well, I would start adding my changes one by one (until it breaks again \o/), but please don't let's work concurrent, as we both will have to rebase again and again. let me finish here and then let us BOTH start from THE SAME codebase :)

c0d3z3r0 · 2019-12-15T00:41:50Z

I bet it's clang :)

@c0d3z3r0 I do get A LOT of feedback from my old pal Google, telling me CLANG IS A BITCH WITH THIS.

Oh, it could be Linux kernel compile-time checks, too (CONFIG_STACK_VALIDATION)

PrivatePuffin · 2019-12-15T01:07:22Z

But lets be clear... @dan-and what we are discussing is if we can hide this. It indeed seems to bea related error to the other hidden one...

PrivatePuffin · 2019-12-15T08:28:50Z

Some good news:
I've found a way to ignore folders from codecov.
I still need to do some testing to find the cleanest way, but it's going somewhere.

dan-and · 2019-12-15T08:36:10Z

I will remove all "well known" parts, which have nothing to do with the PR. Give me a bit of time to complete this.

Updated with details to compiler and kernel version.

BrainSlayer · 2019-12-15T09:16:48Z

objtool warnings can be ignored. i do compile zfs with gcc 9.1 bzw. on kernel 5.4
objtool is a kernel tool which is used to generate strack trace informations. this warning you see will only occure if the ORC unwinder is activated in kernel config. i have not seen yet warnings like unreachable instruction with latest kernel and latest gcc. but its also harmless. objtool is not a bug free tool and does only generate metadata for the orc unwinder which is used to show stack trace informations if a kernel crash occures. so if a newer gcc does generate warnings with objtool. objtool must be fixed but not our code

BrainSlayer · 2019-12-15T09:32:45Z

see also #6950

dan-and · 2019-12-15T09:44:16Z

Thanks @BrainSlayer
Then everything is fine :-)

PrivatePuffin · 2019-12-15T15:16:32Z

Okey, a headsup:
codecoverage work is going fine.

I have found a way to cleanly exclude paths from code coverage.
But said solution was a dirty patch that wasn't going to get merged.

Now I'm doing some small refactoring and syntax tests on said solution to find the cleanest and maintanable solution for the ignores.

I estimate i'll have a good PR in tomorrow, which I should be able to get merged in 1 or 2 days, as its relatively simple. Together with that PR i'll send @c0d3z3r0 a PR to add the ignore here too. We can clean the duplicates during rebase quite easily as it is just a few rows.

I'll also make sure the PR description file gets updated accordingly.

That would delay schedule a bit, but 2-3 days won't be a problem I think :)
But we should be able to put the PR up the moment the codecoverage PR gets merged 👍

c0d3z3r0 · 2019-12-15T15:18:18Z

Okey, a headsup:
codecoverage work is going fine.

I have found a way to cleanly exclude paths from code coverage.
But said solution was a dirty patch that wasn't going to get merged.

Now I'm doing some small refactoring and syntax tests on said solution to find the cleanest and maintanable solution for the ignores.

I estimate i'll have a good PR in tomorrow, which I should be able to get merged in 1 or 2 days, as its relatively simple. Together with that PR i'll send @c0d3z3r0 a PR to add the ignore here too. We can clean the duplicates during rebase quite easily as it is just a few rows.

I'll also make sure the PR description file gets updated accordingly.

That would delay schedule a bit, but 2-3 days won't be a problem I think :)
But we should be able to put the PR up the moment the codecoverage PR gets merged

Perfect! :)

PrivatePuffin · 2019-12-15T18:14:13Z

Preliminary codecoverage ignore fix is ready here: #9726

It am not 100% certain the quoting when using ?= and += is going to work out completely.
But for all these issues I know the workarounds already and I think it might work the way it is.

@c0d3z3r0 I don't think I even need to write you the oneliner you need to ignore zstd if you read my changes, do I? ;)
(keep in mind the * in: "*/contrib/zstd/*" ;) )

Do remember this replaces(!) the previous ignore fix!

c0d3z3r0 · 2019-12-15T18:20:52Z

@Ornias1993 very nice :) integrated

c0d3z3r0 · 2019-12-15T18:21:41Z

oops.. newline f**+up

- Removes broken codecov ignore - Places ignore section ax_code_coverage - Modifies LCOV command to deal with multiple ignored paths - Forwards users from codecov to LCOV for ignores Signed-off-by: Kjeld Schouten-Lebbing <[email protected]> Signed-off-by: Michael Niewöhner <[email protected]>

c0d3z3r0 · 2019-12-15T18:23:17Z

oh damn.... one shouldn't touch code, while hungry.... -.-

This imports the unmodified ZStandard source to contrib/ which will be used by ZFS. This code shall not be modified in any way to keep it easily updatable. Only the required files from lib/ are imported. contrib/zstd is excluded from codecov calculation as dependencies don't need full codecov. Signed-off-by: Allan Jude <[email protected]> Signed-off-by: Kjeld Schouten-Lebbing [email protected] Signed-off-by: Michael Niewöhner <[email protected]>

These files make ZStandard compile as a kernel module without modifying the ZStandard source files. Removed unexpanded SVN version tracking tags from FreeBSD Signed-off-by: Allan Jude <[email protected]> Signed-off-by: Michael Niewöhner <[email protected]>

Extend APIs that deals with compression to also pass the compression level. Signed-off-by: Allan Jude <[email protected]> Signed-off-by: Sebastian Gottschall <[email protected]> Signed-off-by: Michael Niewöhner <[email protected]>

Signed-off-by: Allan Jude <[email protected]> Signed-off-by: Sebastian Gottschall <[email protected]> Signed-off-by: Kjeld Schouten-Lebbing <[email protected]> Signed-off-by: Michael Niewöhner <[email protected]>

This add a new build intermediary libzstd. This allows us to keep zstd cleanly separated from libzpool and to override the frame-size compiler option without for zstd only. (This is needed due to unused coude in the unmodified zstd source.) Signed-off-by: Michael Niewöhner <[email protected]>

Add zstd to the test suite - add zstd to history_002_pos.ksh - add random levels of zstd to history_002_pos.ksh - add zstd-fast to history_002_pos.ksh - add random levels of zstd-fast to history_002_pos.ksh Add documentation - add man page content - add README for contrib/zstd Fixup copyright headers of touched and new files - Adds copyright headers for Allan Jude / Klara Inc. - Added Ornias1993 Copyright - Cleans copyright header formatting Signed-off-by: Allan Jude <[email protected]> Signed-off-by: Sebastian Gottschall <[email protected]> Signed-off-by: Kjeld Schouten-Lebbing <[email protected]> Signed-off-by: Michael Niewöhner <[email protected]>

BrainSlayer · 2019-12-15T20:41:47Z

i should not code when i'm thirsty. especially on saturday nights

PrivatePuffin · 2019-12-16T14:02:19Z

@c0d3z3r0
Multi line variables for this ignore didn't work out, had to oneline it to get it working...
Please follow the change I made in #9726

That PR is now out for final review too btw, so drop a quick review if you like :)
(don't start bitching about grammar please, that also distracted people in my previous PR)

@c0d3z3r0 What you could do:

Drop the current commit you added.
Just add : "/contrib/zstd/" to the line I used.
Remove the ignore in codecov.yml
Just rebase if there is something to rebase.

You should be able to do this without my PR being merged, those two do not rely on eachother anymore :)

@c0d3z3r0 and @BrainSlayer I did some CPPCHECK tests and the issues that are there are from the library and are fixed in upcoming ZSTD 1.4.5

No new relevant issues it seems

I also did additional testing on different values for pool_count = (boot_ncpus * 4);

I've seen no statistically relevant performance differences between the following values:
pool_count = (boot_ncpus * 4);
pool_count = (boot_ncpus * 8);
pool_count = (boot_ncpus * 12);
pool_count = (boot_ncpus * 16);

So the guestimated 4 value is perfectly fine! :)

c0d3z3r0 · 2019-12-16T23:51:22Z

#9735

BrainSlayer · 2019-12-17T05:15:18Z

try *2 or *1 :-) i have choosen 4 but believe 2 is enough. so even 4 was already a "i'm afraid" value :-)

what issues do you refer too with CPPCHECK?

PrivatePuffin · 2019-12-17T07:56:27Z

@BrainSlayer I already discussed any issues with the folks at zstd.
It has no relevance to discuss already fixed issues with someone from another project.

c0d3z3r0 mentioned this pull request Dec 4, 2019

[DO NOT MERGE] zstd test #9655

Closed

c0d3z3r0 changed the title ~~Zstd nomutexcrash~~ [TEST | DO NOT MERGE]Zstd nomutexcrash Dec 4, 2019

c0d3z3r0 force-pushed the zstd_nomutexcrash branch from aece48d to 6e2c7ab Compare December 15, 2019 18:20

c0d3z3r0 force-pushed the zstd_nomutexcrash branch from 6e2c7ab to b4c8d70 Compare December 15, 2019 18:22

c0d3z3r0 added 6 commits December 15, 2019 19:25

Add zstd support to zfs

f4e47e1

Signed-off-by: Allan Jude <[email protected]> Signed-off-by: Sebastian Gottschall <[email protected]> Signed-off-by: Kjeld Schouten-Lebbing <[email protected]> Signed-off-by: Michael Niewöhner <[email protected]>

c0d3z3r0 force-pushed the zstd_nomutexcrash branch from b4c8d70 to e1f088b Compare December 15, 2019 18:26

c0d3z3r0 closed this Dec 16, 2019

PrivatePuffin mentioned this pull request Dec 17, 2019

Introduce ZSTD compression to ZFS #9735

Closed

12 tasks

[WIP | DO NOT MERGE] zstd restructuring #9673

[WIP | DO NOT MERGE] zstd restructuring #9673

Conversation

c0d3z3r0 commented Dec 4, 2019 • edited Loading

PrivatePuffin commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019 • edited Loading

PrivatePuffin commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019 • edited Loading

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

BrainSlayer commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

BrainSlayer commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019

c0d3z3r0 commented Dec 5, 2019

PrivatePuffin commented Dec 5, 2019

c0d3z3r0 commented Dec 5, 2019

BrainSlayer commented Dec 5, 2019

BrainSlayer commented Dec 5, 2019

PrivatePuffin commented Dec 5, 2019

BrainSlayer commented Dec 5, 2019

c0d3z3r0 commented Dec 5, 2019

c0d3z3r0 commented Dec 15, 2019

PrivatePuffin commented Dec 15, 2019

PrivatePuffin commented Dec 15, 2019

dan-and commented Dec 15, 2019 • edited Loading

BrainSlayer commented Dec 15, 2019

BrainSlayer commented Dec 15, 2019

dan-and commented Dec 15, 2019

PrivatePuffin commented Dec 15, 2019

c0d3z3r0 commented Dec 15, 2019

PrivatePuffin commented Dec 15, 2019 • edited Loading

c0d3z3r0 commented Dec 15, 2019

c0d3z3r0 commented Dec 15, 2019

c0d3z3r0 commented Dec 15, 2019

BrainSlayer commented Dec 15, 2019

PrivatePuffin commented Dec 16, 2019 • edited Loading

c0d3z3r0 commented Dec 16, 2019

BrainSlayer commented Dec 17, 2019

PrivatePuffin commented Dec 17, 2019

c0d3z3r0 commented Dec 4, 2019 •

edited

Loading

PrivatePuffin commented Dec 4, 2019 •

edited

Loading

PrivatePuffin commented Dec 4, 2019 •

edited

Loading

dan-and commented Dec 15, 2019 •

edited

Loading

PrivatePuffin commented Dec 15, 2019 •

edited

Loading

PrivatePuffin commented Dec 16, 2019 •

edited

Loading