avoid symbol collision with in-kernel zstdlib #10775

BrainSlayer · 2020-08-22T10:47:52Z

In case zfs is compiled as in kernel static variant
and the in-kernel zstd library is compiled static into the kernel, a
symbol collision will occur. this wrapper header does rename all
relevant zstd function to avoid this problem

closes #10763

Signed-off-by: Sebastian Gottschall s.gottschall@dd-wrt.com

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

Ornias1993 · 2020-08-22T10:49:18Z

Clean PR, nice work @BrainSlayer

BrainSlayer · 2020-08-22T10:52:22Z

@Ornias1993 first time for me that something is not dirty :-)

Ornias1993 · 2020-08-22T10:54:46Z

@BrainSlayer Lets not go there shall we ;-)

Question: there is no option to wildcard define in header files?

BrainSlayer · 2020-08-22T10:58:28Z

not that i know. you can do something like
#define RENAME(func) zfs_ ## func

but this will not help since i cannot do #define RENAME(func) #define func zfs_ ## func

i just used "nm" and a awk script to generate the header. so its simple to renew it

BrainSlayer · 2020-08-22T11:00:28Z

hint: the same problem might affect freebsd which has also a in kernel zstd library. but zfs isnt build yet static into the freebsd kernel. but i wait for comments first

Ornias1993 · 2020-08-22T11:02:03Z

@BrainSlayer Good points indeed.
Next step (if confirmed working fine) would indeed be documenting and adding any required scripting, to keep updating solid

BrainSlayer · 2020-08-22T11:14:05Z

@BrainSlayer Good points indeed.
Next step (if confirmed working fine) would indeed be documenting and adding any required scripting, to keep updating solid

not usefull since i hand crafted it after generating. i needed to remove compiler generated symbols and the function zstd_isError which would collide with another macro definition in zstd.h. and i needed to modify the result for passing cstyle check of zfs

nm zstd.o | awk '{print "#define "$3 " zfs_" $3}' > rename.h

lib/libzstd/Makefile.am

nivedita76 · 2020-08-22T15:28:57Z

It builds for me. Thanks @BrainSlayer

BrainSlayer · 2020-08-22T16:49:06Z

It builds for me. Thanks @BrainSlayer

thx. will set it to review now and i hope it gets merged soon

behlendorf

Thanks for sorting this out and opening a PR.

module/zstd/include/zstd_compat_wrapper.h

codecov · 2020-08-22T17:52:08Z

Codecov Report

Merging #10775 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10775      +/-   ##
==========================================
- Coverage   79.62%   79.60%   -0.03%     
==========================================
  Files         395      395              
  Lines      125039   125039              
==========================================
- Hits        99567    99541      -26     
- Misses      25472    25498      +26

Flag	Coverage Δ
#kernel	`80.40% <ø> (+0.02%)`	⬆️
#user	`65.45% <ø> (+0.48%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/zfs/vdev_indirect.c	`73.54% <0.00%> (-10.65%)`	⬇️
module/zcommon/zfs_fletcher.c	`75.65% <0.00%> (-2.64%)`	⬇️
module/zfs/vdev_raidz_math.c	`76.57% <0.00%> (-2.26%)`	⬇️
module/os/linux/zfs/vdev_file.c	`80.55% <0.00%> (-1.86%)`	⬇️
module/zfs/zio_compress.c	`92.72% <0.00%> (-1.82%)`	⬇️
cmd/zed/agents/fmd_api.c	`88.61% <0.00%> (-1.78%)`	⬇️
module/os/linux/spl/spl-kmem-cache.c	`88.57% <0.00%> (-1.30%)`	⬇️
lib/libzpool/util.c	`94.93% <0.00%> (-1.27%)`	⬇️
module/zfs/zap_micro.c	`85.35% <0.00%> (-1.26%)`	⬇️
cmd/zed/agents/zfs_diagnosis.c	`77.55% <0.00%> (-1.17%)`	⬇️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07ce896...6ff1349. Read the comment docs.

Ornias1993

Please also add an explanation to the readme, describing the fact this new headerfile needs to be updated accordingly when we update to new ZSTD versions .

Besides those points this looks good and build, thanks for the effort @BrainSlayer and glad to have this issue fixed with due priority! 🥇

BrainSlayer · 2020-08-22T22:28:56Z

Please also add an explanation to the readme, describing the fact this new headerfile needs to be updated accordingly when we update to new ZSTD versions .

Besides those points this looks good and build, thanks for the effort @BrainSlayer and glad to have this issue fixed with due priority! 🥇

it doesnt need to be updated as far as it covers the symbols used in the kernel and this is unlikelly to be changed very soon. i rename much more symbols than required

Ornias1993 · 2020-08-22T22:34:49Z

Please also add an explanation to the readme, describing the fact this new headerfile needs to be updated accordingly when we update to new ZSTD versions .
Besides those points this looks good and build, thanks for the effort @BrainSlayer and glad to have this issue fixed with due priority! 🥇

it doesnt need to be updated as far as it covers the symbols used in the kernel and this is unlikelly to be changed very soon. i rename much more symbols than required

It needs a short and simple single scentence as a reminder. Remember: This is meant for years from now too. Something like:
ZSTD-on-ZFS might, in some cases, collide with in-kernel ZSTD. If this is the case, please update zstd_compat_wrapper.h

BrainSlayer · 2020-08-23T02:37:38Z

Please also add an explanation to the readme, describing the fact this new headerfile needs to be updated accordingly when we update to new ZSTD versions .
Besides those points this looks good and build, thanks for the effort @BrainSlayer and glad to have this issue fixed with due priority! 🥇

it doesnt need to be updated as far as it covers the symbols used in the kernel and this is unlikelly to be changed very soon. i rename much more symbols than required

It needs a short and simple single scentence as a reminder. Remember: This is meant for years from now too. Something like:
ZSTD-on-ZFS might, in some cases, collide with in-kernel ZSTD. If this is the case, please update zstd_compat_wrapper.h

not my style. i will not tell 3rd parties to update headers to stay compatible. thats our responsibility. i just said its not likelly that the in-kernel zstd changes the symbol names nor will they add new names. this wrapper is a workarround for all zstd symbols used by zfs. so we are safe, since my wrapper renamed all symbols in use by zfs

Ornias1993 · 2020-08-23T09:20:12Z

not my style. i will not tell 3rd parties to update headers to stay compatible. thats our responsibility.

The update readme is meant to document all issues that might arrise from updating, this is one of them. It's meant for people in year, 5 years, 10 years or whatever years from now if we are not there to maintain it anymore (which isn't a guarantee)

However:
I don't think it's acceptable to hold this patch due to this disagreement on a formality, so i'm okey with merging it as-is.

behlendorf · 2020-08-24T04:21:45Z

@BrainSlayer when you get a chance to refresh this and add the short comment to zstd_compat_wrapper.h you'll also need to rebase it on master to resolve the small conflict.

BrainSlayer · 2020-08-24T07:46:45Z

@behlendorf i rebased it, compile checked it and added some small documentation to the header

BrainSlayer · 2020-08-24T07:51:12Z

not my style. i will not tell 3rd parties to update headers to stay compatible. thats our responsibility.

The update readme is meant to document all issues that might arrise from updating, this is one of them. It's meant for people in year, 5 years, 10 years or whatever years from now if we are not there to maintain it anymore (which isn't a guarantee)

However:
I don't think it's acceptable to hold this patch due to this disagreement on a formality, so i'm okey with merging it as-is.

yes, but this is not neccessary. since we rename all symbols locally in use by zfs, nothing will happen in the kernel lib will be changed. our zfs implementation still has everything renamed. if we update zstd in future within zfs, we need usually to check if we need to update the symbols. i added also a small comment to the header about it

Ornias1993 · 2020-08-24T09:14:18Z

We need usually to check if we need to update the symbols. i added also a small comment to the header about it

We made a seperate Readme listing all the things that need to be checked when updating the integrated zstd lib. as you said: this is one of those.
If you dont add it im fine with it and will PR it myself, ive no interest in debating this any further.

BrainSlayer · 2020-08-24T11:19:10Z

We need usually to check if we need to update the symbols. i added also a small comment to the header about it

We made a seperate Readme listing all the things that need to be checked when updating the integrated zstd lib. as you said: this is one of those.
If you dont add it im fine with it and will PR it myself, ive no interest in debating this any further.

README updated

Ornias1993 · 2020-08-24T11:35:10Z

@BrainSlayer Thank you! :)
I think you forgot to add the file to the commit though ;)

BrainSlayer · 2020-08-24T11:44:39Z

better?

Ornias1993 · 2020-08-24T11:50:57Z

@BrainSlayer Yeah, I see the file now.
Looks perfectly.

LGTM. :)

aidanharris · 2020-08-24T12:05:30Z

@BrainSlayer Is this error related or a separate issue?

ld.bfd: fs/btrfs/zstd.o: in function `zstd_decompress':                                                  
/var/tmp/portage/sys-kernel/gentoo-5.8.3/work/linux-5.8.3-gentoo/fs/btrfs/zstd.c:627: multiple definition of `zstd_decompress'; fs/zfs/zstd/zfs_zstd.o:/var/tmp/portage/sys-kernel/gentoo-5.8.3/work/linux-5.8.3-gentoo/fs/zfs/zstd/zfs_zstd.c:526: first defined here

I'm assuming this happens because I'm building both btrfs and zfs statically and they both define a zstd_decompress function in which case building btrfs as a module might fix this (I don't need it statically anymore since I only have one btrfs drive now and my rootfs is on zfs, I've just always done it that way and never changed my kernel config)?

BrainSlayer · 2020-08-24T12:26:57Z

@BrainSlayer Is this error related or a separate issue?
ld.bfd: fs/btrfs/zstd.o: in function `zstd_decompress':       

you got it. this happens if you staticly compile zfs into the kernel. but your error extends this bug, since btrfs seem to use the same symbol name as zfs. let me update my patch to cover your issue

/var/tmp/portage/sys-kernel/gentoo-5.8.3/work/linux-5.8.3-gentoo/fs/btrfs/zstd.c:627: multiple definition of `zstd_decompress'; fs/zfs/zstd/zfs_zstd.o:/var/tmp/portage/sys-kernel/gentoo-5.8.3/work/linux-5.8.3-gentoo/fs/zfs/zstd/zfs_zstd.c:526: first defined here
I'm assuming this happens because I'm building both btrfs and zfs statically and they both define a zstd_decompress function in which case building btrfs as a module might fix this (I don't need it statically anymore since I only have one btrfs drive now and my rootfs is on zfs, I've just always done it that way and never changed my kernel config)?

Ornias1993 · 2020-08-24T12:39:29Z

@BrainSlayer
Could you clearly link this with #10763 and add closes #10763 to the PR description, so it's clear for everyone that this is the fix for issue #10763 ...

BrainSlayer · 2020-08-24T12:49:40Z

can you link it? i dont know how to link the PR to this issue. i commited now a fix for the other issue to and renamed the zstd_decompess/zstd_compress functions to be prefixed with zfs_

Ornias1993 · 2020-08-24T12:53:01Z

@BrainSlayer Seems you did it alright 👍

BrainSlayer · 2020-08-24T12:56:45Z

@BrainSlayer Is this error related or a separate issue?
ld.bfd: fs/btrfs/zstd.o: in function `zstd_decompress':                                                  
/var/tmp/portage/sys-kernel/gentoo-5.8.3/work/linux-5.8.3-gentoo/fs/btrfs/zstd.c:627: multiple definition of `zstd_decompress'; fs/zfs/zstd/zfs_zstd.o:/var/tmp/portage/sys-kernel/gentoo-5.8.3/work/linux-5.8.3-gentoo/fs/zfs/zstd/zfs_zstd.c:526: first defined here 
I'm assuming this happens because I'm building both btrfs and zfs statically and they both define a zstd_decompress function in which case building btrfs as a module might fix this (I don't need it statically anymore since I only have one btrfs drive now and my rootfs is on zfs, I've just always done it that way and never changed my kernel config)?

please check if this PR does solve your issue. to make sure its ready for beeing merged

in case zfs is compiled as in kernel static variant and the in kernel zstd library is compiled static into the kernel, a symbol collision will occur. this wrapper header does rename all relevant zstd functions to avoid this problem Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>

aidanharris · 2020-08-24T13:15:46Z

It builds for me now with CONFIG_ZFS=y and CONFIG_BTRFS_FS=y.

Thanks, @BrainSlayer.

For Linux, when zfs is compiled as an in kernel static variant and the in kernel zstd library is compiled statically into the kernel a symbol collision will occur. This wrapper header renames all of the relevant zstd functions to avoid this problem. Reviewed-by: Kjeld Schouten <kjeld@schouten-lebbing.nl> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com> Closes openzfs#10775

BrainSlayer mentioned this pull request Aug 22, 2020

In-tree ZFS-ZSTD clash with in-kernel ZSTD #10763

Closed

BrainSlayer changed the title ~~avoid symbol collission with in-kernel zstdlib~~ avoid symbol collision with in-kernel zstdlib Aug 22, 2020

BrainSlayer force-pushed the zstd_symbol_rename branch from ba080d2 to 2df4a81 Compare August 22, 2020 10:49

BrainSlayer force-pushed the zstd_symbol_rename branch 2 times, most recently from b3c5d1a to a0fee60 Compare August 22, 2020 10:51

nivedita76 reviewed Aug 22, 2020

View reviewed changes

lib/libzstd/Makefile.am Show resolved Hide resolved

BrainSlayer marked this pull request as ready for review August 22, 2020 16:49

behlendorf approved these changes Aug 22, 2020

View reviewed changes

behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 22, 2020

behlendorf reviewed Aug 22, 2020

View reviewed changes

module/zstd/include/zstd_compat_wrapper.h Show resolved Hide resolved

Ornias1993 suggested changes Aug 22, 2020

View reviewed changes

Ornias1993 approved these changes Aug 23, 2020

View reviewed changes

BrainSlayer force-pushed the zstd_symbol_rename branch 3 times, most recently from 348f0b6 to ad3c115 Compare August 24, 2020 07:41

BrainSlayer force-pushed the zstd_symbol_rename branch from ad3c115 to 5eb4cfb Compare August 24, 2020 07:54

BrainSlayer force-pushed the zstd_symbol_rename branch from 5eb4cfb to 3a9cc7d Compare August 24, 2020 11:19

BrainSlayer force-pushed the zstd_symbol_rename branch from 3a9cc7d to 777e308 Compare August 24, 2020 11:44

BrainSlayer force-pushed the zstd_symbol_rename branch from 777e308 to b4f279c Compare August 24, 2020 12:48

BrainSlayer force-pushed the zstd_symbol_rename branch from b4f279c to 0bac120 Compare August 24, 2020 12:56

BrainSlayer force-pushed the zstd_symbol_rename branch from 0bac120 to 6ff1349 Compare August 24, 2020 13:04

behlendorf approved these changes Aug 24, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 24, 2020

behlendorf merged commit 184df27 into openzfs:master Aug 24, 2020

tsjk mentioned this pull request Feb 18, 2021

ZSTD-related crashes after many iterations of a "bad task" #11386

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid symbol collision with in-kernel zstdlib #10775

avoid symbol collision with in-kernel zstdlib #10775

BrainSlayer commented Aug 22, 2020 •

edited

Ornias1993 commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020

Ornias1993 commented Aug 22, 2020 •

edited

BrainSlayer commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020

Ornias1993 commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020 •

edited

nivedita76 commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020

behlendorf left a comment

codecov bot commented Aug 22, 2020 •

edited

Ornias1993 left a comment

BrainSlayer commented Aug 22, 2020 •

edited

Ornias1993 commented Aug 22, 2020

BrainSlayer commented Aug 23, 2020

Ornias1993 commented Aug 23, 2020

behlendorf commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

aidanharris commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020 •

edited

aidanharris commented Aug 24, 2020

avoid symbol collision with in-kernel zstdlib #10775

avoid symbol collision with in-kernel zstdlib #10775

Conversation

BrainSlayer commented Aug 22, 2020 • edited

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

Ornias1993 commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020

Ornias1993 commented Aug 22, 2020 • edited

BrainSlayer commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020

Ornias1993 commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020 • edited

nivedita76 commented Aug 22, 2020

BrainSlayer commented Aug 22, 2020

behlendorf left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2020 • edited

Codecov Report

Ornias1993 left a comment

Choose a reason for hiding this comment

BrainSlayer commented Aug 22, 2020 • edited

Ornias1993 commented Aug 22, 2020

BrainSlayer commented Aug 23, 2020

Ornias1993 commented Aug 23, 2020

behlendorf commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

aidanharris commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020

Ornias1993 commented Aug 24, 2020

BrainSlayer commented Aug 24, 2020 • edited

aidanharris commented Aug 24, 2020

BrainSlayer commented Aug 22, 2020 •

edited

Ornias1993 commented Aug 22, 2020 •

edited

BrainSlayer commented Aug 22, 2020 •

edited

codecov bot commented Aug 22, 2020 •

edited

BrainSlayer commented Aug 22, 2020 •

edited

BrainSlayer commented Aug 24, 2020 •

edited