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

Use Multi-buffer sha256 support from SPL #6546

Closed
wants to merge 1 commit into from

Conversation

dong-liuliu
Copy link

Description

Let sha256 checksum using multi-buffer api if it is exported by SPL

Motivation and Context

Using multi-buffer type, performance of sha256 will be increased 2~7 times.
Now a patch for multi-buffer sha256 facility in kernel space is implemented and submitted to SPL.
Its userspace facility and sha512 parts will be following up after this patch is reviewed and commented.

How Has This Been Tested?

Run FIO sequential write test, on Intel Xeon server (Haswell E5-2699 v3, 18 core), with 6x SSD :

Sha256 CPU-sys% BW(MB/s)
multi-buffer version 27 1859
icp version 71 1876

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:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@loli10K
Copy link
Contributor

loli10K commented Aug 23, 2017

@dong-liuliu you can add Requires-spl: refs/pull/646/head to the commit message to tell the buildbot this requires changes from openzfs/spl#646.

See https://github.com/zfsonlinux/zfs/wiki/Buildbot-Options#requiring-spl-versions

@dong-liuliu dong-liuliu force-pushed the master branch 3 times, most recently from 803468a to fc293c0 Compare August 23, 2017 09:00
If SPL is configured with multi-buffer hash, it will export a C
predefine marco HAVE_HASH_MB and C api mulbuf_sha256 to ZFS. Using
this api, performance of sha256 will be increased 2~7 times.

Signed-off-by: Xiaodong Liu <xiaodong.liu@intel.com>
Requires-spl: refs/pull/646/head
@behlendorf
Copy link
Contributor

@dong-liuliu very nice! So we're currently in the process of pulling the SPL source in to the ZFS tree #6479. I'd like to defer making this change until we have everything in one source tree.

@dong-liuliu
Copy link
Author

Thank you, @behlendorf, @loli10K . Let's check it after #6479 process

@kernelOfTruth
Copy link
Contributor

Are the buildbots even checking the proper functionality of sha256 checksums with the new multibuffer api ?

a quick search for sha256 didn't turn up anything: https://github.com/zfsonlinux/zfs-buildbot/search?utf8=%E2%9C%93&q=sha256&type=

@dong-liuliu
Copy link
Author

No, the buildbots just checked that if not configure with multi-bufer hash, this patch won't impact ZFS.
Checking work on new multibuffer api needs a configured SPL which is configured and built with ISA-L crypto's object file.

@behlendorf
Copy link
Contributor

Right, but once the SPL is in-tree we'll want to give some thought as to if it's possible to configure a builder to provide test coverage for this.

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Feb 9, 2018
@behlendorf
Copy link
Contributor

@dong-liuliu sorry about the delay but the spl has now been merged to the zfs repository. If you're still interested in getting this support included please go ahead and open a new PR against ZFS.

@behlendorf behlendorf closed this May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants