Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 21 Jun 2019 14:08:01 -0700
From:   Eric Biggers <ebiggers@kernel.org>
To:     Chandan Rajendra <chandan@linux.ibm.com>
Cc:     linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        linux-f2fs-devel@lists.sourceforge.net,
        linux-fscrypt@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, jaegeuk@kernel.org, yuchao0@huawei.com,
        hch@infradead.org
Subject: Re: [PATCH V3 2/7] Integrate read callbacks into Ext4 and F2FS
Message-ID: <20190621210800.GB167064@gmail.com>
References: <20190616160813.24464-1-chandan@linux.ibm.com>
 <20190616160813.24464-3-chandan@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190616160813.24464-3-chandan@linux.ibm.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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