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

[2.3.2] Revert "zinject: count matches and injections for each handler" #17137

Merged

Conversation

robn
Copy link
Member

@robn robn commented Mar 12, 2025

Motivation and Context

2.3.1 introduced a userspace/kernel ABI break that I missed. As a result, 2.3.1 userspace tools are incompatible with <2.3.1 kernel modules for many tasks.

After I updated userspace to 2.3.1, zfs send started returning EINVAL, and zfs events returned EBADF. I rebooted and didn't think much of it; my local machine is weird sometimes.

Later I noticed why: the "match counts" change adds a couple of fields to zinject_record_t. There's one of those embedded in the middle of zfs_cmd_t. zc_cleanup_fd and some of the send params both occur after that field. The kernel and userspace were running on different layouts, so couldn't find what they needed.

I don't know if you want to revert the change and cut a 2.3.2 release. If you do, this is the revert. It might be unnecessary though. depends how long before folks can reboot.

(also, an apology from me: feels like something I should have noticed. I'm gonna go put some compile time size checks on a few things to hopefully help prevent this next time).

Description

Adding fields to zinject_record_t unexpectedly extended zfs_cmd_t, preventing some things working properly with 2.3.1 userspace tools against 2.3.0 kernel module.

This reverts commit fabdd50.

How Has This Been Tested?

Compile checked only.

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:

Adding fields to zinject_record_t unexpectedly extended zfs_cmd_t,
preventing some things working properly with 2.3.1 userspace tools
against 2.3.0 kernel module.

This reverts commit fabdd50.

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Signed-off-by: Rob Norris <[email protected]>
@robn robn requested review from behlendorf and tonyhutter March 12, 2025 04:46
@vedranmiletic
Copy link

Not everybody upgrades immediately after a release comes out, so it might be worth it.

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

If it affects things outside of zinject itself (I hoped it won't) and we will to release 2.3.2 soon enough, then it makes sense.

@amotin amotin linked an issue Mar 17, 2025 that may be closed by this pull request
@tonyhutter tonyhutter merged commit 5f70370 into openzfs:zfs-2.3.2-staging Mar 24, 2025
31 of 37 checks passed
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.

zed process exits after "Processing events since eid=0"
4 participants