From: Chandan Rajendra <chandan@linux.ibm.com>
To: Eric Biggers <ebiggers@kernel.org>
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 V2 02/13] Consolidate "read callbacks" into a new file
Date: Wed, 01 May 2019 18:00:45 +0530 [thread overview]
Message-ID: <1834737.zJtJvJ5dM2@localhost.localdomain> (raw)
In-Reply-To: <20190430000027.GB251866@gmail.com>
On Tuesday, April 30, 2019 5:30:28 AM IST Eric Biggers wrote:
> Hi Chandan,
>
> On Sun, Apr 28, 2019 at 10:01:10AM +0530, Chandan Rajendra wrote:
> > The "read callbacks" code is used by both Ext4 and F2FS. Hence to
> > remove duplicity, this commit moves the code into
> > include/linux/read_callbacks.h and fs/read_callbacks.c.
> >
> > The corresponding decrypt and verity "work" functions have been moved
> > inside fscrypt and fsverity sources. With these in place, the read
> > callbacks code now has to just invoke enqueue functions provided by
> > fscrypt and fsverity.
> >
> > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
> > ---
> > fs/Kconfig | 4 +
> > fs/Makefile | 4 +
> > fs/crypto/Kconfig | 1 +
> > fs/crypto/bio.c | 23 ++---
> > fs/crypto/crypto.c | 17 +--
> > fs/crypto/fscrypt_private.h | 3 +
> > fs/ext4/ext4.h | 2 -
> > fs/ext4/readpage.c | 183 +++++----------------------------
> > fs/ext4/super.c | 9 +-
> > fs/f2fs/data.c | 148 ++++----------------------
> > fs/f2fs/super.c | 9 +-
> > fs/read_callbacks.c | 136 ++++++++++++++++++++++++
> > fs/verity/Kconfig | 1 +
> > fs/verity/verify.c | 12 +++
> > include/linux/fscrypt.h | 20 +---
> > include/linux/read_callbacks.h | 21 ++++
> > 16 files changed, 251 insertions(+), 342 deletions(-)
> > create mode 100644 fs/read_callbacks.c
> > create mode 100644 include/linux/read_callbacks.h
> >
>
> For easier review, can you split this into multiple patches? Ideally the ext4
> and f2fs patches would be separate, but if that's truly not possible due to
> interdependencies it seems you could at least do:
>
> 1. Introduce the read_callbacks.
> 2. Convert encryption to use the read_callbacks.
> 3. Remove union from struct fscrypt_context.
>
> Also: just FYI, fs-verity isn't upstream yet, and in the past few months I
> haven't had much time to work on it. So you might consider arranging your
> series so that initially just fscrypt is supported. That will be useful on its
> own, for block_size < PAGE_SIZE support. Then fsverity can be added later.
>
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index 97f9eb8df713..03084f2dbeaf 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -308,6 +308,10 @@ config NFS_COMMON
> > depends on NFSD || NFS_FS || LOCKD
> > default y
> >
> > +config FS_READ_CALLBACKS
> > + bool
> > + default n
>
> 'default n' is unnecesary, since 'n' is already the default.
>
> > +
> > source "net/sunrpc/Kconfig"
> > source "fs/ceph/Kconfig"
> > source "fs/cifs/Kconfig"
> > diff --git a/fs/Makefile b/fs/Makefile
> > index 9dd2186e74b5..e0c0fce8cf40 100644
> > --- a/fs/Makefile
> > +++ b/fs/Makefile
> > @@ -21,6 +21,10 @@ else
> > obj-y += no-block.o
> > endif
> >
> > +ifeq ($(CONFIG_FS_READ_CALLBACKS),y)
> > +obj-y += read_callbacks.o
> > +endif
> > +
>
> This can be simplified to:
>
> obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
>
> > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> > new file mode 100644
> > index 000000000000..b6d5b95e67d7
> > --- /dev/null
> > +++ b/fs/read_callbacks.c
> > @@ -0,0 +1,136 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file tracks the state machine that needs to be executed after reading
> > + * data from files that are encrypted and/or have verity metadata associated
> > + * with them.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/mm.h>
> > +#include <linux/pagemap.h>
> > +#include <linux/bio.h>
> > +#include <linux/fscrypt.h>
> > +#include <linux/fsverity.h>
> > +#include <linux/read_callbacks.h>
> > +
> > +#define NUM_PREALLOC_POST_READ_CTXS 128
> > +
> > +static struct kmem_cache *read_callbacks_ctx_cache;
> > +static mempool_t *read_callbacks_ctx_pool;
> > +
> > +/* Read callback state machine steps */
> > +enum read_callbacks_step {
> > + STEP_INITIAL = 0,
> > + STEP_DECRYPT,
> > + STEP_VERITY,
> > +};
> > +
> > +void end_read_callbacks(struct bio *bio)
> > +{
> > + struct page *page;
> > + struct bio_vec *bv;
> > + int i;
> > + struct bvec_iter_all iter_all;
> > +
> > + bio_for_each_segment_all(bv, bio, i, iter_all) {
> > + page = bv->bv_page;
> > +
> > + BUG_ON(bio->bi_status);
> > +
> > + if (!PageError(page))
> > + SetPageUptodate(page);
> > +
> > + unlock_page(page);
> > + }
> > + if (bio->bi_private)
> > + put_read_callbacks_ctx(bio->bi_private);
> > + bio_put(bio);
> > +}
> > +EXPORT_SYMBOL(end_read_callbacks);
>
> end_read_callbacks() is only called by read_callbacks() just below, so it should
> be 'static'.
>
> > +
> > +struct read_callbacks_ctx *get_read_callbacks_ctx(struct inode *inode,
> > + struct bio *bio,
> > + pgoff_t index)
> > +{
> > + unsigned int read_callbacks_steps = 0;
>
> Rename 'read_callbacks_steps' => 'enabled_steps', since it's clear from context.
>
> > + struct read_callbacks_ctx *ctx = NULL;
> > +
> > + if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode))
> > + read_callbacks_steps |= 1 << STEP_DECRYPT;
> > +#ifdef CONFIG_FS_VERITY
> > + if (inode->i_verity_info != NULL &&
> > + (index < ((i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT)))
> > + read_callbacks_steps |= 1 << STEP_VERITY;
> > +#endif
>
> To avoid the #ifdef, this should probably be made a function in fsverity.h.
>
> > + if (read_callbacks_steps) {
> > + ctx = mempool_alloc(read_callbacks_ctx_pool, GFP_NOFS);
> > + if (!ctx)
> > + return ERR_PTR(-ENOMEM);
> > + ctx->bio = bio;
> > + ctx->inode = inode;
> > + ctx->enabled_steps = read_callbacks_steps;
> > + ctx->cur_step = STEP_INITIAL;
> > + bio->bi_private = ctx;
> > + }
> > + return ctx;
> > +}
> > +EXPORT_SYMBOL(get_read_callbacks_ctx);
>
> The callers don't actually use the returned read_callbacks_ctx. Instead, they
> rely on this function storing it in ->bi_private. So, this function should just
> return an error code, and it should be renamed. Perhaps:
>
> int read_callbacks_setup_bio(struct inode *inode, struct bio *bio,
> pgoff_t first_pgoff);
>
> Please rename 'index' to 'first_pgoff' to make it clearer what it is, given that
> a bio can contain many pages.
>
> Please add kerneldoc for this function.
>
I will implement the changes suggested above.
--
chandan
next prev parent reply other threads:[~2019-05-01 14:24 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-28 4:31 [PATCH V2 00/13] Consolidate FS read I/O callbacks code Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 01/13] ext4: Clear BH_Uptodate flag on decryption error Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 02/13] Consolidate "read callbacks" into a new file Chandan Rajendra
2019-04-30 0:00 ` Eric Biggers
2019-05-01 12:30 ` Chandan Rajendra [this message]
2019-04-30 1:37 ` Chao Yu
2019-05-01 12:31 ` Chandan Rajendra
2019-04-30 18:05 ` Eric Biggers
2019-05-01 12:32 ` Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 03/13] fsverity: Add call back to decide if verity check has to be performed Chandan Rajendra
2019-04-30 21:10 ` Jeremy Sowden
2019-05-01 12:33 ` Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 04/13] fsverity: Add call back to determine readpage limit Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 05/13] fs/mpage.c: Integrate read callbacks Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 06/13] ext4: Wire up ext4_readpage[s] to use mpage_readpage[s] Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 07/13] Add decryption support for sub-pagesized blocks Chandan Rajendra
2019-04-30 0:38 ` Eric Biggers
2019-05-01 13:40 ` Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 08/13] ext4: Decrypt all boundary blocks when doing buffered write Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 09/13] ext4: Decrypt the block that needs to be partially zeroed Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 10/13] fscrypt_encrypt_page: Loop across all blocks mapped by a page range Chandan Rajendra
2019-04-30 17:11 ` Eric Biggers
2019-04-30 23:08 ` [f2fs-dev] " Eric Biggers
2019-05-01 14:49 ` Chandan Rajendra
2019-05-01 22:29 ` Eric Biggers
2019-05-02 5:52 ` Chandan Rajendra
2019-05-02 18:16 ` Eric Biggers
2019-04-28 4:31 ` [PATCH V2 11/13] ext4: Compute logical block and the page range to be encrypted Chandan Rajendra
2019-04-30 17:01 ` Eric Biggers
2019-05-01 14:11 ` Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 12/13] fscrypt_zeroout_range: Encrypt all zeroed out blocks of a page Chandan Rajendra
2019-04-30 16:51 ` Eric Biggers
2019-05-01 14:22 ` Chandan Rajendra
2019-04-28 4:31 ` [PATCH V2 13/13] ext4: Enable encryption for subpage-sized blocks Chandan Rajendra
2019-04-30 0:27 ` [PATCH V2 00/13] Consolidate FS read I/O callbacks code Matthew Wilcox
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=1834737.zJtJvJ5dM2@localhost.localdomain \
--to=chandan@linux.ibm.com \
--cc=adilger.kernel@dilger.ca \
--cc=ebiggers@kernel.org \
--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).