zfs_vnops: make zfs_get_data OS-independent #10979

problame · 2020-09-25T13:39:42Z

zfs_vnops: refactor + make zfs_get_data OS-independent

- include/sys/zfs_vnops.h contains declarations of vnop functions
  that implement the same behavior on every platform.
  Function definitions are in
    - module/zfs/zfs_vnops.c for OS-independent code
    - module/os/*/zfs_vnops.c for OS-specific code

  In order to share the zfs_symlink declaration, the FreeBSD variant had
  to lose it's constness on the `char*` parameters.

- As a PoC, move zfs_get_data to zfs_vnops.c
    - This required creating a compatible zfs_zrele_async for FreeBSD.

Motivation

zfs_get_data is almost platform-independent
- it's sibling zvol_get_data already is platform-independent
the only platform-specific aspect of it is the way we release an inode (Linux) / vnode_t (FreeBSD)
I am not aware of a platform that could be supported by ZFS that couldn't implement zfs_rele_async itself.

Future work: evaluate whether we could make all interactions with inode / vnode lifecycle management C-style virtual function calls (i.e. store function pointers to the platform-specific lifecycle functions in the znode).

How Has This Been Tested?

Straight-forward refactor -> rely on ZFS test suite in OpenZFS CI.

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

problame · 2020-09-25T13:43:09Z

Requesting @mattmacy for review.

darkfiberiru · 2020-09-25T16:44:29Z

@problame Seems to be small typo in commit message 's/zfs_rele_async/zfs_zrele_async/'

problame · 2020-09-25T17:20:32Z

http://build.zfsonlinux.org/builders/FreeBSD%20stable%2F12%20amd64%20%28TEST%29/builds/2432/steps/shell_6/logs/stdio

kldload: an error occurred while loading module openzfs. Please check dmesg(8) for more details.

This is the ztest stage.

I'm afraid I don't understand why the module builds on FreeBSD (but doesn't load?).
Also, it would be helfpul if buildbot captured dmesg somewhere in that stage.

problame · 2020-09-25T17:21:05Z

Fix commit message zfs_vnops: make zfs_get_data OS-independent #10979 (comment)

ghost · 2020-09-25T17:38:59Z

http://build.zfsonlinux.org/builders/FreeBSD%20stable%2F12%20amd64%20%28TEST%29/builds/2432/steps/shell_6/logs/stdio
kldload: an error occurred while loading module openzfs. Please check dmesg(8) for more details.
This is the ztest stage.

I'm afraid I don't understand why the module builds on FreeBSD (but doesn't load?).
Also, it would be helfpul if buildbot captured dmesg somewhere in that stage.

Dmesg is captured as console

You'll need to add the new file to module/Makefile.bsd for the FreeBSD build.

ghost · 2020-09-25T19:29:07Z

Welcome to the jungle... 😅

problame · 2020-09-25T19:55:18Z

I'll set up a FreeBSD dev system.

problame · 2020-09-25T19:57:47Z

How do you feel about this refactor in general?
I feel like there are a bunch of more functions in zfs_vnops that are almost platform-independent.
Look at zfs_write for example.

Is this duplication intentional and intended to stay long-term or was it just easier to initially duplicate code for a faster FreeBSD port?

ghost · 2020-09-25T22:41:52Z

It was easier and faster to keep the the ZPL bits in vnops/vfsops separate. There is indeed a lot of refactoring that should be done to share more of the code, and I see that as a worthwhile effort.

I haven't closely audited your changes yet, but at a glance there is one suggestion I have, apart from setting yourself up with a FreeBSD VM and getting it to build on FreeBSD. Rather than *_common.c, the convention has been to name the platform-dependent files *_os.c where there is a name collision between files in common and platform code.

problame · 2020-09-26T13:07:45Z

I haven't closely audited your changes yet, but at a glance there is one suggestion I have, apart from setting yourself up with a FreeBSD VM and getting it to build on FreeBSD. Rather than *_common.c, the convention has been to name the platform-dependent files *_os.c where there is a name collision between files in common and platform code.

I restructured the PR to follow this pattern and updated the commit message accordingly.
It should be usable for future refactorings of the ZPL.

I set up a test VM with FreeBSD 12.1 release and found that my version of master didn't compile there.
I added the missing includes in a separate commit included in this PR.

ghost · 2020-09-26T18:57:14Z

The build errors appear to be all 32-bit platforms, except Fedora. Not sure what happened to that one.

module/os/linux/zfs/zfs_ctldir.c

module/zfs/zfs_vnops.c

problame · 2020-10-07T16:56:51Z

@freqlabs I addressed the issues mentioned in the review and rebased onto master. Your Throw const on some strings forced me to move most of the functions in the shared zfs_vnops.h back into zfs_vnops_os.h since the const-ness is only on FreeBSD.

ghost · 2020-10-07T17:13:46Z

@freqlabs I addressed the issues mentioned in the review and rebased onto master. Your Throw const on some strings forced me to move most of the functions in the shared zfs_vnops.h back into zfs_vnops_os.h since the const-ness is only on FreeBSD.

There seemed to be some differences after all in how those functions are implemented. I don't fully understand why, but as I recall the Linux code passes around ownership of some strings instead of merely borrowing the strings from the caller. The eventual call to free forces that the strings not be const in a number of functions.

It would be good to investigate further if it is really necessary to pass around the string ownership like that.

problame · 2020-10-07T17:24:45Z

Agreed, although that won't be part of this PR.

mattmacy · 2020-10-15T23:53:40Z

I wasn't aware of this PR and just re-did the bulk of the re-factoring. I need to share some common code for the async I/O rework. zfs_fsync is a simpler place to start BTW.

problame · 2020-10-16T09:19:21Z

@mattmacy certainly. Could you please link to the commits where you did this refactoring?

mattmacy · 2020-10-16T16:42:13Z

https://github.com/zfsonfreebsd/ZoF/commits/projects/zfs_vnops

mattmacy · 2020-10-16T21:41:50Z

Please see also #11078

mattmacy · 2020-10-16T22:30:39Z

@problame It looks like the core of your changes should be just drop in on top of mine

problame · 2020-10-17T10:45:20Z

I guess this PR is obsolete, then. Close at your discretion.

behlendorf · 2020-10-20T15:42:16Z

These changes nicely complement the work done in #11078. Let's get that PR in first then this work can be applied on top.

behlendorf · 2020-10-21T22:46:15Z

@problame with #11078 merged we should be all set to move zfs_get_data in to the common code.

mattmacy · 2020-10-27T19:10:33Z

@problame if you just keep your vnops changes we can get that merged

problame · 2020-10-27T20:57:18Z

Sry, I've been awfully busy in the last few days. I'll try to get it done until the end of the week!

closes openzfs#10979 Signed-off-by: Christian Schwarz <me@cschwarz.com>

problame · 2020-11-01T17:45:25Z

Rebase is done and it builds on my test VMs.

behlendorf

Looks good. Thanks!

Move zfs_get_data() in to platform-independent code. The only platform-specific aspect of it is the way we release an inode (Linux) / vnode_t (FreeBSD). I am not aware of a platform that could be supported by ZFS that couldn't implement zfs_rele_async itself. It's sibling zvol_get_data already is platform-independent. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Christian Schwarz <me@cschwarz.com> Closes openzfs#10979

Move zfs_get_data() in to platform-independent code. The only platform-specific aspect of it is the way we release an inode (Linux) / vnode_t (FreeBSD). I am not aware of a platform that could be supported by ZFS that couldn't implement zfs_rele_async itself. It's sibling zvol_get_data already is platform-independent. Reviewed-by: Ryan Moeller <ryan@iXsystems.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Christian Schwarz <me@cschwarz.com> Closes #10979

problame changed the title ~~os independent zfs get data~~ zfs_vnops: move zfs_get_data to OS-independent code Sep 25, 2020

problame force-pushed the os-independent-zfs_get_data branch 4 times, most recently from e7968c2 to c449627 Compare September 25, 2020 14:59

ghost added the Status: Work in Progress Not yet ready for general review label Sep 25, 2020

ghost self-requested a review September 25, 2020 17:09

problame force-pushed the os-independent-zfs_get_data branch from c449627 to 44145dd Compare September 25, 2020 17:46

problame force-pushed the os-independent-zfs_get_data branch 3 times, most recently from a35dc5e to 86d8a78 Compare September 26, 2020 13:07

problame changed the title ~~zfs_vnops: move zfs_get_data to OS-independent code~~ zfs_vnops: refactor + make zfs_get_data OS-independent Sep 26, 2020

ghost added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Sep 26, 2020

ghost reviewed Sep 26, 2020

View reviewed changes

module/os/linux/zfs/zfs_ctldir.c Outdated Show resolved Hide resolved

module/zfs/zfs_vnops.c Outdated Show resolved Hide resolved

problame force-pushed the os-independent-zfs_get_data branch from 86d8a78 to aa202c7 Compare October 7, 2020 14:47

behlendorf added Status: Work in Progress Not yet ready for general review and removed Status: Code Review Needed Ready for review and testing labels Oct 22, 2020

zfs_vnops: make zfs_get_data OS-independent

288ed4a

closes openzfs#10979 Signed-off-by: Christian Schwarz <me@cschwarz.com>

problame force-pushed the os-independent-zfs_get_data branch from aa202c7 to 288ed4a Compare November 1, 2020 16:27

ghost added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Nov 2, 2020

behlendorf approved these changes Nov 2, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 2, 2020

problame changed the title ~~zfs_vnops: refactor + make zfs_get_data OS-independent~~ zfs_vnops: make zfs_get_data OS-independent Nov 2, 2020

behlendorf merged commit ab8c935 into openzfs:master Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zfs_vnops: make zfs_get_data OS-independent #10979

zfs_vnops: make zfs_get_data OS-independent #10979

problame commented Sep 25, 2020 •

edited

Loading

problame commented Sep 25, 2020

darkfiberiru commented Sep 25, 2020

problame commented Sep 25, 2020

problame commented Sep 25, 2020 •

edited

Loading

ghost commented Sep 25, 2020

ghost commented Sep 25, 2020

problame commented Sep 25, 2020

problame commented Sep 25, 2020

ghost commented Sep 25, 2020

problame commented Sep 26, 2020

ghost commented Sep 26, 2020

problame commented Oct 7, 2020

ghost commented Oct 7, 2020 •

edited by ghost

Loading

problame commented Oct 7, 2020

mattmacy commented Oct 15, 2020

problame commented Oct 16, 2020

mattmacy commented Oct 16, 2020

mattmacy commented Oct 16, 2020

mattmacy commented Oct 16, 2020

problame commented Oct 17, 2020

behlendorf commented Oct 20, 2020

behlendorf commented Oct 21, 2020

mattmacy commented Oct 27, 2020

problame commented Oct 27, 2020

problame commented Nov 1, 2020

behlendorf left a comment

zfs_vnops: make zfs_get_data OS-independent #10979

zfs_vnops: make zfs_get_data OS-independent #10979

Conversation

problame commented Sep 25, 2020 • edited Loading

Motivation

How Has This Been Tested?

Types of changes

Checklist:

problame commented Sep 25, 2020

darkfiberiru commented Sep 25, 2020

problame commented Sep 25, 2020

problame commented Sep 25, 2020 • edited Loading

ghost commented Sep 25, 2020

ghost commented Sep 25, 2020

problame commented Sep 25, 2020

problame commented Sep 25, 2020

ghost commented Sep 25, 2020

problame commented Sep 26, 2020

ghost commented Sep 26, 2020

problame commented Oct 7, 2020

ghost commented Oct 7, 2020 • edited by ghost Loading

problame commented Oct 7, 2020

mattmacy commented Oct 15, 2020

problame commented Oct 16, 2020

mattmacy commented Oct 16, 2020

mattmacy commented Oct 16, 2020

mattmacy commented Oct 16, 2020

problame commented Oct 17, 2020

behlendorf commented Oct 20, 2020

behlendorf commented Oct 21, 2020

mattmacy commented Oct 27, 2020

problame commented Oct 27, 2020

problame commented Nov 1, 2020

behlendorf left a comment

Choose a reason for hiding this comment

problame commented Sep 25, 2020 •

edited

Loading

problame commented Sep 25, 2020 •

edited

Loading

ghost commented Oct 7, 2020 •

edited by ghost

Loading