Re: [PATCH v6 20/43] btrfs: add fscrypt_info and encryption_type to ordered_extent

[Daniel Vacek <neelx@suse.com>]

On Sun, 8 Feb 2026 at 16:20, Chris Mason <clm@meta.com> wrote:
> Daniel Vacek <neelx@suse.com> wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> >
> > 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 <josef@toxicpanda.com>
>
> 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?
>