On Sun, 8 Feb 2026 at 16:20, Chris Mason <[email protected]> wrote:
> 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->fscrypt_info` is set in the very next patch "[21/43] btrfs:
plumb through setting the fscrypt_info for ordered extents". The
series is meant to be applied as a whole. It's just factored into
implementation steps.
> > 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.
This was actually changed from int (as Dave suggested, IIRC). But I
did not move it.
I guess we can do this:
--- a/fs/btrfs/ordered-data.h
+++ b/fs/btrfs/ordered-data.h
@@ -127,18 +127,18 @@ struct btrfs_ordered_extent {
/* flags (described above) */
unsigned long flags;
- /* compression algorithm */
- int compress_type;
-
- /* encryption mode */
- u8 encryption_type;
-
/* Qgroup reserved space */
int qgroup_rsv;
/* reference count */
refcount_t refs;
+ /* compression algorithm */
+ int compress_type;
+
+ /* encryption mode */
+ u8 encryption_type;
+
/* the inode we belong to */
struct btrfs_inode *inode;
> > /* 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
Ditto. This is changed in the next patch [21/43].
alloc_ordered_extent() correctly sets these fields.
Thanks.
--nX
> 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?
>
