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.

Sign up for GitHub

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

Jump to bottom

Merge SPL into ZFS [WIP] #6479

Closed
wants to merge 3 commits into from
Closed

Merge SPL into ZFS [WIP] #6479

wants to merge 3 commits into from

Conversation

ofaaland
Copy link
Contributor

@ofaaland ofaaland commented Aug 8, 2017 •
edited
Loading

Description

Merge the SPL into ZFS to eliminate the extra work required when SPL code must change due to kernel or distro changes, and to simplify the build process.

Work In Progress.

Motivation and Context

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:

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

@ofaaland ofaaland added the Status: Work in Progress Not yet ready for general review label Aug 8, 2017
@chrisrd
Copy link
Contributor

chrisrd commented Aug 9, 2017

Can/should we be importing the SPL history rather than simply importing the files at a point in time?

@jspiros
Copy link

jspiros commented Aug 9, 2017

@chrisrd It has been a while since I've done it, and there are a couple different approaches that can be taken, but it is certainly possible to retain SPL history in a combined git repository. git subtree or git merge --allow-unrelated-histories are two tools that could be used off the top of my head. I do think doing it, one way or another, would be the right way to go, if combining SPL and ZFS is happening either way. (Note: I have nothing to do with this effort, just commenting.)

@ofaaland
Copy link
Contributor Author

ofaaland commented Aug 9, 2017

@chrisrd @jspiros, thanks for the suggestions. I'll take a look once I've got the functional issues corrected.

@behlendorf
Copy link
Contributor

Keeping the history would be nice. Particularly if we could somehow limit it only the specific files we want to merge.

@tuxoko
Copy link
Contributor

tuxoko commented Aug 9, 2017

somehow limit it only the specific files we want to merge

You should be able to just remove those files you don't want before merge.

Also, we should probably use git merge instead of git subtree, since spl and zfs have similar directory structure, and we probably want to merge the directory structure instead of making spl exist in a subdirectory.

It's very simple in principle. Just add spl repo as a remote. Fetch it, and merge it into zfs master like a normal branch. Though there will likely be quite a lot of conflict in the common files like Makefiles and stuffs.

@chrisrd
Copy link
Contributor

chrisrd commented Aug 9, 2017

Which files shouldn't be merged? (Apart from obvious things like empty .h files, e.g. spl/include/sys/fm/util.h)

FYI, you can start looking at the merge (with history) using, in your zfs repository:

git remote add spl https://github.com/zfsonlinux/spl.git
git fetch spl
git checkout -b spl-merge
git merge --allow-unrelated-histories spl/master

I found this useful for looking through the conflicts:

less +/'^(<<<<<|=====|>>>>>)' $(git status | sed -rn 's/^[[:space:]]*both added:[[:space:]]*//p')

To my mind, it should be relatively straight forward to merge the technical parts. As a licensing neophyte the trickier part of the merge is deciding what to do with the GPL vs CDDL licensing issue. E.g. files which appear in both repositories which look to have have come from upstream on the zfs side with, I guess, implied copyright, vs newly written, with explicit copyright header, on the spl side, e.g. config/Rules.am

@behlendorf
Copy link
Contributor

Which files shouldn't be merged?

I don't think we need to bring over the SPLAT sources. This test framework was extremely useful when initially implementing the various primitives, but now that we have good test coverage for ZFS in place it's much less useful. Except for specific cases ZFS itself does a much better job validating that code is working as intended.

As a licensing neophyte the trickier part of the merge is deciding what to do with the GPL vs CDDL licensing issue.

It would be best to keep the SPL code and headers entirely separate in their own sub-directory. We'll need to double check that every file has the correct license at the top, and we're going to want to add a top-level LICENSE file clearly describing which parts of the source tree are covered by which license. This would also be a good time to reach out to anyone who's contributed to the SPL and determine if we can dual license everything from the SPL as CDDL / GPL.

@ofaaland
Copy link
Contributor Author

Pushed an update to allows packages to be built and include SPL.
I haven't yet attempted to use the tips above to get the spl commit history in.

@ofaaland ofaaland mentioned this pull request Aug 12, 2017
Closed
@ofaaland ofaaland force-pushed the b_merge_spl_b branch 6 times, most recently from e587451 to c4e4456 Compare August 17, 2017 23:56
@behlendorf behlendorf mentioned this pull request Aug 23, 2017
Closed
13 tasks
@ofaaland ofaaland force-pushed the b_merge_spl_b branch 2 times, most recently from 55cd0b4 to d592d0f Compare August 31, 2017 22:22
@ofaaland
Add SPL version 0.7.0 files
f78f0a3 
Add SPL files as of spl-0.7.0 release.  No integration, Makefile
adjustments, etc. so they are not built.

Build-SPL: no
Build-ZFS: yes
Build-Lustre: no

Signed-off-by: Olaf Faaland <[email protected]>
@ofaaland
Correct locations for SPL headers and build SPL
93811f3 
* spl autoconf checks
* use zfs_config.h instead of spl_config.h
* generate a Makefile for spl module
* SPL include files for modules under include/spl
* Mark SPL module as GPL (different than META_LICENSE)
* fixup to unload_modules() in zfs-common.sh
* Add spl.ko to list of ZFS modules
* Remove splat
* Include SPL in built packages
* Add a script for fixing style issues, to be run
against each file e.g.

find ./include -name '*.h' | xargs -n1 scripts/fix-spl-style

* Update dkms build script
* Update zimport.sh to skip SPL build if no appropriate tag
is present, so old and new versions of ZFS are built properly.
* Replace SPL uses of DEFINE_* and DECLARE_* with non-constant arguments
that do not build under std=gnu99.

Build-SPL: no
Build-ZFS: yes
Build-Lustre: no

Signed-off-by: Olaf Faaland <[email protected]>
@ofaaland
Remove instances of kernel ratelimit type
150b586 
There is no dynamic initializer for the ratelimit type provided by
the kernel, but the provided macro which both declares and initializes
cannot build under std=gnu99 because the value used to initialize the
lock field in the structure is not constant.

The two cases where the type was used should be very infrequent anyway.

Build-SPL: no
Build-ZFS: yes
Build-Lustre: no

Signed-off-by: Olaf Faaland <[email protected]>
@dinatale2
Copy link
Contributor

dinatale2 commented Sep 12, 2017 •
edited
Loading

@ofaaland Can you retire the freemem macro in include/sys/vmsystm.h as part of this PR? I'm moving it's functionality to module/zfs/arc.c.

@behlendorf behlendorf added this to the 0.8.0 milestone Feb 9, 2018
@behlendorf
Copy link
Contributor

Closing. I'll open a new PR for this in the next few weeks.

@behlendorf behlendorf closed this Mar 21, 2018
@ofaaland ofaaland deleted the b_merge_spl_b branch September 5, 2018 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ofaaland @chrisrd @jspiros @behlendorf @tuxoko @dinatale2