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

[Chris Mason <clm@meta.com>]

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

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