Daniel Vacek <[email protected]> wrote: > From: Josef Bacik <[email protected]> > > We're going to need these to update the file extent items once the > writes are complete. Add them and add the pieces necessary to assign > them and free everything. >
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 61cd57301840de69a3f6573374d41ad4b4bcf348 Author: Josef Bacik <[email protected]> btrfs: add fscrypt_info and encryption_type to ordered_extent This commit adds encryption-related fields (encryption_type and fscrypt_info) to struct btrfs_ordered_extent to support fscrypt integration in btrfs. > diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c > index 5df02c707aee6..7a5701937184e 100644 > --- a/fs/btrfs/ordered-data.c > +++ b/fs/btrfs/ordered-data.c > @@ -192,6 +192,7 @@ static struct btrfs_ordered_extent *alloc_ordered_extent( > } > entry->inode = inode; > entry->compress_type = compress_type; > + entry->encryption_type = BTRFS_ENCRYPTION_NONE; > entry->truncated_len = (u64)-1; > entry->qgroup_rsv = qgroup_rsv; This isn't a bug, but the fscrypt_info field isn't explicitly initialized to NULL here. While kmem_cache_zalloc() at line 175 does zero the memory, all the other important fields in this function are explicitly initialized (compress_type, encryption_type, truncated_len, qgroup_rsv, refs). Would adding an explicit 'entry->fscrypt_info = NULL;' improve consistency? > entry->flags = flags; > @@ -630,6 +631,7 @@ void btrfs_put_ordered_extent(struct btrfs_ordered_extent > *entry) > btrfs_add_delayed_iput(entry->inode); > list_for_each_entry_safe(sum, tmp, &entry->list, list) > kvfree(sum); > + fscrypt_put_extent_info(entry->fscrypt_info); > kmem_cache_free(btrfs_ordered_extent_cache, entry); > } > } > diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h > index 1e6b0b182b295..ce19198e7f844 100644 > --- a/fs/btrfs/ordered-data.h > +++ b/fs/btrfs/ordered-data.h > @@ -124,6 +124,9 @@ struct btrfs_ordered_extent { > /* compression algorithm */ > int compress_type; > > + /* encryption mode */ > + u8 encryption_type; > + This isn't a bug, but placing the u8 encryption_type field between two int fields creates 3 bytes of padding. This is minor, but in a filesystem with many ordered extents, it could add up. The current placement does have the advantage of grouping encryption-related fields together logically, though. > /* Qgroup reserved space */ > int qgroup_rsv; > > @@ -133,6 +136,9 @@ struct btrfs_ordered_extent { > /* the inode we belong to */ > struct btrfs_inode *inode; > > + /* the fscrypt_info for this extent, if necessary */ > + struct fscrypt_extent_info *fscrypt_info; > + > /* list of checksums for insertion when the extent io is done */ > struct list_head list; How does btrfs_split_ordered_extent() handle the new fscrypt_info field? Looking at that function in ordered-data.c, it calls alloc_ordered_extent() which initializes encryption_type to BTRFS_ENCRYPTION_NONE and fscrypt_info to NULL. If the original ordered extent has encryption_type set to BTRFS_ENCRYPTION_FSCRYPT with a non-NULL fscrypt_info pointer, the split creates an inconsistency where the two extents representing parts of the same encrypted data have different encryption metadata. The function already has an assertion preventing splits of compressed extents at line 1244: ASSERT(!(flags & (1U << BTRFS_ORDERED_COMPRESSED))) Should there be similar protection for encrypted extents, or if splits must be supported, should the function call fscrypt_get_extent_info() to properly handle the reference count and copy the encryption_type to the new extent?
