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

vdev_file: unify FreeBSD and Linux implementations #17046

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

robn
Copy link
Member

@robn robn commented Feb 12, 2025

[Sponsors: Klara, Inc., Wasabi Technology, Inc.]

Motivation and Context

The platform-specifics for vdev_file are almost entirely hidden away in zfs_file_os.c. There's basically no reason for separate vdev_file.c to exist, so lets merge them.

Description

The process here was to load both files side-by-side in vimdiff, then move things around until there was no diff left. Where there were differences, I've taken the "best" version, though this is almost entirely in comments and asserts.

There are very minor changes to make it fit together sensibly:

Both:

  • FLUSH ZIOs are now queued via zio_interrupt() instead of executed directly (slightly nicer behaviour, and allows reuse of the vdev_file_io_fsync() in the inline case.
    FreeBSD:
  • honour zfs_nocacheflush for FLUSH ZIOs. This matches Linux, userspace, disk drivers for both platforms, and the documentation.
    Linux+userspace:
  • z_vdev_file is created using max_ncpus rather than boot_ncpus. I don't think there will be any practical difference.

The one wart is the Linux-specific thing to put flushes on the taskq if called within an filesystem transaction. For now I've gated that behind #ifdef __linux__. I wouldn't normally leave it there, but the reason I did this change at all is because I've got a change coming that always puts flushes on the taskq, so the wart will be removed with that.

I've put the respective diffs for each platform in their own commit, so the changes can be more easily seen. If this is approved, I will squash the commits before merge.

How Has This Been Tested?

Compiled on FreeBSD 14.2, Linux 6.1 and Linux 4.19. On all, ran zpool_add and replacement test tags, which aren't special but use file-backed pools, and also change vdev_file tuneables. All passed.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

robn added 2 commits February 12, 2025 15:23
This commit is here to make it easier to review the diffs for the
individual platform files and confirm they make sense for each platform.
It should be squashed into the next commit before merge.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
Kernel & userspace specifics are in zfs_file_os.c, so there's no
particular reason these have to be separate.

The one platform-specific part is in the Linux kernel part, to offload
flushes to a taskq if we're already inside a filesystem transaction.
This would be normally be an unsatisfying wart, but I'm intending to
remove this shortly, so I'm content to leave it gated for the moment.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@lundman
Copy link
Contributor

lundman commented Feb 12, 2025

You glanced at macOS and Windows to see if there are any upcoming surprises?

@robn
Copy link
Member Author

robn commented Feb 12, 2025

You glanced at macOS and Windows to see if there are any upcoming surprises?

@lundman Nope, because even if I had I don't know what the current plan is for them. I take it that means this is a breaking change. Tell me more? Are there changes there that require more than the (admittedly limited) zfs_file abstraction?

@lundman
Copy link
Contributor

lundman commented Feb 12, 2025

Not at all, I took a look now, and Windows doesn't look too bad, some path work before open.
io_done might need to use a workqueue, but over all pretty minor changes.
macOS has one hack needed, but otherwise the same.

@robn
Copy link
Member Author

robn commented Feb 12, 2025

I was just skimming myself, and didn't see anything too weird, though obviously I know nothing about it. Does seem like we could extend zfs_file to have a path transform hook of some sort.

I did wonder if it'd be better if it would be better if vdev_file went the other way, and properly separate for each platform (incl userspace), but at least on Linux+FreeBSD, we use in other places. Not much actually file-ish once you take send/recv away (maybe only zpool.cache), so possibly it makes sense to get rid of zfs_file instead. That's a bigger lift though.

The other possibility is to make zfs_file fully async, ie every func gets called with a callback function, and any required taskqs are pushed down there instead. I don't even mind that, makes it more like a device, and vdev_file more like a driver for it. I have not thought about this even a little bit.

I'll have a look soon (hopefully tonight) at the extra hooks mac/win might need. It might be straightforward to add those hooks in this PR and so avoid making a huge mess for you (just a small one) :)

@lundman
Copy link
Contributor

lundman commented Feb 12, 2025

I wouldn't worry about hooks just yet, be better to do that when I have caught up and can test it directly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants