On Wed, 18 Feb 2026 at 16:50, Chris Mason <[email protected]> wrote:
> On 2/18/26 10:29 AM, Daniel Vacek wrote:
> > 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.
>
> 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
>
