Hi Chandan,
On Sun, Jun 16, 2019 at 09:38:08PM +0530, Chandan Rajendra wrote:
> This commit gets Ext4 and F2FS to make use of read callbacks API to
> perform decryption of file data read from the disk.
> ---
> fs/crypto/bio.c | 30 +--------
> fs/crypto/crypto.c | 1 +
> fs/crypto/fscrypt_private.h | 3 +
> fs/ext4/readpage.c | 29 +++------
> fs/f2fs/data.c | 124 +++++++-----------------------------
> fs/f2fs/super.c | 9 +--
> fs/read_callbacks.c | 1 -
> include/linux/fscrypt.h | 18 ------
> 8 files changed, 40 insertions(+), 175 deletions(-)
>
This patch changes many different components. It would be much easier to
review, and might get more attention from the other ext4 and f2fs developers, if
it were split into 3 patches:
a. Convert ext4 to use read_callbacks.
b. Convert f2fs to use read_callbacks.
c. Remove the functions from fs/crypto/ that became unused as a result of
patches (a) and (b). (Actually, this part probably should be merged with the
patch that removes the fscrypt_ctx, and the patch renamed to something like
"fscrypt: remove decryption I/O path helpers")
Any reason why this wouldn't work? AFAICS, you couldn't do it only because you
made this patch change fscrypt_enqueue_decrypt_work() to be responsible for
initializing the work function. But as per my comments on patch 1, I don't
think we should do that, since it would make much more sense to put the work
function in read_callbacks.c.
However, since you're converting ext4 to use mpage_readpages() anyway, I don't
think we should bother with the intermediate change to ext4_mpage_readpages().
It's useless, and that intermediate state of the ext4 code inevitably won't get
tested very well. So perhaps order the whole series as:
- fs: introduce read_callbacks
- fs/mpage.c: add decryption support via read_callbacks
- fs/buffer.c: add decryption support via read_callbacks
- f2fs: convert to use read_callbacks
- ext4: convert to use mpage_readpages[s]
- ext4: support encryption with subpage-sized blocks
- fscrypt: remove decryption I/O path helpers
That order would also give the flexibility to possibly apply the fs/ changes
first, without having to update both ext4 and f2fs simultaneously with them.
> @@ -557,8 +511,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> struct bio *bio;
> - struct bio_post_read_ctx *ctx;
> - unsigned int post_read_steps = 0;
> + int ret;
Nit: 'err' rather than 'ret', since this is 0 or a -errno value.
> -int __init f2fs_init_post_read_processing(void)
> -{
> - bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, 0);
> - if (!bio_post_read_ctx_cache)
> - goto fail;
> - bio_post_read_ctx_pool =
> - mempool_create_slab_pool(NUM_PREALLOC_POST_READ_CTXS,
> - bio_post_read_ctx_cache);
> - if (!bio_post_read_ctx_pool)
> - goto fail_free_cache;
> - return 0;
> -
> -fail_free_cache:
> - kmem_cache_destroy(bio_post_read_ctx_cache);
> -fail:
> - return -ENOMEM;
> -}
> -
> -void __exit f2fs_destroy_post_read_processing(void)
> -{
> - mempool_destroy(bio_post_read_ctx_pool);
> - kmem_cache_destroy(bio_post_read_ctx_cache);
> -}
Need to remove the declarations of these functions from fs/f2fs/f2fs.h to.
> diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> index a4196e3de05f..4b7fc2a349cd 100644
> --- a/fs/read_callbacks.c
> +++ b/fs/read_callbacks.c
> @@ -76,7 +76,6 @@ void read_callbacks(struct read_callbacks_ctx *ctx)
> switch (++ctx->cur_step) {
> case STEP_DECRYPT:
> if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
> - INIT_WORK(&ctx->work, fscrypt_decrypt_work);
> fscrypt_enqueue_decrypt_work(&ctx->work);
> return;
> }
Again, I think the work initialization should remain here as:
INIT_WORK(&ctx->work, decrypt_work);
rather than moving it to fs/crypto/.
Thanks!
- Eric
