11667 remove duplicate lz4 implementations

Review Request #2289 — Created Sept. 9, 2019 and submitted

tsoome
illumos-gate
master
11667
daff975...
general
11667 remove duplicate lz4 implementations

Listed in issue.

andy_js
  1. Ship It!
  2. 
      
tsoome
tsoome
tsoome
mahrens
  1. Changing the LZ4 code used by ZFS should be done with care, because it impacts the on-disk format. Changing the compressed bytesteam at all can have unexpected consequences, even if it can still be decompressed by older ZFS LZ4 code. I think the areas to look out for are l2arc (w/ compressed ARC disabled) and "nopwrite". Allan Jude has investigated this in the context of trying to update the LZ4 code in ZFS to a later version of LZ4.

    It isn't impossible to have one LZ4 implementation for ZFS and other uses, but you may find that the other use cases have requirements that are at odds with ZFS's. (e.g. other use cases would prefer to always have the most up-to-date LZ4 code, whereas the considerations mentioned above require that we stick with older LZ4 code for ZFS).

    1. Yep, just to emphasise - we do not update the lz4, we move the uts/common/fs/zfs/lz4.c to common/lz4, then we do fix few issues pointer by compilers - like dropping const qualifier etc. This allows us to share one single code with loader (which needs it to read zfs and uncompress forth dictionary and console font) and userland makesoftcore and vtfontcvt.

  2. 
      
citrus
  1. Ship It!
  2. 
      
tsoome
tsoome
richlowe
  1. Can you add links to diffs of the other lz4.c versus the new ones, so we can see how they (logically) change too?

    1. I did update the issue with step to step explanations of the changes.

      Diffs are available at: http://148-52-235-80.sta.estpak.ee/lz4/

    2. Sounds reasonable. I guess my last question is: Did you check for problems like the one when you made this same change for zlib, and found it called things that can't be used as early in boot as it was used?

      I don't think we support lz4 of/in the boot archive, right? Do we do anything else with it early?

    3. No, we do not support it for BA, also in kernel, we only do use it for zfs.

  2. 
      
tsoome
citrus
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...