Re: [PATCH v2 15/17] btrfs: start tracking extent encryption context info

[Josef Bacik <josef@toxicpanda.com>]

On Sun, Jul 16, 2023 at 11:52:46PM -0400, Sweet Tea Dorminy wrote:
> This puts the long-preserved 1-byte encryption field to work, storing
> whether the extent is encrypted and the length of its fscrypt_context.
> Since there is no way for a btrfs inode to be encrypted right now, we
> set context length to be 0 in all locations. We then update all the
> locations which check the size of an extent item to expect the size to
> agree with the context length.
> 
> The 1-byte field packs the encryption type and context length together,
> to avoid a format change. Right now we don't anticipate very many
> encryption policies, so 2 bits should be ample; similarly right now we
> can't imagine a context larger than fscrypt's current contexts, which
> are 40 bytes, so 6 bits is ample to store the context's size; and
> therefore we can pack these together into the one-byte encryption field
> without touching other space reserved for future use.
> 

I don't like this design, I think it doesn't give us enough flexibility.

I think I'd rather use the whole u8 to have the encryption policy, and then the
first u32 of the extent context is the size.  So something like

struct btrfs_encryption_context {
	__le32 len;
	__u8 context[0];
};

struct btrfs_file_extent {
	struct btrfs_encryption_context fscrypt_context[0];
};

It takes up a little more space but is more flexible and enables us more
flexibility in the future.

> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/btrfs/accessors.h    | 31 +++++++++++++++++++++++++++++++
>  fs/btrfs/file-item.c    |  8 ++++++++
>  fs/btrfs/fscrypt.h      | 24 ++++++++++++++++++++++++
>  fs/btrfs/inode.c        | 26 ++++++++++++++++++++++----
>  fs/btrfs/tree-checker.c | 37 +++++++++++++++++++++++++++++--------
>  fs/btrfs/tree-log.c     |  3 +++
>  6 files changed, 117 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/accessors.h b/fs/btrfs/accessors.h
> index ceadfc5d6c66..fbee45b9d9c2 100644
> --- a/fs/btrfs/accessors.h
> +++ b/fs/btrfs/accessors.h
> @@ -3,6 +3,8 @@
>  #ifndef BTRFS_ACCESSORS_H
>  #define BTRFS_ACCESSORS_H
>  
> +#include "fscrypt.h"
> +

I don't want to pull other dependencies into accessors.h, I want to make syncing
*something* into btrfs-progs easy.

>  struct btrfs_map_token {
>  	struct extent_buffer *eb;
>  	char *kaddr;
> @@ -936,6 +938,16 @@ BTRFS_SETGET_STACK_FUNCS(stack_file_extent_disk_num_bytes,
>  			 struct btrfs_file_extent_item, disk_num_bytes, 64);
>  BTRFS_SETGET_STACK_FUNCS(stack_file_extent_compression,
>  			 struct btrfs_file_extent_item, compression, 8);
> +BTRFS_SETGET_STACK_FUNCS(stack_file_extent_encryption,
> +			 struct btrfs_file_extent_item, encryption, 8);
> +static inline u8 btrfs_stack_file_extent_encryption_ctxsize(
> +	struct btrfs_file_extent_item *e)
> +{
> +	u8 ctxsize;
> +
> +	btrfs_unpack_encryption(e->encryption, NULL, &ctxsize);
> +	return ctxsize;
> +}

This helper can go into fscrypt with the other related helpers.

>  
>  
>  BTRFS_SETGET_FUNCS(file_extent_type, struct btrfs_file_extent_item, type, 8);
> @@ -958,6 +970,25 @@ BTRFS_SETGET_FUNCS(file_extent_encryption, struct btrfs_file_extent_item,
>  BTRFS_SETGET_FUNCS(file_extent_other_encoding, struct btrfs_file_extent_item,
>  		   other_encoding, 16);
>  
> +static inline u8
> +btrfs_file_extent_encryption_ctxsize(const struct extent_buffer *eb,
> +				     struct btrfs_file_extent_item *e)
> +{
> +	u8 ctxsize;
> +
> +	btrfs_unpack_encryption(btrfs_file_extent_encryption(eb, e),
> +				NULL, &ctxsize);
> +	return ctxsize;
> +}

Same for this one.  Thanks,

Josef