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

Support zstd compression (port of Allan Judes patch from FreeBSD) #8941

Closed
wants to merge 6 commits into from

Conversation

BrainSlayer
Copy link
Contributor

@BrainSlayer BrainSlayer commented Jun 20, 2019

This Patch adds zstd compression support zo ZFS

Note:
this is a rework of the original pull request to fullfill the requirement of only offering a single patch. unfortunatly it seems that i have to spend alot of work every day to maintain this pull requests since it gets in conflict with the upstream master quickly so it will evolve quickly and will change usually every day.

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

Motivation and Context

Description

How Has This Been Tested?

Types of changes

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

Checklist:

@behlendorf behlendorf added the Status: Design Review Needed Architecture or design is under discussion label Jun 20, 2019
@c0d3z3r0
Copy link
Contributor

oh man... seems like a never-ending story...
can we get this reviewed and merged anytime soon? @allanjude @behlendorf @kpande @ahrens

@allanjude
Copy link
Contributor

There is newer code on my side as well, that cleans up a number of issues and sorts out some of the API changes. I am looking at proposing it as a number of separate pull requests to make it easier to review as well.

@BrainSlayer
Copy link
Contributor Author

BrainSlayer commented Jun 28, 2019 via email

@BrainSlayer
Copy link
Contributor Author

in addition. i run this code on my main filesystem on a server for compiling sourcecodes on my router firmware project which proofed that even the current zol variant is bullet proof stable. (at least for me). i would appreciate if it gets merged to the development tree finally. so we can focus to enhance the api for bsd code sharing as next step. but we finally need a shared codebase to track changes from your side

@dan-and
Copy link

dan-and commented Jul 2, 2019

in addition. i run this code on my main filesystem on a server for compiling sourcecodes on my router firmware project which proofed that even the current zol variant is bullet proof stable. (at least for me).

Same on my side. I run 2 systems (1x Proxmox Hypervisor with Kernel 4.15) and a Kernel 5.0 Ubuntu machine with mentioned ZSTD patches for several months without problems aswell.

@c0d3z3r0
Copy link
Contributor

c0d3z3r0 commented Jul 9, 2019

in addition. i run this code on my main filesystem on a server for compiling sourcecodes on my router firmware project which proofed that even the current zol variant is bullet proof stable. (at least for me).

Same on my side. I run 2 systems (1x Proxmox Hypervisor with Kernel 4.15) and a Kernel 5.0 Ubuntu machine with mentioned ZSTD patches for several months without problems aswell.

me too

@c0d3z3r0
Copy link
Contributor

c0d3z3r0 commented Jul 9, 2019

@allanjude everyone is waiting on you :P

@allanjude
Copy link
Contributor

I am aware, I'll try to get things posted as soon as I can

@allanjude allanjude mentioned this pull request Jul 12, 2019
12 tasks
@allanjude
Copy link
Contributor

My updated version has been posted finally: #9024

@BrainSlayer BrainSlayer force-pushed the master branch 10 times, most recently from 421255d to fe68cd2 Compare July 27, 2019 14:05
@BrainSlayer
Copy link
Contributor Author

My updated version has been posted finally: #9024

i will review your version soon and merge it with my changes. i already have seen that your version has some issues which have been resolved in my tree. mainly the memory allocation issues arent handled by your code and will fail

@BrainSlayer BrainSlayer force-pushed the master branch 7 times, most recently from e489c0f to 568593c Compare July 28, 2019 20:06
@PrivatePuffin
Copy link
Contributor

/* We might still need this in the future */

is a very bad reason to keep code that's outside test coverage anyway.

@BrainSlayer
Copy link
Contributor Author

yes and as i said. its simple enough to bring it back if required one time in future.

@BrainSlayer
Copy link
Contributor Author

@Ornias1993 @c0d3z3r0 finally i managed how to use git merge + rebase to get clean history :-)

@PrivatePuffin
Copy link
Contributor

Someone deserves a cookie!

@c0d3z3r0
Copy link
Contributor

yay for random crashes....... Amazon 2 unrelated

@BrainSlayer
Copy link
Contributor Author

@c0d3z3r0 i dont expect any error here related to my last commits. these are cosmetic

@c0d3z3r0
Copy link
Contributor

@c0d3z3r0 i dont expect any error here related to my last commits. these are cosmetic

yeah, seems unrelated at all to me

@c0d3z3r0
Copy link
Contributor

@c0d3z3r0 i dont expect any error here related to my last commits. these are cosmetic

yeah, seems unrelated at all to me

oh.. oops. just realized I commented on the wrong PR... sorry

@dan-and
Copy link

dan-and commented Dec 12, 2019

Just FYI:
I have transferred around 80 TB of data back and forth using the last patch on pull 8941. I switched between several zstd and zstd-fast variants. All good so far. No problems at all.

@BrainSlayer
Copy link
Contributor Author

@dan-and cool. thanks for update. any performance issues or everything running smooth?

@dan-and
Copy link

dan-and commented Dec 12, 2019

All running smooth. I used compressible and non-compressible data. Performance just great.
When using zstd over 15 my old trusty Xeon is a bit too slow to reach the harddisk performance. However that was already clear by the ramdisk tests before.

It's all fine

@PrivatePuffin
Copy link
Contributor

@dan-and Thats what we wanna hear! :)

Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
in case a background thread released already a object at that time, we
are able to reuse it. so we dont need to skip the object if the size
fits for our needs

Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
right now kmem_alloc using KM_SLEEP will never fail since its dead lock
routing, so the double mutex_exit will not happen. but implementation
wise its still wrong. there are future plans to enhance the memory
allocation to allow it to be failed under OOM conditions to prevent
deadlocks. so this bad code here must be fixed

Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
previously this macro was unavailable in zfs, but since its now
introduced we dont need to use our own solution anymore

Signed-off-by: Sebastian Gottschall <s.gottschall@dd-wrt.com>
@PrivatePuffin
Copy link
Contributor

@BrainSlayer Why the force push?

@BrainSlayer
Copy link
Contributor Author

@Ornias1993 just rebase to be in sync with upstream

@c0d3z3r0 c0d3z3r0 mentioned this pull request Dec 16, 2019
12 tasks
@BrainSlayer
Copy link
Contributor Author

i will close this PR here in favor of #9735 which is the successor of this PR based on the same code.

@PrivatePuffin
Copy link
Contributor

LGTM
Lets Get THAT merged, in this case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Design Review Needed Architecture or design is under discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.