search

koverstreet / bcachefs-tools

http://bcachefs.org
GNU General Public License v2.0
120 stars 89 forks source link

`check_for_key` before `ask_for_passphrase` #263

Closed tmuehlbacher closed 3 months ago

tmuehlbacher commented 4 months ago

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

koverstreet commented 4 months ago

What else were you going to do?

tmuehlbacher commented 4 months ago

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.

breavyn commented 4 months ago

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

https://github.com/koverstreet/bcachefs-tools/blob/59e04ddf4090867cf25d13eb9ab69fd33920e968/src/commands/cmd_mount.rs#L248

tmuehlbacher commented 4 months ago

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

JohnRTitor commented 4 months ago

@reedriley @codebam can you test this?

NOTE: NixOS users can easily test the fix using a overlay to fetch this patch, as shown in https://github.com/JohnRTitor/nix-conf/commit/b60df8a18feb8c9e6e4edc16fb62fe2a5ad0449b

codebam commented 4 months ago

Tested and working on NixOS with rc7

Edit: works with 6.9 as well

JohnRTitor commented 4 months ago

As I have stated in https://github.com/NixOS/nixpkgs/pull/310504#issuecomment-2110271723, 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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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

tmuehlbacher commented 3 months ago

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 commented 3 months ago

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.

koverstreet commented 3 months ago

This is merged

koverstreet commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Yup, I'll be around