11667 remove duplicate lz4 implementations

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

Information
tsoome
illumos-gate
master
11667
daff975...
Reviewers
general 
11667 remove duplicate lz4 implementations
Listed in issue.
andy_js
andy_js 
  1. Ship It!
  2. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 



 

  

   




tsoome




   

   

  

 



 















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




mahrens




   

   





  

 



 

  


   

   

    
 
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. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




citrus




   

   







  

 



 

  


   

   

    
 
citrus


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




richlowe




   

   





  

 



 

  


   

   

    
 
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. 



    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. 



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

      4. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




citrus




   

   







  

 



 

  


   

   

    
 
citrus


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...