Zfs: In-tree ZFS-ZSTD clash with in-kernel ZSTD

Created on 21 Aug 2020  ·  78Comments  ·  Source: openzfs/zfs

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

picture nivedita76

Most helpful comment

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.

picture allanjude on 31 Aug 2020
👍3

All 78 comments

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 64025fa3a1f0f710f7f8678f2ac459b07ed9f88f) and suspect this is related to the merge yesterday of https://github.com/openzfs/zfs/pull/10278#issuecomment-677809493

thehaven picture thehaven on 21 Aug 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

Ornias1993 picture Ornias1993 on 21 Aug 2020

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

BrainSlayer picture BrainSlayer on 21 Aug 2020

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 picture BrainSlayer on 21 Aug 2020

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

BrainSlayer picture BrainSlayer on 21 Aug 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

BrainSlayer picture BrainSlayer on 21 Aug 2020

@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?

Ornias1993 picture Ornias1993 on 21 Aug 2020

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

BrainSlayer picture BrainSlayer on 21 Aug 2020

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 GitHubhttps://github.com/openzfs/zfs/issues/10763#issuecomment-678348711, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AJBU6WIW2CQXKWFNHZ4KYMDSB2GOJANCNFSM4QG654KA.

nivedita76 picture nivedita76 on 21 Aug 2020

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

Ornias1993 picture Ornias1993 on 21 Aug 2020

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

BrainSlayer picture BrainSlayer on 21 Aug 2020

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

Ornias1993 picture Ornias1993 on 21 Aug 2020

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 picture BrainSlayer on 21 Aug 2020

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

BrainSlayer picture BrainSlayer on 21 Aug 2020

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

Ornias1993 picture Ornias1993 on 21 Aug 2020

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

Ornias1993 picture Ornias1993 on 21 Aug 2020

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

nivedita76 picture nivedita76 on 21 Aug 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 ?

Ornias1993 picture Ornias1993 on 21 Aug 2020

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 picture nivedita76 on 21 Aug 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.

Ornias1993 picture Ornias1993 on 21 Aug 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)

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 picture nivedita76 on 21 Aug 2020

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

nivedita76 picture nivedita76 on 21 Aug 2020

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

Ornias1993 picture Ornias1993 on 21 Aug 2020

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

BrainSlayer picture BrainSlayer on 21 Aug 2020
👍2

@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

BrainSlayer picture BrainSlayer on 21 Aug 2020

@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_

Ornias1993 picture Ornias1993 on 21 Aug 2020

@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 picture nivedita76 on 21 Aug 2020

@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?

nivedita76 picture nivedita76 on 21 Aug 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...

Ornias1993 picture Ornias1993 on 21 Aug 2020

Ok, I wasn't sure if it errored on symbol clashes in modules vs kernel.

nivedita76 picture nivedita76 on 21 Aug 2020

@nivedita76 I'm not 100% sure, but if we fix this bug that would be fixed anyway if thats the case. It should at least be fixed on OpenZFS2.0 RELEASE :)

Ornias1993 picture Ornias1993 on 21 Aug 2020

please just let me sleep this night. tomorrow we have a solution. its easy to fix and no there is no risc of using wrong symbols if its compiled out of tree as module

BrainSlayer picture BrainSlayer on 21 Aug 2020
👍1

lol @BrainSlayer sweet dreams :)

nivedita76 picture nivedita76 on 21 Aug 2020
😄2

@Ornias1993 one interesting point. isnt static linking non gpl code like zfs to the kernel a gpl license violation?

BrainSlayer picture BrainSlayer on 22 Aug 2020

@nivedita76 please try this commit/patch on top of master and give us some feedback if this solves your problem https://github.com/BrainSlayer/zfs/commit/2fee2750a68ed7826d17554181971c0ee6cb703b

BrainSlayer picture BrainSlayer on 22 Aug 2020

@BrainSlayer
As long as you don't distribute or publish, thats not a GPL violation according to article 2 section b of the GPLv2.

Funny enough the GPL_ONLY tags are a GPL violation themselves, in accordance to article 6 of the GPLv2 and the general explaination of which in the Preamble which, in contrast to what a lot of people say, is a legally accepted source to explain articles of a licence or agreement in (at least) Europe.

TLDR:

  1. By static linking it yourself, you're not in violation
  2. The Linux kernel itself is the biggest GPL violator worldwide.
Ornias1993 picture Ornias1993 on 22 Aug 2020

@BrainSlayer Didnt the patch suggested by @nivedita76 work out?

Ornias1993 picture Ornias1993 on 22 Aug 2020

@Ornias1993 havent seen a patch from him. i made my own (see my commit link to my private tree). just prefixing the symbols isnt enough since you also need to prefix the references to it which cannot easily done by objcopy, except with a symbol list. so my solution is more clean (usually)

regarding the linux kernel, i dont understand why you say that the kernel is a biggest violator by itself. explain

BrainSlayer picture BrainSlayer on 22 Aug 2020

@BrainSlayer Ohh okey, I was refering to his patch suggestion. Good point indeed.

I explained why Linux is the biggest violator already, second part of my last post.

Ornias1993 picture Ornias1993 on 22 Aug 2020

@BrainSlayer Could you file a draft-PR for your patch, just so we can get those tests going?

Ornias1993 picture Ornias1993 on 22 Aug 2020

@Ornias1993 i want to get some feedback from @nivedita76 first to make sure. i compiled it locally already with no issue

BrainSlayer picture BrainSlayer on 22 Aug 2020

and we need to ask @behlendorf to change the testsystem to include this scenario.

BrainSlayer picture BrainSlayer on 22 Aug 2020

@brainslayer I know, thats not why i'm asking... I was asking to make sure it doesn't crap out on the existing tests.

Ornias1993 picture Ornias1993 on 22 Aug 2020

@Ornias1993 me and git a never ending story. gimme some minutes

BrainSlayer picture BrainSlayer on 22 Aug 2020
👍1

@BrainSlayer No worries, I think we are all used to that one by now :)
Draft is the little dropdown button right of the big green submit button btw ;)

Ornias1993 picture Ornias1993 on 22 Aug 2020

its about branching. my git tree has also another fix for my own system (backward compatiblity with dev versions). i already branched it now
https://github.com/openzfs/zfs/pull/10775

BrainSlayer picture BrainSlayer on 22 Aug 2020

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

FYI I am in the process of updating it to use the latest upstream directly. The only feature not supported is calls to malloc() and free() (without ZSTD_customMem, e.g. ZSTD_createCCtx()), because there isn't a great default, and it needs to work pre-boot. But, providing a ZSTD_customMem will work just fine.

Let me know if you would be interested in using the zstd in the kernel once it has been upgraded. You can reach out via email at ${github-username}@fb.com, or I'll check back on this thread.

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.

The zstd format is stable and zstd versions are both forward and backward compatible. The version in the kernel can understand data produced by the latest version, and visa-versa. Using a different version is not a problem.

terrelln picture terrelln on 28 Aug 2020

@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.
ZFS does checksumming, even compatible versions of ZSTD might result in slightly different checksums and we need to account for it. ZFS 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....

Ornias1993 picture Ornias1993 on 28 Aug 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:
https://github.com/openzfs/zfs/issues/10763#issuecomment-683037238

There is at least one benefit: getting rid of duplication. zstd adds
about 0.5Mb to the ZFS kernel build.

Thanks.

nivedita76 picture nivedita76 on 28 Aug 2020

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

Ornias1993 picture Ornias1993 on 28 Aug 2020

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 :)

terrelln picture terrelln on 28 Aug 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.

BrainSlayer picture BrainSlayer on 29 Aug 2020

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

Ornias1993 picture Ornias1993 on 29 Aug 2020

@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

BrainSlayer picture BrainSlayer on 29 Aug 2020

@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) ;)

Ornias1993 picture Ornias1993 on 29 Aug 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.

terrelln picture terrelln on 30 Aug 2020
❤1 👍1

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

BrainSlayer picture BrainSlayer on 30 Aug 2020

@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?

Ornias1993 picture Ornias1993 on 30 Aug 2020

@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

BrainSlayer picture BrainSlayer on 30 Aug 2020

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 ;)

Ornias1993 picture Ornias1993 on 30 Aug 2020

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 https://github.com/openzfs/zfs/blob/abe4fbfd01c2440813c9f31ca6727473e22d0039/module/zstd/zfs_zstd.c#L365
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 https://github.com/facebook/zstd/pull/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 picture terrelln on 31 Aug 2020

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

terrelln picture terrelln on 31 Aug 2020

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

BrainSlayer picture BrainSlayer on 31 Aug 2020

@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 👍

Ornias1993 picture Ornias1993 on 31 Aug 2020

@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

BrainSlayer picture BrainSlayer on 31 Aug 2020
👍1

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.

allanjude picture allanjude on 31 Aug 2020
👍3

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.

terrelln picture terrelln on 31 Aug 2020

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? :)

Ornias1993 picture Ornias1993 on 31 Aug 2020

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

BrainSlayer picture BrainSlayer on 1 Sep 2020

@BrainSlayer Why are you asking me? go ask Allan, its his work not mine. Guess he is more than qualified to explain it way better than I understand it ;)

Ornias1993 picture Ornias1993 on 1 Sep 2020

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

BrainSlayer picture BrainSlayer on 1 Sep 2020

@BrainSlayer: @Ornias1993 replied to @allanjude bringing it up:

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

So I undersrood Allan is working on it and even if if I had no idea who he was, a quick look at his profile picture would have me guess this work to be BSD-related. :wink:

mschilli87 picture mschilli87 on 1 Sep 2020

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

BrainSlayer picture BrainSlayer on 1 Sep 2020
😕1

@BrainSlayer:

alan[sic!] was talking about a bootloader. i dont see that this is related to any optimization

Allan was giving input on 'reuse-context optimization' pointing you to the API part about resetting a CCtx because he had this on his list of potential future enhancements. He just mentioned the bootloader work as a reason for not getting to it earlier and @Ornias1993 wen't on a tangent there because he is interested in this (independent from ZST in ZFS optimizations). He also clearly labelled his comment as off-topic.
With a quick search you could find that this came up before on a PR you are probably very familiar with. :wink:

So no: The bootloader is not related to optimization other than @allanjude thinking about both and mentioning one when talking about the other, but @Ornias1993 still was interested and followed up. Then you seemed interested in / confused about that tangent and asked @Ornias1993 to give you the very details he just inquired from @allenjude and I simply tried to point you to the context I thought you must have overlooked when reading through this thread.

I am sorry if I further contributed to your confusion but as far as I understand you can safely ignore the bootloader topic unless you care about it independently from your interest in optimizing ZST on ZFS (like @allanjude and @Ornias1993 apparently do). :slightly_smiling_face:

mschilli87 picture mschilli87 on 1 Sep 2020

@mschilli87 As always: You made comment of the day and my day has just started :)

Ornias1993 picture Ornias1993 on 1 Sep 2020

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

Ornias1993 picture Ornias1993 on 29 Sep 2020

@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

BrainSlayer picture BrainSlayer on 29 Sep 2020
👍1

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

Ornias1993 picture Ornias1993 on 29 Sep 2020
Was this page helpful?
0 / 5 - 0 ratings

Related issues

dbavatar picture dbavatar  ·  4Comments
nh2 picture nh2  ·  3Comments
FransUrbo picture FransUrbo  ·  4Comments
nwf picture nwf  ·  4Comments
cinterloper picture cinterloper  ·  3Comments