[DO NOT MERGE] zstd test #9655

c0d3z3r0 · 2019-12-01T11:42:56Z

just run the tests

PrivatePuffin · 2019-12-01T12:50:15Z

/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/../zfs-0.8.0/contrib/zstd/zstd-1.4.4/lib/zstd.h:18:10: fatal error: limits.h: No such file or directory
 #include <limits.h>   /* INT_MAX */

c0d3z3r0 · 2019-12-01T12:52:04Z

@c0d3z3r0

/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/../zfs-0.8.0/contrib/zstd/zstd-1.4.4/lib/zstd.h:18:10: fatal error: limits.h: No such file or directory
 #include <limits.h>   /* INT_MAX */

yep... I'm not sure why the include works on my machine and for the BUILDs but not for the TESTs....

PrivatePuffin · 2019-12-01T13:08:17Z

@c0d3z3r0 THis is your problem:

if CONFIG_USER
libzfsdir = $(includedir)/libzfs/sys/zstd
libzfs_HEADERS = $(COMMON_H) $(USER_H)
endif

if CONFIG_KERNEL
kerneldir = @prefix@/src/zfs-$(VERSION)/include/sys/zstd
kernel_HEADERS = $(COMMON_H) $(KERNEL_H)
endif

Build:

  CC [M]  /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zstd/zstd.o
  CC [M]  /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zfs/bqueue.o
  CC [M]  /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zstd/../../contrib/zstd/zstd-1.4.4/lib/common/zstd_common.o
  CC [M]  /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zstd/../../contrib/zstd/zstd-1.4.4/lib/common/fse_decompress.o
  CC [M]  /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zstd/../../contrib/zstd/zstd-1.4.4/lib/common/entropy_common.o

Test:

  CC [M]  /tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zstd/zstd.o
  CC [M]  /tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zfs/btree.o
In file included from /tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/../zfs-0.8.0/module/zstd/zstd.c:35:0:
/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/../zfs-0.8.0/contrib/zstd/zstd-1.4.4/lib/zstd.h:18:10: fatal error: limits.h: No such file or directory
 #include <limits.h>   /* INT_MAX */
          ^~~~~~~~~~
compilation terminated.
scripts/Makefile.build:330: recipe for target '/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zstd/zstd.o' failed
make[7]: *** [/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zstd/zstd.o] Error 1
scripts/Makefile.build:604: recipe for target '/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zstd' failed
make[6]: *** [/tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zstd] Error 2
make[6]: *** Waiting for unfinished jobs....

c0d3z3r0 · 2019-12-01T13:29:29Z

@c0d3z3r0 THis is your problem:

if CONFIG_USER
libzfsdir = $(includedir)/libzfs/sys/zstd
libzfs_HEADERS = $(COMMON_H) $(USER_H)
endif

if CONFIG_KERNEL
kerneldir = @prefix@/src/zfs-$(VERSION)/include/sys/zstd
kernel_HEADERS = $(COMMON_H) $(KERNEL_H)
endif

Limits.h is part of the... kerneldir path afaik.

the problem is the way limits.h is included. allan circumvented that by using that BSD compatibility headers, which we want to drop

PrivatePuffin · 2019-12-01T13:33:15Z

Well we wanted to drop them if they wheren't needed....
But I stand corrected, execution paths seem to be fine.

Test@Brainslayer:
CC [M] /tmp/zfs-build-buildbot-2ondghTB/BUILD/zfs-kmod-0.8.0/_kmod_build_4.14.154-128.181.amzn2.x86_64/module/zstd/zstd.o

Build@Brainslayer:
CC [M] /var/lib/buildbot/slaves/zfs/Amazon_2_x86_64__BUILD_/build/zfs/module/zstd/zstd.o

Test@c0d3z3r0
CC [M] /tmp/zfs-build-buildbot-LwpO8Rp6/BUILD/zfs-kmod-0.8.0/_kmod_build_4.15.0-1041-aws/module/zstd/zstd.o

Build@c0d3z3r0
CC [M] /var/lib/buildbot/slaves/zfs/Ubuntu_18_04_x86_64__BUILD_/build/zfs/module/zstd/zstd.o

c0d3z3r0 · 2019-12-01T13:33:45Z

Well we wanted to drop them if they wheren't needed....

they aren't, I just have to get the includes right ;)

PrivatePuffin · 2019-12-01T13:36:07Z

@c0d3z3r0
Fuck i've been stupid.

pkg-kmod pkg-utils Builds kernel binaries.
Which makes a it changing kernelpath.
Under test, linux-headers-generic should make limits.h available.
Under build, libc-dev should make limits.h available.

Normal build path is possibly just the default and thats why removal goes fine on Build.

PrivatePuffin · 2019-12-01T14:21:41Z

@c0d3z3r0
I went over all the code again and Actually I like Allans solution more to be honest. Certainly with Freebsd support arround the corner I think it increases mergeability by NOT removing the freebsd compatibility headers

c0d3z3r0 · 2019-12-01T14:23:29Z

@c0d3z3r0
I went over all the code again and Actually I like Allans solution more to be honest. Certainly with Freebsd support arround the corner I think it increases mergeability by NOT removing the freebsd compatibility headers

Well, freebsd support could be added later, too. I'm just missing some makefile foo atm

c0d3z3r0 · 2019-12-01T14:25:02Z

@c0d3z3r0
I went over all the code again and Actually I like Allans solution more to be honest. Certainly with Freebsd support arround the corner I think it increases mergeability by NOT removing the freebsd compatibility headers

Well, freebsd support could be added later, too. I'm just missing some makefile foo atm

you know what? I don't care... let's see what the others say. I just readd it and get it building

PrivatePuffin · 2019-12-01T14:27:46Z

@c0d3z3r0
Lets say we sell it as a feature, not a bug ;)

ZSTD, now with accidental FreeBSD support.

That being said:
Be aware the references in that makefile are wrong...

There is no:
include/sys/zstd/
in your version.

And those files it references aren't there either.

I think you meant the files from: include/zstd.
Maybe just put them back into: modules/zstd/includes like Allan did?

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]>

c0d3z3r0 · 2019-12-01T15:19:30Z

Maybe just put them back into: modules/zstd/includes like Allan did?

no, that's completely wrong... I found what was missing... as I said.. it was simple Makefile foo :D
Running a test now before I push

c0d3z3r0 · 2019-12-01T16:29:23Z

wtf...

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]>

PrivatePuffin · 2019-12-01T16:40:27Z

@c0d3z3r0 Be aware: The first commits will fail the tests... Only take 9115985 into account!

c0d3z3r0 · 2019-12-01T16:43:11Z

@c0d3z3r0 Be aware: The first commits will fail the tests... Only take 9115985 into account!

yeah, the build failed because of a Makefile mistake

PrivatePuffin · 2019-12-01T16:43:53Z

@c0d3z3r0 I'm interested to see how this run goes :)

.editorconfig

FIXME - adds zstd to history_002_pos.ksh - adds random levels of zstd to history_002_pos.ksh - adds zstd-fast to history_002_pos.ksh - adds random levels of zstd-fast to history_002_pos.ksh - ... 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 · 2019-12-01T18:29:24Z

oh... we need find a fix for built-in builds

PrivatePuffin · 2019-12-01T18:30:57Z

@c0d3z3r0
There are no results yet, the results you see now are for the first commit only!*
It seems

http://build.zfsonlinux.org/builders/Kernel.org%20Built-in%20x86_64%20%28BUILD%29/builds/34305 is the last commit... Builds are all green

c0d3z3r0 · 2019-12-01T18:33:31Z

http://build.zfsonlinux.org/builders/Kernel.org%20Built-in%20x86_64%20%28BUILD%29/builds/34305 is the last commit... Builds are all green

yes, because the module is not there ;)

@c0d3z3r0
There are no results yet, the results you see now are for the first commit only!

of course, see http://build.zfsonlinux.org/builders/Kernel.org%20Built-in%20x86_64%20%28BUILD%29/builds/34332

PrivatePuffin · 2019-12-01T18:34:45Z

@c0d3z3r0 9115985 should be the last commit from this PR, it has all Builds passed. THe last commit has ALL changes included.

http://build.zfsonlinux.org/builders/Kernel.org%20Built-in%20x86_64%20%28BUILD%29/builds/34332
is the first commit from this PR.

(Or am I needlessly getting confused?)

Edit
I am needlessly confused. Git is so unclear at times...
Fix Copyright Headers was the last commit sorry!

BrainSlayer · 2019-12-04T13:23:09Z

and also if you switch between branches. the bug may trigger sometimes or it may not. thats the bad thing on race conditions. its more a question of luck than real a code issue

BrainSlayer · 2019-12-04T13:24:21Z

@c0d3z3r0 unused code is never run, correct. but it makes the space between functions bigger. do you know the difference between long jumps and short jumps (same for calls) in assembler on x86?

c0d3z3r0 · 2019-12-04T13:25:33Z

@c0d3z3r0 unused code is never run, correct. but it makes the space between functions bigger. do you know the difference between long jumps and short jumps (same for calls) in assembler on x86?

thank you! this is the explanation I was waiting for! yes, I know the difference and now I get what you meant ;)

ok, then we could try to compile with -Wl--gc-sections if we can't find the issue finally

BrainSlayer · 2019-12-04T13:29:07Z

-Wl,gc-sections isnt enough. you missed -ffunction-sections -fdata-sections at compile time and it will not help for kernel mode compile. thats just usermode which is not relevant for performance. and we also dont want to shadow this issue. better would it be to find a way to reproduce it quick and 100%

c0d3z3r0 · 2019-12-04T13:29:45Z

better would it be to find a way to reproduce it quick and 100%

and this is, what I am trying here step-by-step

PrivatePuffin · 2019-12-04T13:41:59Z

@c0d3z3r0 I didn't mean reset, sorry...
I meant simply "Go back".

c0d3z3r0 · 2019-12-04T13:54:59Z

\o/ all ztest passed (except Ubuntu, it's still running)

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

@c0d3z3r0 Now i'm interested what actually would be the diff between your two branches...

c0d3z3r0 · 2019-12-04T14:02:37Z

@c0d3z3r0 Now i'm interested what actually would be the diff between your two branches...

you can just do:

git fetch origin refs/pull/9655/head:zstd_reworked
git fetch origin refs/pull/9673/head:zstd_nomuxcrash
git diff zstd_reworked zstd_nomuxcrash

when both branches succeed know, what I expect, then the issue must be in the last revert here

PrivatePuffin · 2019-12-04T14:06:56Z

@c0d3z3r0 Thats quite a big blob of code...
But, it at least narrows it down, so we would be able to squash everything we got now...

I don't think its worth it try to narrow it down even more.
It is starting to put quite the load in the buildsystem to statisfy curiosity.

c0d3z3r0 · 2019-12-04T14:08:20Z

@c0d3z3r0 Thats quite a big blob of code...

not really... actually it's just a few changes but a lot of reordering functions (which in theory should not cause any issues at all)

I don't think its worth it try to narrow it down even more.
It is starting to put quite the load in the buildsystem to statisfy curiosity.

I would go the other way from now on, which is applying small patches to the other branch until we get the problem again

PrivatePuffin · 2019-12-04T14:10:57Z

which in theory should not cause any issues at all

Which, not-in-theory, we spend half a day on fixing the issues just the reshuffle caused alone... ;)

c0d3z3r0 · 2019-12-04T14:12:54Z

which in theory should not cause any issues at all

Which, not-in-theory, we spend half a day on fixing the issues just the reshuffle caused alone... ;)

Well, when I submit a PR, I want the code to be as clean as possible... because I know that I will have to touch the code again in future and there is nothing that I hate more than code-"chaos" (that is not meant offensive!). I simply should have tested much smaller commits from the beginning.. that was the only mistake :/

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

@c0d3z3r0
Ack.

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

@c0d3z3r0 Wise choice just be 200% sure.

PrivatePuffin · 2019-12-04T14:35:37Z

@c0d3z3r0 Might I suggest letting at least the non-coverage tests complete this time?
Might take till the evening, but just to be sure and having a good starting point to continue working on.

c0d3z3r0 · 2019-12-04T14:36:18Z

@c0d3z3r0 Might I suggest letting at least the non-coverage tests complete this time?
Might take till the evening, but just to be sure and having a good starting point to continue working on.

you mean zfstest? ok

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

@c0d3z3r0 No I meant the whole suite.
Which is bascially zfstest + 2 minutes

So actually yes :')

c0d3z3r0 · 2019-12-04T14:37:35Z

ack

PrivatePuffin · 2019-12-04T17:03:33Z

@c0d3z3r0 Awesome, no zloop errors! :)

BrainSlayer · 2019-12-04T17:23:51Z

@c0d3z3r0 my branch succeeded too with the new code and got no single error. not even the ones we normally ignore since we found them unrelated

PrivatePuffin · 2019-12-05T08:54:39Z

@c0d3z3r0
I went over the tests:
2 tests are stuck/failed due to the connection being lost to the buildbot (this seems to be repo wide, so confirmed the issue)
1 test is failed due to unrelated fluke, something seems to have gone wrong while running the test itself.

So I think this can be closed and we can continue on the other branch as planned.

BrainSlayer · 2019-12-05T09:48:33Z

@Ornias1993 the failed ubuntu test looks like a memory allocation failure (not in zstd), so some sort of out of memory

Revert "revert more..." This reverts commit 8ae6322.

c0d3z3r0 · 2019-12-05T11:51:44Z

\o/ no crash so far, so this all was random.... the culprit is not in the reverted commit

PrivatePuffin · 2019-12-05T11:53:47Z

@c0d3z3r0 This could just as well be random luck.
lets focus on the other branch and just add them one by one.

This really is a waste of this repo's buildsystem just close this and start adding one by one.

c0d3z3r0 · 2019-12-05T11:54:56Z

@c0d3z3r0 This could just as well be random luck.
lets focus on the other branch and just add them one by one.

This really is a waste of this repo's buildsystem just close this and start adding one by one.

ack

c0d3z3r0 force-pushed the zstd_reworked branch 2 times, most recently from 0a5e786 to 89b4ad6 Compare December 1, 2019 12:21

c0d3z3r0 force-pushed the zstd_reworked branch from 89b4ad6 to 3545923 Compare December 1, 2019 16:14

c0d3z3r0 added 3 commits December 1, 2019 17:32

Add zstd support to zfs

d1ad340

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_reworked branch from 3545923 to 79d6340 Compare December 1, 2019 16:32

PrivatePuffin reviewed Dec 1, 2019

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

no change. just test again.

b8d3e81

Revert the revert. I want to see it crash.

56c09a1

Revert "revert more..." This reverts commit 8ae6322.

c0d3z3r0 closed this Dec 5, 2019

c0d3z3r0 mentioned this pull request Dec 13, 2019

[WIP | DO NOT MERGE] zstd restructuring #9673

Closed

[DO NOT MERGE] zstd test #9655

[DO NOT MERGE] zstd test #9655

Conversation

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019 • edited Loading

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019 • edited Loading

PrivatePuffin commented Dec 1, 2019

c0d3z3r0 commented Dec 1, 2019

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019 • edited Loading

c0d3z3r0 commented Dec 1, 2019

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019 • edited Loading

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019 • edited Loading

c0d3z3r0 commented Dec 1, 2019

PrivatePuffin commented Dec 1, 2019 • edited Loading

BrainSlayer commented Dec 4, 2019

BrainSlayer commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019 • edited Loading

BrainSlayer 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 • edited Loading

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019 • edited Loading

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019 • edited Loading

PrivatePuffin commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

c0d3z3r0 commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019 • edited Loading

c0d3z3r0 commented Dec 4, 2019

PrivatePuffin commented Dec 4, 2019

BrainSlayer commented Dec 4, 2019

PrivatePuffin commented Dec 5, 2019

BrainSlayer commented Dec 5, 2019

c0d3z3r0 commented Dec 5, 2019

PrivatePuffin commented Dec 5, 2019

c0d3z3r0 commented Dec 5, 2019

PrivatePuffin commented Dec 1, 2019 •

edited

Loading

PrivatePuffin commented Dec 1, 2019 •

edited

Loading

PrivatePuffin commented Dec 1, 2019 •

edited

Loading

PrivatePuffin commented Dec 1, 2019 •

edited

Loading

PrivatePuffin commented Dec 1, 2019 •

edited

Loading

PrivatePuffin commented Dec 1, 2019 •

edited

Loading

c0d3z3r0 commented Dec 4, 2019 •

edited

Loading

c0d3z3r0 commented Dec 4, 2019 •

edited

Loading

c0d3z3r0 commented Dec 4, 2019 •

edited

Loading

c0d3z3r0 commented Dec 4, 2019 •

edited

Loading

PrivatePuffin commented Dec 4, 2019 •

edited

Loading