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

Factor out non-portable vnode_t usage #9556

Merged
merged 1 commit into from
Nov 21, 2019

Conversation

mattmacy
Copy link
Contributor

@mattmacy mattmacy commented Nov 6, 2019

On FreeBSD file offset state is maintained in struct file. A given
vnode can be referenced from many different struct file *. As a
consequence, FreeBSD's SPL doesn't support vn_rdwr with the FAPPEND
flag.

This change replaces the non-portable vnode_t with the portable
zfs_file_t in the common code.

Upon discussion with Brian - have expanded the scope to eliminate all references to vn_rdwr and have limited the scope of the use of the vn_ functions to vdev_file.c

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:

@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch from 87cf8a0 to f69e95e Compare November 6, 2019 02:07
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 6, 2019
@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch 22 times, most recently from 2ebe49d to 3ccebe9 Compare November 12, 2019 07:25
@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Nov 12, 2019
@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch 4 times, most recently from 7111c86 to 470f2a6 Compare November 13, 2019 04:11
@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch 6 times, most recently from 27dc2f9 to 4f97fd4 Compare November 15, 2019 23:29
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.

LGTM, thanks

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.

It's awesome to see this refactoring removes 600 lines of mostly ugly compatibility code!

include/os/linux/zfs/sys/zfs_vnops.h Outdated Show resolved Hide resolved
include/sys/vdev_file.h Outdated Show resolved Hide resolved
include/sys/zfs_file.h Outdated Show resolved Hide resolved
include/sys/zfs_file.h Show resolved Hide resolved
lib/libzpool/kernel.c Outdated Show resolved Hide resolved
module/zfs/spa_config.c Outdated Show resolved Hide resolved
module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved
module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved
module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved
module/zfs/zfs_ioctl.c Outdated Show resolved Hide resolved
@lundman
Copy link
Contributor

lundman commented Nov 18, 2019

vtype -> mode, file_t -> zfs_file_t, there is a lot to like in this version.

and the rest can be handled with wrappers in libspl for things like umem_zalloc() in libzpool/kernel.c etc.

@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch 4 times, most recently from b459dc4 to 2727960 Compare November 19, 2019 04:27
@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

Merging #9556 into master will decrease coverage by 0.29%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #9556     +/-   ##
=========================================
- Coverage   79.22%   78.93%   -0.3%     
=========================================
  Files         419      418      -1     
  Lines      123704   123531    -173     
=========================================
- Hits        98009    97504    -505     
- Misses      25695    26027    +332
Flag Coverage Δ
#kernel 79.7% <85.47%> (-0.22%) ⬇️
#user 66.28% <68.58%> (-0.45%) ⬇️

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 540312d...4a1f22c. Read the comment docs.

@kithrup
Copy link
Contributor

kithrup commented Nov 19, 2019

A lot of this seems to be adding more indirection, so we can avoid the problems of, say, Linux and FreeBSD having different file and vnode types.

Given that, it seems fine to me.

@behlendorf
Copy link
Contributor

@kithrup thanks for the feedback. That's right, the intent is to provide a basic interface which can be implemented on all platforms which can be used by the common code. The vnode interface effectively served this same purpose, but the compatibility code required to support it ended up being a bit gnarly. After this change there are longer any vnode /VOP calls or related structures in the common code.

@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch from 2727960 to b4edfe5 Compare November 19, 2019 23:22
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing Status: Work in Progress Not yet ready for general review labels Nov 20, 2019
On FreeBSD file offset state is maintained in struct file. A given
vnode can be referenced from many different struct file *. As a
consequence, FreeBSD's SPL doesn't support vn_rdwr with the FAPPEND
flag.

This change replaces the non-portable vnode_t with the portable
file_t in the common code.

Signed-off-by: Matt Macy <[email protected]>
@mattmacy mattmacy force-pushed the projects/dmu_struct_file branch from b4edfe5 to 4a1f22c Compare November 20, 2019 20:46
@mattmacy
Copy link
Contributor Author

mattmacy commented Nov 20, 2019

A lot of this seems to be adding more indirection, so we can avoid the problems of, say, Linux and FreeBSD having different file and vnode types.

Given that, it seems fine to me.

It only really adds extra indirection for Illumos. FreeBSD doesn’t support the FAPPEND flag to vn_rdwr and would need its own complicated spl vnode in order to support it. As is FreeBSD had a bunch of special case handling and routines to pass a struct file * where Illumos was passing a vnode.

@behlendorf behlendorf merged commit da92d5c into openzfs:master Nov 21, 2019
@mattmacy mattmacy deleted the projects/dmu_struct_file branch December 19, 2019 22:26
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