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;
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
Organization: IBM
In-Reply-To: <20190430000027.GB251866@gmail.com>
References: <20190428043121.30925-1-chandan@linux.ibm.com> <20190428043121.30925-3-chandan@linux.ibm.com> <20190430000027.GB251866@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Message-Id: <1834737.zJtJvJ5dM2@localhost.localdomain>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

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