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
Date: Fri, 21 Jun 2019 14:08:01 -0700 [thread overview]
Message-ID: <20190621210800.GB167064@gmail.com> (raw)
In-Reply-To: <20190616160813.24464-3-chandan@linux.ibm.com>
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
next prev parent reply other threads:[~2019-06-21 21:08 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-16 16:08 [PATCH V3 0/7] Consolidate FS read I/O callbacks code Chandan Rajendra
2019-06-16 16:08 ` [PATCH V3 1/7] FS: Introduce read callbacks Chandan Rajendra
2019-06-21 20:03 ` Eric Biggers
2019-06-25 4:59 ` Chandan Rajendra
2019-06-16 16:08 ` [PATCH V3 2/7] Integrate read callbacks into Ext4 and F2FS Chandan Rajendra
2019-06-21 21:08 ` Eric Biggers [this message]
2019-06-25 6:05 ` Chandan Rajendra
2019-06-16 16:08 ` [PATCH V3 3/7] fscrypt: remove struct fscrypt_ctx Chandan Rajendra
2019-06-21 22:00 ` Eric Biggers
2019-06-16 16:08 ` [PATCH V3 4/7] fs/mpage.c: Integrate read callbacks Chandan Rajendra
2019-06-21 21:14 ` Eric Biggers
2019-06-25 6:21 ` Chandan Rajendra
2019-06-16 16:08 ` [PATCH V3 5/7] ext4: Wire up ext4_readpage[s] to use mpage_readpage[s] Chandan Rajendra
2019-06-16 16:08 ` [PATCH V3 6/7] Add decryption support for sub-pagesized blocks Chandan Rajendra
2019-06-21 21:29 ` Eric Biggers
2019-06-25 6:22 ` Chandan Rajendra
2019-06-16 16:08 ` [PATCH V3 7/7] ext4: Enable encryption for subpage-sized blocks Chandan Rajendra
2019-06-21 22:15 ` [PATCH V3 0/7] Consolidate FS read I/O callbacks code Eric Biggers
2019-06-25 6:24 ` Chandan Rajendra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190621210800.GB167064@gmail.com \
--to=ebiggers@kernel.org \
--cc=adilger.kernel@dilger.ca \
--cc=chandan@linux.ibm.com \
--cc=hch@infradead.org \
--cc=jaegeuk@kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=yuchao0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).