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

Revert commit to fix LUKS write errors #16676

Closed
wants to merge 2 commits into from

Conversation

tonyhutter
Copy link
Contributor

Motivation and Context

Workaround ZFS on LUKS write errors on master branch.

Description

Revert bd7a02c as a workaround for #16631 and add a LUKS test case. This revert is more of a stop-gap for 2.3.x while we come up with a proper fix for #16631.

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

Checklist:

tonyhutter and others added 2 commits October 22, 2024 11:55
Add a LUKS sanity test to trigger:
openzfs#16631

Signed-off-by: Tony Hutter <[email protected]>
This reverts commit bd7a02c which
can trigger an unlikely existing bio alignment issue on Linux.
This change is good, but the underlying issue it exposes needs to
be resolved before this can be re-applied.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#15533
Issue openzfs#16631
@amotin
Copy link
Member

amotin commented Oct 22, 2024

Lets not hurry with this. If needed, we can use smaller workaround patch from #16631 (comment) .

@robn
Copy link
Member

robn commented Oct 22, 2024

I'm not opposed to reverting (we haven't had this for most of 2.2 so it's a known quantity at least). I would prefer @amotin's far smaller patch if we think it covers enough (sounds right to me right now).

I will note that this will likely silence the uptick of recent issues, but it's not going to work around all related issues with LUKS, which have been seen in places that didn't have the the allocator change this patch reverts (late 2.2, and 2.1), because 4K blocks split across memory pages are still possible (gang ABDs, LINEAR_PAGE scatterlists). That's ok, reverting to a known and smaller broken is sensible, I just wanted anyone with an interest in those older problem to not get too excited by this change (based on the PR name).

@tonyhutter
Copy link
Contributor Author

I'm putting this PR on hold in favor of the fix here:
#16631 (comment)

@robn
Copy link
Member

robn commented Oct 22, 2024

Thanks @tonyhutter 👑

@behlendorf behlendorf marked this pull request as draft October 22, 2024 22:55
Copy link
Contributor

@mcmilk mcmilk left a comment

Choose a reason for hiding this comment

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

The luks sanity check is really fine.

# STRATEGY:
# 1. Create a LUKS device
# 2. Make a pool with it
# 3. Write files to the pool
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a tab.

@tonyhutter
Copy link
Contributor Author

I'm closing this in favor of the workaround PR (#16678) and the standalone test case PR (#16681).

@tonyhutter tonyhutter closed this Oct 23, 2024
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.

5 participants