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

Upstream: add macOS calls to AVL #10390

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Conversation

lundman
Copy link
Contributor

@lundman lundman commented Jun 1, 2020

macOS kmem uses avl_update() and related functions. If other platforms
get "defined but not used" warnings, they may need to be wrapped
in #ifdefs.

This is the first PR to add macOS support to the OpenZFS repo. Will start slow to work out the procedure and find a good balance. Also unsure how upstream platforms handle functions defined but not used.

Be extra picky here, so future PRs will go smoother :)

For a sneak peek of future PRs have a look at;
openzfsonosx/openzfs#4

Motivation and Context

Assuming a macOS port is desired, this is required.

Description

Additional AVL methods lifted directly from illumos.

How Has This Been Tested?

Running zfs-tests on macOS VMs.

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.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2020

Codecov Report

Merging #10390 into master will decrease coverage by 0.23%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10390      +/-   ##
==========================================
- Coverage   79.68%   79.44%   -0.24%     
==========================================
  Files         391      391              
  Lines      123603   123624      +21     
==========================================
- Hits        98492    98217     -275     
- Misses      25111    25407     +296     
Flag Coverage Δ
#kernel 79.95% <0.00%> (-0.04%) ⬇️
#user 65.45% <0.00%> (-0.94%) ⬇️
Impacted Files Coverage Δ
module/avl/avl.c 89.93% <0.00%> (-6.59%) ⬇️
module/zfs/vdev_indirect.c 73.50% <0.00%> (-6.84%) ⬇️
module/zfs/space_map.c 92.82% <0.00%> (-5.95%) ⬇️
module/zfs/spa_checkpoint.c 93.78% <0.00%> (-4.35%) ⬇️
module/zfs/bpobj.c 86.86% <0.00%> (-4.29%) ⬇️
module/lua/lmem.c 83.33% <0.00%> (-4.17%) ⬇️
cmd/ztest/ztest.c 78.98% <0.00%> (-3.37%) ⬇️
module/zcommon/zfs_fletcher.c 75.65% <0.00%> (-2.64%) ⬇️
module/zfs/zio_compress.c 89.74% <0.00%> (-2.57%) ⬇️
module/zfs/dsl_synctask.c 92.40% <0.00%> (-2.54%) ⬇️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9052e3d...426cd6c. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 1, 2020
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks like only one minor addition is needed. Providing these extra public library functions shouldn't be a problem for any of the platforms, plus now that they're available we may find they'll be useful elsewhere. I also verified the code matches what's in illumos, just for good measure.

*/
extern boolean_t avl_update(avl_tree_t *, void *);
extern boolean_t avl_update_lt(avl_tree_t *, void *);
extern boolean_t avl_update_gt(avl_tree_t *, void *);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add these three new functions in order to the list of exported symbols at the end of module/avl/avl.c. The EXPORT_SYMBOL line is needed on Linux to be able to call the functions from a different kmod.

macOS kmem uses avl_update() and related functions. If other platforms
get "defined but not used" warnings, they may need to be wrapped
in #ifdefs.

Signed-off-by: Jorgen Lundman <lundman@lundman.net>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 2, 2020
@behlendorf behlendorf merged commit 081de9a into openzfs:master Jun 3, 2020
as-com pushed a commit to as-com/zfs that referenced this pull request Jun 20, 2020
The macOS kmem implementation uses avl_update() and related
functions.  These same function exist in the Solaris AVL code but
were removed because they were unused.  Restore them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Jorgen Lundman <lundman@lundman.net>
Closes openzfs#10390 
(cherry picked from commit 081de9a)
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The macOS kmem implementation uses avl_update() and related
functions.  These same function exist in the Solaris AVL code but
were removed because they were unused.  Restore them.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Jorgen Lundman <lundman@lundman.net>
Closes openzfs#10390
@lundman lundman deleted the macOS_pr1 branch May 28, 2021 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants