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

[Daniel Vacek <neelx@suse.com>]

On Wed, 18 Feb 2026 at 16:50, Chris Mason <clm@meta.com> wrote:
> On 2/18/26 10:29 AM, Daniel Vacek wrote:
> > 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.
>
> The prompts try to flag any patch series that don't follow best
> practices around bisectability.  There isn't a ton of instruction about
> what those best practices are beyond if commit A makes bug and then
> commit B fixes the bug, flag it.
>
> This patch set is pretty unusual because you're trying to preserve
> authorship for a number of people, and so there are a lot of small
> judgement calls about when to change patches and when to fix them up
> later.  I sent claude's comments to help highlight places we want to
> decide things, but I don't think there's really a wrong decision.

I understand that. I was checking every commit to ensure it builds
correctly. But not really testing for bugs or regressions.

> >
> >>>       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;
> > +
>
> Seems mostly the same?  I'd suggest paholing things to find a good spot.

Hmm, that's what I did. There was a 4 bytes hole.
Nah, I see. I just moved it. It's not a big deal then. The structure's
size remains unchanged in either case. Still plugging a hole. Or am I
missing something?

> >      /* 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.
>
> It seems unlikely that we're really going to maintain bisectability for
> encryption being on in the middle of this patchset.  So this seems fine
> to me as long as the bug doesn't impact encryption being off.

Yeah, I think it should not. (Famous last words...)

Thanks.

--nX

> -chris
>