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

[Chris Mason <clm@meta.com>]

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.

> 
>>>       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.

>      /* 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.

-chris