On Tue, Dec 05, 2023 at 05:48:09PM -0500, Josef Bacik wrote:
> > Also, I think that for now we should keep things simple by doing the following
> > instead of fscrypt_generate_dun():
> >
> > u64 dun[BLK_CRYPTO_DUN_ARRAY_SIZE] = { first_lblk };
> >
> > Note that these two changes would eliminate the need for the inode parameter to
> > the function.
> >
> > Please also make sure to update the function comment. Note that the first line
> > gives the function name incorrectly.
> >
>
> First off thanks for the review, I'm currently going through it one by one, so
> I've come to this bit and I've got a question.
>
> The DUN has to be le64, so this strictly isn't ok. So would you be ok with
>
> __le64 first_lblk_le = le64_to_cpu(first_lblk;
> u64 dun [BLK_CRYPTO_DUN_ARRAY_SIZE] = { first_lblk_le };
>
> or do you want something different? Additionally fscrypt_generate_dun() also
> takes into account the data units per block bits, which as I'm typing this out
> I'm realizing doesn't really matter for us even if we have different sectorsize
> per pagesize. I guess I'll put a big comment about why we're not using that in
> there to make sure future Josef isn't confused.
The blk-crypto DUN is CPU endian, as indicated by the type (array of u64). Note
that fscrypt_generate_dun() converts u64 => __le64 => u64. (And then
blk-crypto-fallback converts u64 => __le64.) Getting rid of that round trip is
nice, which is why I'm recommending it. The only reason that
fscrypt_generate_dun() does the round trip is because it allows it to reuse
fscrypt_generate_iv(), and I wanted to be 100% sure that all of the IV methods
were implemented the same as in FS layer encryption. With the extent-based
encryption we're just supporting one method, so things are simpler.
Yeah, it should be 'first_lblk << ci_data_units_per_block_bits'.
That would just be for future-proofing, since btrfs isn't setting
supports_subblock_data_units anyway.
> > > +/**
> > > + * fscrypt_extent_context_size() - Return the size of the on-disk extent context
> > > + * @inode: the inode this extent belongs to.
> > > + *
> > > + * Return the size of the extent context for this inode. Since there is only
> > > + * one extent context version currently this is just the size of the extent
> > > + * context if the inode is encrypted.
> > > + */
> > > +size_t fscrypt_extent_context_size(struct inode *inode)
> > > +{
> > > + if (!IS_ENCRYPTED(inode))
> > > + return 0;
> > > +
> > > + return sizeof(struct fscrypt_extent_context);
> > > +}
> > > +EXPORT_SYMBOL_GPL(fscrypt_extent_context_size);
> >
> > Huh, shouldn't the filesystem use the size from fscrypt_set_extent_context() (or
> > fscrypt_context_for_new_extent() as I recommend calling it)? Why is the above
> > function necessary?
>
> This is because we need to know how big the context is ahead of time before
> searching down the b-tree to insert the file extent item, and we have to add
> this size to the file extent item ahead of time. The disconnect between when we
> need to know how big it'll be and when we actually generate the context to save
> on disk is the reason for there being two helpers.
>
> The main reason for this is code separation. I would have to have the context
> on stack for the !fscrypt case if I were to only have one helper, because I
> would need it there to generate the extent context to save on disk to pass it
> around to all the things that add file extents, and I could use the length from
> that. Not a huge deal, but this way is a little cleaner. You can see how I've
> used the two helpers in later patches, but the general flow is
>
> total_size = sizeof(our file extent);
> total_size += fscrypt_extent_context_size(inode);
>
> btrfs_insert_empty_item(path, total_size);
>
> btrfs_fscrypt_save_extent_info(path, inode);
>
> and then in btrfs_fscrypt_save_extent_info() we do
>
> u8 ctx[BTRFS_MAX_EXTENT_CTX_SIZE];
>
> size = fscrypt_set_extent_context(inode, ctx);
>
> Now the above case does seem like we could just uconditionally do something like
>
> u8 ctx[BTRFS_MAX_EXTENT_CTX_SIZE];
>
> total_size = sizeof(our file extent);
> ctx_size = fscrypt_set_extent_context(inode, ctx);
> if (ctx_size < 0)
> goto error;
> total_size += ctx_size;
> btrfs_insert_empty_item(path, total_size);
> copy_ctx_into_file_extent(path, ctx);
>
> But there's some other file extent manipulation cases that aren't this clear
> cut, so I'd have to pass this ctx around to be copied in. And in the !encrypted
> part we're paying a higher stack memory cost.
>
> If you still don't like this I can rework it, but I wanted to explain my
> reasoning before I went changing a bunch of stuff to make the new paradigm fit.
>
> The rest of the comments make sense, I'll fix everything up. Thanks,
Maybe fscrypt_set_extent_context() (or fscrypt_context_for_new_extent()?) should
return the size if it's passed a NULL pointer? It just seems error-prone have
the size of the thing being generated be returned by a different function.
- Eric
