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

Refactor zvol in to platform and platform independent bits #9295

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

mattmacy
Copy link
Contributor

@mattmacy mattmacy commented Sep 6, 2019

Signed-off-by: Matt Macy [email protected]

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:

Copy link
Contributor

@allanjude allanjude left a comment

Choose a reason for hiding this comment

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

Reviewed By: Allan Jude [email protected]

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 for tackling this further refactoring. Before we move ahead with this I'd like to investigate using a slightly more traditional approach and have the platform specific code register callbacks with the common code. The common code would then only need to provide platform independent functionality and all the Linux/FreeBSD/MacOS/ specific code/headers could be private. I'm happy to help hash out any design details.

@behlendorf behlendorf added the Status: Revision Needed Changes are required for the PR to be accepted label Sep 10, 2019
@mattmacy mattmacy force-pushed the projects/platform_zvol branch 5 times, most recently from 5091ab9 to cf04ec0 Compare September 11, 2019 20:11
@mattmacy mattmacy force-pushed the projects/platform_zvol branch 2 times, most recently from 6f6a857 to 16383cc Compare September 11, 2019 21:20
@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Revision Needed Changes are required for the PR to be accepted labels Sep 11, 2019
@mattmacy mattmacy force-pushed the projects/platform_zvol branch from 16383cc to 472471a Compare September 12, 2019 21:27
@codecov
Copy link

codecov bot commented Sep 13, 2019

Codecov Report

Merging #9295 into master will decrease coverage by 12.37%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9295       +/-   ##
===========================================
- Coverage   79.06%   66.68%   -12.38%     
===========================================
  Files         401      325       -76     
  Lines      122495   105116    -17379     
===========================================
- Hits        96846    70100    -26746     
- Misses      25649    35016     +9367
Flag Coverage Δ
#kernel ?
#user 66.68% <100%> (+0.05%) ⬆️

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 73d7820...0917102. Read the comment docs.

@behlendorf
Copy link
Contributor

@ikozhukhov @lundman would you mind looking at this proposed refactoring to see if it's flexible enough to accommodate your platforms.

@mattmacy mattmacy force-pushed the projects/platform_zvol branch from 472471a to 4051ce1 Compare September 23, 2019 23:14
@ghost ghost force-pushed the projects/platform_zvol branch from 4051ce1 to 20d7ba2 Compare September 24, 2019 14:38
@ghost
Copy link

ghost commented Sep 24, 2019

Rebased and changed void (*zv_free)(void *) to void (*zv_free)(zvol_state_t *) (and adjusted accordingly in the two places affected by this).

@ghost ghost force-pushed the projects/platform_zvol branch from 20d7ba2 to 0917102 Compare September 24, 2019 18:27
@ghost
Copy link

ghost commented Sep 24, 2019

void set_disk_ro -> static void zvol_set_disk_ro_impl
void set_capacity -> static void zvol_set_capacity_impl
Eliminated some reordering diffs.
No functional changes.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 24, 2019
@behlendorf
Copy link
Contributor

The Amazon Linux 2 bot hit #9357 which appears to be a existing issue. Regardless, I've resubmitted that build for good measure.

@lundman
Copy link
Contributor

lundman commented Sep 24, 2019

Oh I like this idea

@lundman
Copy link
Contributor

lundman commented Sep 24, 2019

Biggest changes for us is that we call C++/IOkit to create/remove the device node(s), which has to be named /dev/disk - we are nice and create symlinks for users as to maintain /var/run/zfs/dsk/$pool/$zvol - but since we can't create symlinks in kernel, we talk to zed to do that. Rename is just re-creating symlinks. But all of that should fit into zvol_os.c (and our own zvolIO.cpp) nicely. The code that looks inside uio has also moved into zvol_os.c so that is a bonus for us (as it is opaque on osx).
This will be much nicer, I look forward to attacking it locally.

Copy link
Contributor

@ikozhukhov ikozhukhov left a comment

Choose a reason for hiding this comment

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

well, will see how to try to use/reuse it with dilos builds
thanks for update!

@behlendorf
Copy link
Contributor

@lundman @ikozhukhov thanks for making the time to look this over. I'll go ahead and get this merged once the bots wrap up their testing.

@behlendorf behlendorf merged commit 5df7e9d into openzfs:master Sep 25, 2019
@mattmacy mattmacy deleted the projects/platform_zvol branch December 19, 2019 22:29
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.

5 participants