Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
nivedita76 opened this issue Aug 21, 2020 · 78 comments · Fixed by #10775
Closed

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

nivedita76 opened this issue Aug 21, 2020 · 78 comments · Fixed by #10775

Comments

@nivedita76
Copy link
Contributor

nivedita76 commented Aug 21, 2020

System information

Type Version/Name
Distribution Name
Distribution Version
Linux Kernel
Architecture
ZFS Version
SPL Version

Describe the problem you're observing

ZSTD is already in the linux kernel, and it produces multiple definition errors if ZSTD is builtin to the kernel and ZFS is being compiled as builtin.

Describe how to reproduce the problem

Include any warning/errors/backtraces from the system logs

@thehaven
Copy link

thehaven commented Aug 21, 2020

Seeing this also against both linux-5.8.0-gentoo-r1 and gentoo-sources-5.8.2, example below is from linux-5.8.0-gentoo-r1:

saratoga /usr/src/linux # grep ZSTD .config
# CONFIG_ZSWAP_COMPRESSOR_DEFAULT_ZSTD is not set
# CONFIG_SQUASHFS_ZSTD is not set
# CONFIG_PSTORE_ZSTD_COMPRESS is not set
# CONFIG_CRYPTO_ZSTD is not set
CONFIG_ZSTD_COMPRESS=y
CONFIG_ZSTD_DECOMPRESS=y
saratoga /usr/src/linux # make -j 81
scripts/kconfig/conf  --syncconfig Kconfig
  DESCEND  objtool
  CALL    scripts/atomic/check-atomics.sh
  CALL    scripts/checksyscalls.sh
  CHK     include/generated/compile.h
  GZIP    kernel/config_data.gz
  CC      kernel/configs.o
Cyclomatic Complexity 1 kernel/configs.c:ikconfig_cleanup
Cyclomatic Complexity 2 kernel/configs.c:ikconfig_init
Cyclomatic Complexity 1 kernel/configs.c:ikconfig_read_current
  AR      kernel/built-in.a
  GEN     .version
  CHK     include/generated/compile.h
  UPD     include/generated/compile.h
  CC      init/version.o
  AR      init/built-in.a
  LD      vmlinux.o
ld: fs/btrfs/zstd.o: in function `zstd_decompress':
/usr/src/linux/fs/btrfs/zstd.c:627: multiple definition of `zstd_decompress'; fs/zfs/zstd/zfs_zstd.o:/usr/src/linux/fs/zfs/zstd/zfs_zstd.c:526: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_buildCTable_wksp':
/usr/src/linux/lib/zstd/fse_compress.c:93: multiple definition of `FSE_buildCTable_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7548: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_NCountWriteBound':
/usr/src/linux/lib/zstd/fse_compress.c:200: multiple definition of `FSE_NCountWriteBound'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7668: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_writeNCount':
/usr/src/linux/lib/zstd/fse_compress.c:303: multiple definition of `FSE_writeNCount'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7769: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_optimalTableLog_internal':
/usr/src/linux/lib/zstd/fse_compress.c:495: multiple definition of `FSE_optimalTableLog_internal'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7806: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_optimalTableLog_internal':
/usr/src/linux/lib/zstd/fse_compress.c:495: multiple definition of `FSE_optimalTableLog'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7819: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_normalizeCount':
/usr/src/linux/lib/zstd/fse_compress.c:609: multiple definition of `FSE_normalizeCount'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7917: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_buildCTable_raw':
/usr/src/linux/lib/zstd/fse_compress.c:667: multiple definition of `FSE_buildCTable_raw'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7978: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_buildCTable_rle':
/usr/src/linux/lib/zstd/fse_compress.c:710: multiple definition of `FSE_buildCTable_rle'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8018: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_compress_usingCTable':
/usr/src/linux/lib/zstd/fse_compress.c:787: multiple definition of `FSE_compress_usingCTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8096: first defined here
ld: lib/zstd/fse_compress.o: in function `FSE_compressBound':
/usr/src/linux/lib/zstd/fse_compress.c:795: multiple definition of `FSE_compressBound'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8105: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_optimalTableLog':
/usr/src/linux/lib/zstd/huf_compress.c:70: multiple definition of `HUF_optimalTableLog'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8413: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_buildCTable_wksp':
/usr/src/linux/lib/zstd/huf_compress.c:421: multiple definition of `HUF_buildCTable_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8703: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compressBound':
/usr/src/linux/lib/zstd/huf_compress.c:526: multiple definition of `HUF_compressBound'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8805: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress1X_usingCTable':
/usr/src/linux/lib/zstd/huf_compress.c:548: multiple definition of `HUF_compress1X_usingCTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8894: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress4X_usingCTable':
/usr/src/linux/lib/zstd/huf_compress.c:592: multiple definition of `HUF_compress4X_usingCTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8929: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress4X_usingCTable':
/usr/src/linux/lib/zstd/huf_compress.c:592: multiple definition of `HUF_compress4X_usingCTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:8929: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress1X_wksp':
/usr/src/linux/lib/zstd/huf_compress.c:750: multiple definition of `HUF_compress1X_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:9096: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress1X_repeat':
/usr/src/linux/lib/zstd/huf_compress.c:757: multiple definition of `HUF_compress1X_repeat'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:9108: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress4X_wksp':
/usr/src/linux/lib/zstd/huf_compress.c:763: multiple definition of `HUF_compress4X_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:9130: first defined here
ld: lib/zstd/huf_compress.o: in function `HUF_compress4X_repeat':
/usr/src/linux/lib/zstd/huf_compress.c:770: multiple definition of `HUF_compress4X_repeat'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:9145: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressContinue':
/usr/src/linux/lib/zstd/compress.c:2541: multiple definition of `ZSTD_compressContinue'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:15881: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBlock_greedy_extDict':
/usr/src/linux/lib/zstd/compress.c:2252: multiple definition of `ZSTD_compressBlock_greedy_extDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:19497: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_seqToCodes':
/usr/src/linux/lib/zstd/compress.c:567: multiple definition of `ZSTD_seqToCodes'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:15014: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_initCStream_usingCDict':
/usr/src/linux/lib/zstd/compress.c:3106: multiple definition of `ZSTD_initCStream_usingCDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16811: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_flushStream':
/usr/src/linux/lib/zstd/compress.c:3239: multiple definition of `ZSTD_flushStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:17175: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compress_usingCDict':
/usr/src/linux/lib/zstd/compress.c:2931: multiple definition of `ZSTD_compress_usingCDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16684: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBegin_usingCDict':
/usr/src/linux/lib/zstd/compress.c:2915: multiple definition of `ZSTD_compressBegin_usingCDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16660: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compress_usingDict':
/usr/src/linux/lib/zstd/compress.c:2827: multiple definition of `ZSTD_compress_usingDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16373: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_resetCStream':
/usr/src/linux/lib/zstd/compress.c:3050: multiple definition of `ZSTD_resetCStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:14050: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_adjustCParams':
/usr/src/linux/lib/zstd/compress.c:182: multiple definition of `ZSTD_adjustCParams'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:14152: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_copyCCtx':
/usr/src/linux/lib/zstd/compress.c:350: multiple definition of `ZSTD_copyCCtx'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:14919: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_freeCStream':
/usr/src/linux/lib/zstd/compress.c:3004: multiple definition of `ZSTD_freeCStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:13233: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressStream':
/usr/src/linux/lib/zstd/compress.c:3224: multiple definition of `ZSTD_compressStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:17054: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBegin':
/usr/src/linux/lib/zstd/compress.c:2760: multiple definition of `ZSTD_compressBegin'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16249: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_freeCDict':
/usr/src/linux/lib/zstd/compress.c:2901: multiple definition of `ZSTD_freeCDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16552: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBegin_advanced':
/usr/src/linux/lib/zstd/compress.c:2748: multiple definition of `ZSTD_compressBegin_advanced'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16229: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_getCParams':
/usr/src/linux/lib/zstd/compress.c:3414: multiple definition of `ZSTD_getCParams'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:17337: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBegin_usingDict':
/usr/src/linux/lib/zstd/compress.c:2755: multiple definition of `ZSTD_compressBegin_usingDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16239: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBound':
/usr/src/linux/lib/zstd/compress.c:38: multiple definition of `ZSTD_compressBound'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:13129: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_createCStream_advanced':
/usr/src/linux/lib/zstd/compress.c:2983: multiple definition of `ZSTD_createCStream_advanced'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16707: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_checkCParams':
/usr/src/linux/lib/zstd/compress.c:155: multiple definition of `ZSTD_checkCParams'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:10272: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_maxCLevel':
/usr/src/linux/lib/zstd/compress.c:3295: multiple definition of `ZSTD_maxCLevel'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:17200: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_getSeqStore':
/usr/src/linux/lib/zstd/compress.c:141: multiple definition of `ZSTD_getSeqStore'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:13274: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_endStream':
/usr/src/linux/lib/zstd/compress.c:3252: multiple definition of `ZSTD_endStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:17182: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBlock':
/usr/src/linux/lib/zstd/compress.c:2547: multiple definition of `ZSTD_compressBlock'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:15893: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_CStreamInSize':
/usr/src/linux/lib/zstd/compress.c:3023: multiple definition of `ZSTD_CStreamInSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16720: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_initCStream':
/usr/src/linux/lib/zstd/compress.c:3093: multiple definition of `ZSTD_initCStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16866: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_freeCCtx':
/usr/src/linux/lib/zstd/compress.c:134: multiple definition of `ZSTD_freeCCtx'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:13233: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_getParams':
/usr/src/linux/lib/zstd/compress.c:3438: multiple definition of `ZSTD_getParams'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:17359: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressBound':
/usr/src/linux/lib/zstd/compress.c:38: multiple definition of `ZSTD_CStreamOutSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16725: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressCCtx':
/usr/src/linux/lib/zstd/compress.c:2832: multiple definition of `ZSTD_compressCCtx'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16388: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_invalidateRepCodes':
/usr/src/linux/lib/zstd/compress.c:341: multiple definition of `ZSTD_invalidateRepCodes'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:14679: first defined here
ld: lib/zstd/compress.o: in function `ZSTD_compressEnd':
/usr/src/linux/lib/zstd/compress.c:2807: multiple definition of `ZSTD_compressEnd'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:16299: first defined here
ld: lib/zstd/entropy_common.o: in function `FSE_versionNumber':
/usr/src/linux/lib/zstd/entropy_common.c:49: multiple definition of `FSE_versionNumber'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:2504: first defined here
ld: lib/zstd/entropy_common.o: in function `ERR_isError':
/usr/src/linux/lib/zstd/error_private.h:44: multiple definition of `FSE_isError'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:810: first defined here
ld: lib/zstd/entropy_common.o: in function `HUF_isError':
/usr/src/linux/lib/zstd/entropy_common.c:52: multiple definition of `HUF_isError'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:2509: first defined here
ld: lib/zstd/entropy_common.o: in function `FSE_readNCount':
/usr/src/linux/lib/zstd/entropy_common.c:60: multiple definition of `FSE_readNCount'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:2520: first defined here
ld: lib/zstd/fse_decompress.o: in function `FSE_buildDTable_rle':
/usr/src/linux/lib/zstd/fse_decompress.c:180: multiple definition of `FSE_buildDTable_rle'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:2895: first defined here
ld: lib/zstd/fse_decompress.o: in function `FSE_buildDTable_raw':
/usr/src/linux/lib/zstd/fse_decompress.c:188: multiple definition of `FSE_buildDTable_raw'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:2904: first defined here
ld: lib/zstd/fse_decompress.o: in function `FSE_decompress_usingDTable':
/usr/src/linux/lib/zstd/fse_decompress.c:283: multiple definition of `FSE_decompress_usingDTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:2995: first defined here
ld: lib/zstd/fse_decompress.o: in function `FSE_decompress_wksp':
/usr/src/linux/lib/zstd/fse_decompress.c:295: multiple definition of `FSE_decompress_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:3007: first defined here
ld: lib/zstd/zstd_common.o: in function `ZSTD_malloc':
/usr/src/linux/lib/zstd/zstd_common.c:69: multiple definition of `ZSTD_malloc'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7374: first defined here
ld: lib/zstd/zstd_common.o: in function `ZSTD_free':
/usr/src/linux/lib/zstd/zstd_common.c:73: multiple definition of `ZSTD_free'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:7395: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_readDTableX2_wksp':
/usr/src/linux/lib/zstd/huf_decompress.c:91: multiple definition of `HUF_readDTableX2_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:21903: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress1X2_usingDTable':
/usr/src/linux/lib/zstd/huf_decompress.c:227: multiple definition of `HUF_decompress1X2_usingDTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22222: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress1X2_DCtx_wksp':
/usr/src/linux/lib/zstd/huf_decompress.c:233: multiple definition of `HUF_decompress1X2_DCtx_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22229: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress4X2_usingDTable':
/usr/src/linux/lib/zstd/huf_decompress.c:358: multiple definition of `HUF_decompress4X2_usingDTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22262: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress4X2_DCtx_wksp':
/usr/src/linux/lib/zstd/huf_decompress.c:364: multiple definition of `HUF_decompress4X2_DCtx_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22284: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress1X_usingDTable':
/usr/src/linux/lib/zstd/huf_decompress.c:848: multiple definition of `HUF_decompress1X_usingDTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22324: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress4X_usingDTable':
/usr/src/linux/lib/zstd/huf_decompress.c:855: multiple definition of `HUF_decompress4X_usingDTable'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22343: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_selectDecoder':
/usr/src/linux/lib/zstd/huf_decompress.c:890: multiple definition of `HUF_selectDecoder'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22392: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress4X_hufOnly_wksp':
/usr/src/linux/lib/zstd/huf_decompress.c:927: multiple definition of `HUF_decompress4X_hufOnly_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22470: first defined here
ld: lib/zstd/huf_decompress.o: in function `HUF_decompress1X_DCtx_wksp':
/usr/src/linux/lib/zstd/huf_decompress.c:942: multiple definition of `HUF_decompress1X_DCtx_wksp'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:22495: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompressBlock':
/usr/src/linux/lib/zstd/decompress.c:1480: multiple definition of `ZSTD_decompressBlock'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:27819: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompressBegin_usingDict':
/usr/src/linux/lib/zstd/decompress.c:1970: multiple definition of `ZSTD_decompressBegin_usingDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25688: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_nextInputType':
/usr/src/linux/lib/zstd/decompress.c:1725: multiple definition of `ZSTD_nextInputType'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25367: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_DStreamOutSize':
/usr/src/linux/lib/zstd/decompress.c:2278: multiple definition of `ZSTD_DStreamOutSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25795: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_resetDStream':
/usr/src/linux/lib/zstd/decompress.c:2282: multiple definition of `ZSTD_resetDStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24597: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_getDictID_fromDict':
/usr/src/linux/lib/zstd/decompress.c:2108: multiple definition of `ZSTD_getDictID_fromDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25725: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompress_usingDDict':
/usr/src/linux/lib/zstd/decompress.c:2150: multiple definition of `ZSTD_decompress_usingDDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25761: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_findFrameCompressedSize':
/usr/src/linux/lib/zstd/decompress.c:1511: multiple definition of `ZSTD_findFrameCompressedSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25039: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_createDCtx_advanced':
/usr/src/linux/lib/zstd/decompress.c:130: multiple definition of `ZSTD_createDCtx_advanced'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24642: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_getcBlockSize':
/usr/src/linux/lib/zstd/decompress.c:396: multiple definition of `ZSTD_getcBlockSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:26452: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_freeDStream':
/usr/src/linux/lib/zstd/decompress.c:2258: multiple definition of `ZSTD_freeDStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25788: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_freeDCtx':
/usr/src/linux/lib/zstd/decompress.c:149: multiple definition of `ZSTD_freeDCtx'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24668: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_nextSrcSizeToDecompress':
/usr/src/linux/lib/zstd/decompress.c:1721: multiple definition of `ZSTD_nextSrcSizeToDecompress'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25346: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_findDecompressedSize':
/usr/src/linux/lib/zstd/decompress.c:320: multiple definition of `ZSTD_findDecompressedSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24884: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_isFrame':
/usr/src/linux/lib/zstd/decompress.c:175: multiple definition of `ZSTD_isFrame'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24703: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decodeSeqHeaders':
/usr/src/linux/lib/zstd/decompress.c:795: multiple definition of `ZSTD_decodeSeqHeaders'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:26883: first defined here
ld: lib/zstd/decompress.o: in function `memcpy':
/usr/src/linux/./include/linux/string.h:406: multiple definition of `ZSTD_copyDCtx'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/./include/linux/string.h:406: first defined here
ld: lib/zstd/decompress.o: in function `memcpy':
/usr/src/linux/./include/linux/string.h:406: multiple definition of `ZSTD_decompressBegin'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24597: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompressStream':
/usr/src/linux/lib/zstd/decompress.c:2299: multiple definition of `ZSTD_decompressStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:26095: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompressContinue':
/usr/src/linux/lib/zstd/decompress.c:1744: multiple definition of `ZSTD_decompressContinue'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25395: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_initDStream':
/usr/src/linux/lib/zstd/decompress.c:2215: multiple definition of `ZSTD_initDStream'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25850: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompress_usingDict':
/usr/src/linux/lib/zstd/decompress.c:1709: multiple definition of `ZSTD_decompressDCtx'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25320: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_getDictID_fromFrame':
/usr/src/linux/lib/zstd/decompress.c:2136: multiple definition of `ZSTD_getDictID_fromFrame'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24756: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decompress_usingDict':
/usr/src/linux/lib/zstd/decompress.c:1709: multiple definition of `ZSTD_decompress_usingDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25298: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_getDictID_fromDDict':
/usr/src/linux/lib/zstd/decompress.c:2121: multiple definition of `ZSTD_getDictID_fromDDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24442: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_insertBlock':
/usr/src/linux/lib/zstd/decompress.c:1491: multiple definition of `ZSTD_insertBlock'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25076: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_getFrameParams':
/usr/src/linux/lib/zstd/decompress.c:211: multiple definition of `ZSTD_getFrameContentSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24844: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_decodeLiteralsBlock':
/usr/src/linux/lib/zstd/decompress.c:434: multiple definition of `ZSTD_decodeLiteralsBlock'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:26474: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_freeDDict':
/usr/src/linux/lib/zstd/decompress.c:2091: multiple definition of `ZSTD_freeDDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:24414: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_initDStream_usingDDict':
/usr/src/linux/lib/zstd/decompress.c:2248: multiple definition of `ZSTD_initDStream_usingDDict'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25859: first defined here
ld: lib/zstd/decompress.o: in function `ZSTD_DStreamInSize':
/usr/src/linux/lib/zstd/decompress.c:2277: multiple definition of `ZSTD_DStreamInSize'; fs/zfs/zstd/lib/zstd.o:/usr/src/linux/fs/zfs/zstd/lib/zstd.c:25795: first defined here
make: *** [Makefile:1139: vmlinux] Error 1

I'm building against ZFS HEAD (commit 64025fa) and suspect this is related to the merge yesterday of #10278 (comment)

@Ornias1993
Copy link
Contributor

Ornias1993 commented Aug 21, 2020

Thanks for the headsup!
None of our testing systems had these issues...

@c0d3z3r0 and @BrainSlayer
You guys might want to provide a fix...

Edit
I was wrong we did test 5.8, thanks @BrainSlayer

@BrainSlayer
Copy link
Contributor

mine is running 5.8 with current master git including zstd of course. all working

@BrainSlayer
Copy link
Contributor

server2:~ # uname -a Linux server2 5.8.2-666.ddwrt #1 SMP Fri Aug 21 13:02:10 +03 2020 x86_64 x86_64 x86_64 GNU/Linux server2:~ # zfs get all |grep compres zfs compressratio 2.04x - zfs compression zstd local zfs refcompressratio 2.04x - server2:~ #

@BrainSlayer
Copy link
Contributor

but he is doing inline kernel compiling. i'm compiling out of tree

@BrainSlayer
Copy link
Contributor

BrainSlayer commented Aug 21, 2020

ah the cause is easy. the problem is that he is compiling with zstd in kernel. this collides with symbols from zfs zstd implementation. this is unresolvable unless we rename all zstd functions in our implementation. blocking point

@Ornias1993
Copy link
Contributor

@BrainSlayer I'm not that well versed into this, but can't we exclude the ZSTD included in the kernel on build or is it automatically included for everything without giving us the option not to?

@BrainSlayer
Copy link
Contributor

BrainSlayer commented Aug 21, 2020

no we cannot. the in kernel zstd implementation is feature reduced and out of date

@nivedita76
Copy link
Contributor Author

nivedita76 commented Aug 21, 2020 via email

@Ornias1993
Copy link
Contributor

@nivedita76
We can't rely on the version in the kernel, because it might mismatch the version that originally compressed your data. Simply put: If you don't want you data be corrupted you don't want to rely on the kernel ZSTD.

TLDR: No.

@BrainSlayer
Copy link
Contributor

Why did we need our own implementation? ZSTD has been in the kernel for a while now I think. Can’t we just use it?

________________________________ From: Sebastian Gottschall notifications@github.com Sent: Friday, August 21, 2020 11:23:16 AM To: openzfs/zfs zfs@noreply.github.com Cc: Arvind Sankar nivedita@alum.mit.edu; Author author@noreply.github.com Subject: Re: [openzfs/zfs] ZSTD clash with linux 5.8.x (#10763) ah the cause is easy. the problem is that he is compiling with zstd in kernel support. this collides with symbols from zfs zstd implementation. this is unresolvable unless we rename all zstd functions in our implementation. blocking point — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#10763 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AJBU6WIW2CQXKWFNHZ4KYMDSB2GOJANCNFSM4QG654KA.

as i wrote in last comment. the in kernel zstd lib is out of date and feature reduced. it will not work. but you can compile zfs out of tree as workaround until we agree how we fix that problem. we have only a symbol collision since you compiled it static

@Ornias1993
Copy link
Contributor

@BrainSlayer Any idea how many Distro's would be hit with this issue out-of-the-box?

@BrainSlayer
Copy link
Contributor

100%. The in kernel zstd lib is included since long time. so if you compile zfs static into the kernel and in kernel zstd is enabled, all supported kernels are affected. compiling out of tree is no issue

@BrainSlayer
Copy link
Contributor

solution writing a wrapper header with some #define macros which rename all zstd functions for zfs

@Ornias1993
Copy link
Contributor

@BrainSlayer I meant like: How many kernels compile zfs static into the kernel AND have the in-kernel zstd both enabled?

@Ornias1993
Copy link
Contributor

solution writing a wrapper header with some #define macros which rename all zstd functions for zfs

We could indeed prefix everything with ZFS_, the priority for doing such a thing depends a bit how many distro's would be affected out-of-the-box by compiling zfs static into the kernel AND having the in-kernel zstd both enabled...

@nivedita76
Copy link
Contributor Author

@BrainSlayer what would happen currently if someone loads zfs module and then loads some other kernel module that uses in-kernel ZSTD?

For renaming, it's possible to do it using objcopy --prefix-symbols=zfs_ (for eg) to avoid having to list out all the symbols. An example in the kernel is drivers/firmware/efi/libstub/Makefile, which renames all symbols for the EFI stub on ARM64.

@Ornias1993
Copy link
Contributor

Ornias1993 commented Aug 21, 2020

@nivedita76
Could you rename the title to, please:
In-tree ZFS-ZSTD clash with in-kernel ZSTD

For renaming, it's possible to do it using objcopy --prefix-symbols=zfs_ (for eg) to avoid having to list out all the symbols

I'm okey with such a solution personally.
@allanjude and @c0d3z3r0 ?

@nivedita76
Copy link
Contributor Author

Regarding distros, the upstream kernel now supports initramfs compressed using ZSTD. Not sure how many distros have backported that, but ZSTD compiled into the kernel might be fairly common.

@nivedita76 nivedita76 changed the title ZSTD clash with linux 5.8.x In-tree ZFS-ZSTD clash with in-kernel ZSTD Aug 21, 2020
@Ornias1993
Copy link
Contributor

Ornias1993 commented Aug 21, 2020

@nivedita76 Thats what @BrainSlayer told me previously, but thats not the issue.
The issue is having ZSTD "baked in" AND building ZFS in-tree.
(the combination of both)

But yes, if building ZFS in-tree all kernel versions including ZSTD in-kernel would be affected.

@nivedita76
Copy link
Contributor Author

@nivedita76 Thats what @BrainSlayer told me previously, but thats not the issue.
The issue is having ZSTD "baked in" AND building ZFS in-tree.
(the combination of both)

Are we sure this doesn't cause runtime issues? The ZFS ZSTD implementation is loaded as a module, right? How can that override ZSTD symbols that are already built into the kernel?

@nivedita76
Copy link
Contributor Author

What I mean is, are we sure, zfs.ko is using the symbols from zzstd.ko, and not the ones built into the kernel?

@Ornias1993
Copy link
Contributor

@nivedita76
I think we also tested it in Kernel versions without ZSTD...

But I think just prefixing our version is a nice and clean solution.

@BrainSlayer
Copy link
Contributor

@Ornias1993 i agree. i will prepare something as soon as i find some time. there are alot of functions to consider

@BrainSlayer
Copy link
Contributor

@nivedita76 zzstd.ko is using all functions inline. there is no reference to zstd from zfs.ko etc. so all references to zstd itself are in zzstd.ko itself. so there is no collision if you just compile it out of tree and not static into the kernel

@Ornias1993
Copy link
Contributor

@Ornias1993 i agree. i will prepare something as soon as i find some time. there are alot of functions to consider

Would the solution from @nivedita76 work out for that purpose?
objcopy --prefix-symbols=zfs_

@nivedita76
Copy link
Contributor Author

@nivedita76
I think we also tested it in Kernel versions without ZSTD...

But that doesn't address my concern, which is on kernel versions with ZSTD. Or really, trying to import pools created with kernel w/o ZSTD, which definitely used in-tree ZSTD, on a kernel with ZSTD, which might be using the in-kernel ZSTD.

@nivedita76
Copy link
Contributor Author

@nivedita76 zzstd.ko is using all functions inline. there is no reference to zstd from zfs.ko etc. so all references to zstd itself are in zzstd.ko itself. so there is no collision if you just compile it out of tree and not static into the kernel

Oh, not even zstd_compress/zstd_decompress collide?

@Ornias1993
Copy link
Contributor

Ornias1993 commented Aug 21, 2020

@nivedita76 If it did, we would've the same issue as we have here, wouldn't we? ;)
Because if that was the case, It would also error out because it would've two options for zstd_compress/zstd_decompress...

@nivedita76
Copy link
Contributor Author

nivedita76 commented Aug 28, 2020 via email

@Ornias1993
Copy link
Contributor

@nivedita76 The current ZFS version binds every ZFS version to a specific ZSTD version. Which would be a hell to undo.

You are also underestimating: It wouldn't cut 0.5mb from the kernel, we still would have to supply a ZSTD library for platforms (or kernel versions) that won't have ZSTD integrated. To make it more complicated, previously (about 3 years or so back), there where problems with zstd versions not working correctly when used with ZFS. Ohh and zfs also supports kernels without ZSTD buildin afaik.

So All things considered: We can't remove the ZSTD lib completely. We might with hard rewriting work make it an option to use a kernel zstd version, but no one is going to do the hell of work required to do so and actually keep supporting it. And for what? Because that would mean a few people (its a small minority thats doing what you are doing and a gpl violation to distribute a kernel like that) could compile it to save 0.5Mb?

All these design discussions have been gone over and over, for 3-4 years by now. There is a time and place for every discussion. Imho, this isn't it anymore. I'm not saying your opinion is irrelevant, but it isn't going to change anything.

Stability was always key for ZSTD-on-ZFS and supplying a trusted ZSTD source (which got tested into oblivion) is part of that stability guarantee.

@terrelln
Copy link
Contributor

While it's awesome you provide input on how we could this, ZSTD-on-ZFS has left the design stage by now and with the current design there isn't any benefid on using in-kernel zstd....

No worries, was just letting you know the option is now available :)

@BrainSlayer
Copy link
Contributor

BrainSlayer commented Aug 29, 2020

On Fri, Aug 28, 2020 at 11:22:21AM -0700, Kjeld Schouten-Lebbing wrote: @terrelln ZSTD has already been implemented, there are no plans for using in-kernel zstd, talks about this have been concluded for months by now... Anyhow: In essence this isn't really something about the ZSTD version, its about ZFS. ZSTD does checksumming, even compatible versions of ZSTD might result in slightly different checksums and we need to account for it. ZSTD also needs to support a lot of different platforms, both Linux and FreeBSD based. Having to rely on varying versions of ZSTD in the kernel overcomplicates things to the n'th degree. The current implementation of zstd on zfs is quite solid, vetted just fine and this bug was fixed already without much issue. While it's awesome you provide input on how we could this, ZSTD-on-ZFS has left the design stage by now and with the current design there isn't any benefid on using in-kernel zstd.... -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #10763 (comment)
There is at least one benefit: getting rid of duplication. zstd adds about 0.5Mb to the ZFS kernel build. Thanks.

you know whats a big disadvantage? the in kernel zstd variant is slower and since its missing the custom allocator feature its unuseable and will not work. we wrote a special custom allocator which increased the overall performance multiple times. without it, it will not pass the zfs tests, since they will all timeout for performance reasons. so there was never a option to use the in kernel variant.

@Ornias1993
Copy link
Contributor

@BrainSlayer thanks for pointing out the main argument I was too stupid to fully grasp/explain! 👍

@BrainSlayer
Copy link
Contributor

@Ornias1993 dont blame yourself. i'm more wondering why people requesting such things without doing a little bit deeper research what they are talking about. if the kernel one time in future provides all features it might be a option for future versions, but even then only a benchmark will show if its worth to follow up on this

@Ornias1993
Copy link
Contributor

@Ornias1993 dont blame yourself. i'm more wondering why people requesting such things without doing a little bit deeper research what they are talking about. if the kernel one time in future provides all features it might be a option for future versions, but even then only a benchmark will show if its worth to follow up on this

Good point, but even in that case the whole host of other issues still need to designed around... Not saying it will never happen, but not anytime soon(tm) ;)

@terrelln
Copy link
Contributor

Sorry, I wasn't intending to come here and say that OpenZFS for Linux should have used the version of zstd in the kernel. Definitely, given the state of zstd in the kernel, bundling upstream zstd is the better choice. Not to mention the complexity of using two incompatible versions of zstd, one for Linux and one for BSDs.

I wrote the version in the kernel before upstream zstd was ready to be used as-is, and was too new to the project to know all the changes necessary to get it there. Now, as you've proven with OpenZFS, zstd is ready to be used as-is with either ZSTD_customMem or ZSTD_initStatic*().

you know whats a big disadvantage? the in kernel zstd variant is slower and since its missing the custom allocator feature its unuseable and will not work. we wrote a special custom allocator which increased the overall performance multiple times. without it, it will not pass the zfs tests, since they will all timeout for performance reasons. so there was never a option to use the in kernel variant.

Just curious, what are you comparing against? Are you comparing against allocating a context for every (de)compression call? I would certainly believe that is several times slower. The custom allocator approach seems to be similar to the approach that btrfs has taken of caching workspaces, for example, and I would expect similar performance.

Looking at the code in zfs_zstd.c, you may be able to gain a small amount of compression performance by caching the ZSTD_CCtx objects. When reusing a ZSTD_CCtx it doesn't need to zero its tables, saving a ~100KB memset at level 3. This does introduce the complexity of caching the ZSTD_CCtx objects, so maybe you've already considered this and ruled it out due to the architectural complexity.

When I update zstd in the kernel I will be updating btrfs to reuse contexts instead of just workspace memory. At that point I will carefully measure the speed difference, but I would estimate a gain of no more than 10% at low levels, probably more like 3-5%.

if the kernel one time in future provides all features it might be a option for future versions, but even then only a benchmark will show if its worth to follow up on this

I'm working on porting zstd-1.4.6 (next version) as-is in the kernel, and making it trivial to keep up to date. It will be fully featured and be using upstream directly, like OpenZFS. So it should have identical performance. There would be no good reason to switch, given bundling zstd already works, and probably some extra complexity, but the option will be there if needed.

If there is anything that the OpenZFS project needs/wants from zstd please let me know / open an issue.

@BrainSlayer
Copy link
Contributor

BrainSlayer commented Aug 30, 2020

Sorry, I wasn't intending to come here and say that OpenZFS for Linux should have used the version of zstd in the kernel. Definitely, given the state of zstd in the kernel, bundling upstream zstd is the better choice. Not to mention the complexity of using two incompatible versions of zstd, one for Linux and one for BSDs.

I wrote the version in the kernel before upstream zstd was ready to be used as-is, and was too new to the project to know all the changes necessary to get it there. Now, as you've proven with OpenZFS, zstd is ready to be used as-is with either ZSTD_customMem or ZSTD_initStatic*().

you know whats a big disadvantage? the in kernel zstd variant is slower and since its missing the custom allocator feature its unuseable and will not work. we wrote a special custom allocator which increased the overall performance multiple times. without it, it will not pass the zfs tests, since they will all timeout for performance reasons. so there was never a option to use the in kernel variant.

Just curious, what are you comparing against? Are you comparing against allocating a context for every (de)compression call? I would certainly believe that is several times slower. The custom allocator approach seems to be similar to the approach that btrfs has taken of caching workspaces, for example, and I would expect similar performance.

Looking at the code in zfs_zstd.c, you may be able to gain a small amount of compression performance by caching the ZSTD_CCtx objects. When reusing a ZSTD_CCtx it doesn't need to zero its tables, saving a ~100KB memset at level 3. This does introduce the complexity of caching the ZSTD_CCtx objects, so maybe you've already considered this and ruled it out due to the architectural complexity.

When I update zstd in the kernel I will be updating btrfs to reuse contexts instead of just workspace memory. At that point I will carefully measure the speed difference, but I would estimate a gain of no more than 10% at low levels, probably more like 3-5%.

if the kernel one time in future provides all features it might be a option for future versions, but even then only a benchmark will show if its worth to follow up on this

I'm working on porting zstd-1.4.6 (next version) as-is in the kernel, and making it trivial to keep up to date. It will be fully featured and be using upstream directly, like OpenZFS. So it should have identical performance. There would be no good reason to switch, given bundling zstd already works, and probably some extra complexity, but the option will be there if needed.

If there is anything that the OpenZFS project needs/wants from zstd please let me know / open an issue.

no we dont allocate a new context on decompression and compression. the allocator is creating a multithread safe memory allocation pool which releases objects by itself if not beeing used and does cache allocations in a own way. decompression isnt a big issue since the context for decompression is small, but we cache it too for performance reasons. the compression context is the issue since it can be very big. with high compression ratio (19) its about 80 megs per thread and zfs works multithreaded. on a standard 8 core system 32 threads are running at the same time. this is a problem for allocation (allocating big blocks is just slow even with standard compression settings). the slab cache doesnt help either. 1st the maximum allocatable memory is limited in the zfs implementation. and if we unlock that limit its still slower than our own memory pool approach. this memory pool is also a very specialized implementation just made for zstd and shows the best performance of all solutions. the approach is basicly resusing context whenever possible and avoid allocations at standard run time. so yeah. standard allocation pool approach, just faster than any standard solution we tested. but anyway. as soon as you finished your upstream work in the kernel i will review it and will also do a performance comparisation. consider that our zstd implementation is staticly compiled as whole combine sourcecode with with explicit -O3 like the original library is doing. this allows the compiler to optimize the code much better than any modular source variant. compiling a full featured zstd into the kernel is also not possible since some function of zstd will have a stack usage of > 20kb. this is not kernel code compatible. we dont use these functions in our code, but this needs to be considered by you

@Ornias1993
Copy link
Contributor

@BrainSlayer good note on the stack size, it was one of the few aspects of zstd we didnt really like (even though we dont use said codepaths)

@terrelln do you want an issue for that on the zstd project?

@BrainSlayer
Copy link
Contributor

@Ornias1993 the stack issue can be fixed, but will decrease the performance usually. but maybe there is also a better solution for this hufman table macro hell on local stack

@Ornias1993
Copy link
Contributor

but maybe there is also a better solution for this hufman table macro hell on local stack

Thats why i'm asking if he wants an issue for it ;)

@terrelln
Copy link
Contributor

no we don't allocate a new context on decompression and compression.

I know that you reuse the context memory in your allocator. I mean the creation of the zstd context itself on this line

cctx = ZSTD_createCCtx_advanced(zstd_malloc);

If you cache the ZSTD_CCtx object itself, and reuse it, you avoid a ~100KB memset() at level 3 inside of zstd on subsequent uses of the ZSTD_CCtx with the same parameters. This is a much smaller deal than the allocation, because as you said allocation is costly. But, I suspect it would show up in compression performance, and cost probably around 3-5% of compression CPU. I realized I'm assuming 128KB blocks with that performance assessment, but I'm not sure that is accurate for ZFS. The larger the block size the less this will matter.

consider that our zstd implementation is staticly compiled as whole combine sourcecode with with explicit -O3 like the original library is doing. this allows the compiler to optimize the code much better than any modular source variant

The Linux Kernel also compiles zstd with -O3. I guess I don't quite know what you are saying here. Zstd doesn't have any cross translation-unit function calls in any of its hot loops. In fact zstd doesn't have any hot function calls in any of its hot loops, everything should be inlined (or it is cold). So there shouldn't be any benefit from combining multiple .c files into one, or from LTO.

compiling a full featured zstd into the kernel is also not possible since some function of zstd will have a stack usage of > 20kb. this is not kernel code compatible.

These functions aren't needed by zstd. There are a bunch of functions in huf_compress.c, huf_decompress.c, fse_compress.c, and fse_decompress.c that aren't used by zstd at all. They are present because those files come from the FiniteStateEntropy project. But this is a problem for the kernel because this will raise build warnings, even though the functions are unused. So in facebook/zstd#2289 I am adding ZSTD_NO_UNUSED_FUNCTIONS which hides these functions. So you'll be able to define that in the next zstd version to hide these functions. After that macro is defined zstd has no functions that use more than 2KB of stack space.

In that PR I have also added a test that measures the actual stack high water mark for zstd (in the existing kernel use cases). After I reduced the compression stack usage by 1KB in that PR, I measured that compression needs ~2KB of stack space and decompression needs ~1KB of stack space. This could be reduced further with some further work.

@Ornias1993 if you want to open an issue that would be great. It would be a good place to make sure these answers don't get lost, and a reminder to use the new build macros when we release the next zstd version. I would also like to reduce zstd's actual stack usage further.

@terrelln
Copy link
Contributor

@Ornias1993 the stack issue can be fixed, but will decrease the performance usually. but maybe there is also a better solution for this hufman table macro hell on local stack

To be clear these are the functions that are unused by zstd entirely. We use "workspace" variants of these functions to avoid the large stack usage. We allocate some space in the ZSTD_CCtx and ZSTD_DCtx once and use it as scratch space, instead of the stack.

@BrainSlayer
Copy link
Contributor

i will check if reusing the whole context object will work. normally the context keeps track of the current compression flow/state, so its curious for me, that it works without clearing it on reuse

@Ornias1993
Copy link
Contributor

@BrainSlayer If it works out, give me a call and i'll do the performance tests again too :)
3-5% might be pretty interesting :)

@terrelln Awesome change ZSTD_NO_UNUSED_FUNCTIONS is certainly something we wanna use for ZFS too 👍

@BrainSlayer
Copy link
Contributor

@Ornias1993 if i do it i will just reimplement the init functions to keep the mempool more generic. but i dont think that it works without clearing the context

@allanjude
Copy link
Contributor

i will check if reusing the whole context object will work. normally the context keeps track of the current compression flow/state, so its curious for me, that it works without clearing it on reuse

If I recall correctly, there is a ZSTD API for 'resetting' a CCtx to be able to reuse it. It only has to reset a few fields to make it safe, vs having to setup the entire CCtx.

It was one of the optimizations I was going to look at once I get the rest of the boot loader bits settled etc.

@terrelln
Copy link
Contributor

i will check if reusing the whole context object will work. normally the context keeps track of the current compression flow/state, so its curious for me, that it works without clearing it on reuse

For compression, we use tables of indices into the compressed data to find matches. These tables start with the value of 0. After we compress data of say 5000 bytes, the maximum value is 4999. So the next compression call can "invalidate" entries below 5000 instead of re-zeroing the tables. Context reuse produces byte-identical outputs to single-use contexts, it is just an optimization.

@Ornias1993
Copy link
Contributor

once I get the rest of the boot loader bits settled etc.

Its a bit offtopic on the offtopic, but how is the bootloader work going actually? :)

@BrainSlayer
Copy link
Contributor

@Ornias1993 bootloader? i know that freebsd uses a own bootloader. the grub patch i did works

@Ornias1993

This comment has been minimized.

@BrainSlayer
Copy link
Contributor

@Ornias1993 because you referenced a project which is new to me

@mschilli87

This comment has been minimized.

@BrainSlayer
Copy link
Contributor

@mschilli87 alan was talking about a bootloader. i dont see that this is related to any optimization

@mschilli87

This comment has been minimized.

@Ornias1993

This comment has been minimized.

@Ornias1993
Copy link
Contributor

@BrainSlayer SLight necro:
Did you look into reusing the context yet?

@BrainSlayer
Copy link
Contributor

@Ornias1993 thats what the mempool is doing at the end. i have not found any code which causes performance impact in the current way we are doing. we are already reusing allocated objects. we do not clear the memory on reuse. there is no memset done

@Ornias1993
Copy link
Contributor

@BrainSlayer Thanks for the clearity, I was making a shortlist in my head for ZSTD todo's, good to know this is definately considered to be solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants