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

check_for_key before ask_for_passphrase #263

Closed

Conversation

tmuehlbacher
Copy link

let's always first check if there is already a key in the keyring available before we try to get the key from some more involved means.

Fixes: #261

@tmuehlbacher tmuehlbacher force-pushed the fix-encrypted-unlock branch from 001e3bd to 38b0cb7 Compare May 9, 2024 22:29
@koverstreet
Copy link
Owner

What else were you going to do?

@tmuehlbacher
Copy link
Author

Mostly just wanted to test it first. Works for me though (applied on tag v1.7.0).

Also thought about maybe doing some refactoring but I'll just leave it for now.

@tmuehlbacher tmuehlbacher marked this pull request as ready for review May 9, 2024 23:37
@breavyn
Copy link

breavyn commented May 10, 2024

I would suggest moving the check for an existing key to the very beginning of the process.

// First by password_file, if available

let's always first check if there is already a key in the keyring
available before we try to get the key from some more involved means.

Fixes: koverstreet#261
Signed-off-by: Thomas Mühlbacher <[email protected]>
@tmuehlbacher tmuehlbacher force-pushed the fix-encrypted-unlock branch from 38b0cb7 to f76ad4d Compare May 10, 2024 22:01
causes mounting encrypted devices to become stuck in a busy loop.
@tmuehlbacher
Copy link
Author

Ok, now mounting encrypted devices should fully work again on master. 😁

@JohnRTitor
Copy link

JohnRTitor commented May 12, 2024

@reedriley @codebam can you test this?

NOTE: NixOS users can easily test the fix using a overlay to fetch this patch, as shown in JohnRTitor/nix-conf@b60df8a

@codebam
Copy link

codebam commented May 12, 2024

Tested and working on NixOS with rc7

Edit: works with 6.9 as well

@JohnRTitor
Copy link

JohnRTitor commented May 14, 2024

As I have stated in NixOS/nixpkgs#310504 (comment), the recent update to 6.9 stable kernel broke this on unencrypted systems, including in my own system.
I had to boot into a NixOS iso, run fsck on my bcachefs partition and then rebuild from there using this patch.

@codebam have already confirmed above that this patch works on 6.9 kernel for encrypted bcachefs partitions. and I confirm now too that this patch works on my system as well.

@koverstreet could you please consider checking and merging this as soon as possible?

@reedriley
Copy link

What symptoms did your breakage have?

Updating to 6.9 (rc6 in my case) took 2.5 hours to boot the first time for me; because the 1.7 disk format changes required running check_allocs and I had a large multi-disk setup.

@JohnRTitor
Copy link

It was taking a lot of times to boot then it failed, this is the image I was able to capture.
IMG_20240513_235714

I couldn't even boot into previous generations of NixOS, because of this. So booted into a Nix ISO, and manually ran fsck from master branch bcachefs, with the patch enabled.

I then tested with a 6.8.9 kernel, it downgraded the disk format but no boot issues were encountered. Then, upgraded to 6.9 again, no issues.

I'd assume it didn't break because the patch was working.

@tmuehlbacher
Copy link
Author

I had assumed that the bcachefs-tools in nixos-unstable should be fine to use currently thanks to @reedriley including my initial patch (thanks btw!). The second commit in this PR is only for a problem that affects master.

I initially had issues with 6.9 that required fscks until including 6.9-rc6 but after that it worked on my systems. fs_usage_nr_inodes_wrong can still be seen in the errors log using bcachefs show-super.

@JohnRTitor can you check if you also have an error logged when you use show-super?

@JohnRTitor
Copy link

I do see these errors.


errors (size 88):
inode_multiple_links_but_nlink_0            379             Tue May 14 17:59:30 2024
inode_wrong_backpointer                     379             Tue May 14 17:59:30 2024
inode_wrong_nlink                           5924            Tue May 14 18:02:36 2024
inode_unreachable                           23385           Mon Mar 25 15:35:37 2024
dirent_to_missing_inode                     17081           Tue May 14 17:57:46 2024

JohnRTitor added a commit to JohnRTitor/nixpkgs that referenced this pull request May 22, 2024
Moved temporarily to unstable to fix NixOS#313350

Also vendor the updated patch for NixOS#309388
from koverstreet/bcachefs-tools#263
@JohnRTitor
Copy link

@koverstreet @tmuehlbacher is this ready to be merged or blocked on something?

@tmuehlbacher
Copy link
Author

It's good to merge as far as I am concerned. :)
Maybe Kent can take a look at those problems you've reported in the comments here but I don't think that should block this PR.

@JohnRTitor
Copy link

Well, the errors logged are not fatal, and does not affect this PR.

The patch you provided still works as tested by me and others.

alyssais pushed a commit to NixOS/nixpkgs that referenced this pull request May 26, 2024
Moved temporarily to unstable to fix #313350

Also vendor the updated patch for #309388
from koverstreet/bcachefs-tools#263
github-actions bot pushed a commit to NixOS/nixpkgs that referenced this pull request May 26, 2024
Moved temporarily to unstable to fix #313350

Also vendor the updated patch for #309388
from koverstreet/bcachefs-tools#263

(cherry picked from commit 1037866)
@koverstreet
Copy link
Owner

This is merged

@tmuehlbacher tmuehlbacher deleted the fix-encrypted-unlock branch May 26, 2024 17:46
@koverstreet
Copy link
Owner

Can you join the IRC channel? I'll need a metadata dump to debug this, but if you can get me that it should be straightforward to see what's going on.

I'm also debugging some issues in noradtux's filesystem right now that might also be related - dirent to missing inode

@JohnRTitor
Copy link

You mean me?
Unfortunately I can't right now, due to a tropical storm heading to my city. Will try tomorrow, 11 AM EST sounds good?

@koverstreet
Copy link
Owner

Yup, I'll be around

Sobte pushed a commit to Sobte/nixpkgs that referenced this pull request May 28, 2024
Moved temporarily to unstable to fix NixOS#313350

Also vendor the updated patch for NixOS#309388
from koverstreet/bcachefs-tools#263
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.

booting with bcachefs compression on is broken
6 participants