Re: [PATCH V3 2/7] Integrate read callbacks into Ext4 and F2FS

[Chandan Rajendra <chandan@linux.ibm.com>]

On Saturday, June 22, 2019 2:38:01 AM IST Eric Biggers wrote:
> 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.

Yes, you are right about that. I will make the changes suggested by you.

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


-- 
chandan