Daniel Vacek <[email protected]> wrote: > From: Josef Bacik <[email protected]> > > The RAID5/6 code will re-arrange bios and submit them through a > different mechanism. This is problematic with inline encryption as we > have to get the bio and csum it after it's been encrypted, and the > radi5/6 bio's don't have the btrfs_bio embedded, so we have no way to > get the csums put on disk. > > This isn't an unsolvable problem, but would require a bit of reworking. > Since we discourage users from using this code currently simply don't > allow encryption on RAID5/6 setups. If there's sufficient demand in the > future we can add the support for this. >
Hi everyone, I'm running the btrfs for-next branch, along with the encryption patch set through my AI review prompts: https://github.com/masoncl/review-prompts I'm working on some btrfs specific instructions, but hopefully these reviews are already useful. I've kept the headers to make it easier to double check that I'm actually replying to the right email, but please let me know if any parts of this are incorrect: commit 6a39a21e95e116ad02c6ce9f832838acc111692f Author: Josef Bacik <[email protected]> btrfs: disable encryption on RAID5/6 The RAID5/6 code will re-arrange bios and submit them through a different mechanism to handle parity. This is problematic with inline encryption as we have to get the bio and csum it after it's been encrypted, and the RAID5/6 bios don't have the btrfs_bio embedded, so there's no way to get the csums put on disk. > commit 6a39a21e95e116ad02c6ce9f832838acc111692f > Author: Josef Bacik <[email protected]> > Date: Fri Feb 6 19:23:14 2026 +0100 > > btrfs: disable encryption on RAID5/6 > > The RAID5/6 code will re-arrange bios and submit them through a > different mechanism. This is problematic with inline encryption as we > have to get the bio and csum it after it's been encrypted, and the > radi5/6 bio's don't have the btrfs_bio embedded, so we have no way to ^^^^ This isn't a bug, but there's a typo: 'radi5/6' should be 'raid5/6'. [ ... ] > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index a8adf99ad0a81..1bade8fea16e8 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -5198,6 +5198,10 @@ long btrfs_ioctl(struct file *file, unsigned int > return -EOPNOTSUPP; > if (sb_rdonly(fs_info->sb)) > return -EROFS; > + if (btrfs_fs_incompat(fs_info, RAID56)) { > + btrfs_warn(fs_info, "can't enable encryption with > RAID5/6"); > + return -EINVAL; > + } This check prevents enabling encryption when RAID56 is already active, but does NOT prevent converting to RAID5/6 after encryption is enabled. The check_raid56_incompat_flag() function in fs/btrfs/volumes.c:5558 sets the RAID56 flag during balance operations without checking if ENCRYPT is already set. This allows users to: 1. Enable encryption via FS_IOC_SET_ENCRYPTION_POLICY (sets ENCRYPT at line 5210 in this function) 2. Later run btrfs balance to convert to RAID5/6 (sets RAID56 flag at volumes.c:5558) 3. End up with both ENCRYPT and RAID56 flags set The commit message states "simply don't allow encryption on RAID5/6 setups" which implies preventing the incompatible state entirely, not just one entry path. The volumes.c code path needs a corresponding check: static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type) { if (!(type & BTRFS_BLOCK_GROUP_RAID56_MASK)) return; // MISSING: check for ENCRYPT flag here btrfs_set_fs_incompat(info, RAID56); } Without this check, data corruption can occur via the RAID5/6 bio path that cannot handle encrypted data properly, as explained in the commit message. > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 4a2887147ead4..aefcbe56e85ad 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -734,6 +734,12 @@ bool btrfs_check_options(const struct btrfs_fs_info > *info, > if (btrfs_check_mountopts_zoned(info, mount_opt)) > ret = false; > > + if (btrfs_fs_incompat(info, RAID56) && > + btrfs_raw_test_opt(*mount_opt, TEST_DUMMY_ENCRYPTION)) { > + btrfs_err(info, "cannot use test_dummy_encryption with > RAID5/6"); > + ret = false; > + } > + Similar to the ioctl.c issue, this prevents mounting with test_dummy_encryption when RAID56 is already active, but does NOT prevent converting to RAID5/6 after mounting with test_dummy_encryption. The check_raid56_incompat_flag() function in fs/btrfs/volumes.c:5558 sets the RAID56 flag during balance operations without checking if test_dummy_encryption is in use. Since test_dummy_encryption cannot be changed after mount (enforced by btrfs_check_test_dummy_encryption at line 1547-1560), a user can: 1. Mount filesystem with -o test_dummy_encryption 2. Later run btrfs balance to convert to RAID5/6 (sets RAID56 flag) 3. End up with both test_dummy_encryption active and RAID56 flag set While test_dummy_encryption is primarily a testing/development feature, the patch explicitly protects against this combination, indicating it's an important incompatibility. The volumes.c code path needs a check for the TEST_DUMMY_ENCRYPTION mount option: static void check_raid56_incompat_flag(struct btrfs_fs_info *info, u64 type) { if (!(type & BTRFS_BLOCK_GROUP_RAID56_MASK)) return; // MISSING: check for TEST_DUMMY_ENCRYPTION mount option btrfs_set_fs_incompat(info, RAID56); } Does this allow the same encrypted bio / RAID5/6 bio incompatibility described in the commit message?
