Message-Id: <3763974.FiHZtjIrND@localhost.localdomain>
Date:   Tue, 25 Jun 2019 10:29:42 +0530
From:   Chandan Rajendra <chandan@...ux.ibm.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-fscrypt@...r.kernel.org, tytso@....edu,
        adilger.kernel@...ger.ca, jaegeuk@...nel.org, yuchao0@...wei.com,
        hch@...radead.org
Subject: Re: [PATCH V3 1/7] FS: Introduce read callbacks

On Saturday, June 22, 2019 1:33:57 AM IST Eric Biggers wrote:
> Hi Chandan,
> 
> On Sun, Jun 16, 2019 at 09:38:07PM +0530, Chandan Rajendra wrote:
> > Read callbacks implements a state machine to be executed after a
> > buffered read I/O is completed. They help in further processing the file
> > data read from the backing store. Currently, decryption is the only post
> > processing step to be supported.
> > 
> > The execution of the state machine is to be initiated by the endio
> > function associated with the read operation.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@...ux.ibm.com>
> > ---
> >  fs/Kconfig                     |   3 +
> >  fs/Makefile                    |   2 +
> >  fs/crypto/Kconfig              |   1 +
> >  fs/crypto/bio.c                |  11 +++
> >  fs/read_callbacks.c            | 174 +++++++++++++++++++++++++++++++++
> >  include/linux/fscrypt.h        |   5 +
> >  include/linux/read_callbacks.h |  38 +++++++
> >  7 files changed, 234 insertions(+)
> >  create mode 100644 fs/read_callbacks.c
> >  create mode 100644 include/linux/read_callbacks.h
> > 
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index f1046cf6ad85..d869859c88da 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -21,6 +21,9 @@ if BLOCK
> >  config FS_IOMAP
> >  	bool
> >  
> > +config FS_READ_CALLBACKS
> > +       bool
> 
> This should be intended with a tab, not spaces.
> 
> > +
> >  source "fs/ext2/Kconfig"
> >  source "fs/ext4/Kconfig"
> >  source "fs/jbd2/Kconfig"
> > diff --git a/fs/Makefile b/fs/Makefile
> > index c9aea23aba56..a1a06f0db5c1 100644
> > --- a/fs/Makefile
> > +++ b/fs/Makefile
> > @@ -21,6 +21,8 @@ else
> >  obj-y +=	no-block.o
> >  endif
> >  
> > +obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
> > +
> >  obj-$(CONFIG_PROC_FS) += proc_namespace.o
> 
> Nit: maybe move this to just below the line for iomap.o, to be consistent with
> where FS_READ_CALLBACKS is in the Kconfig file.
> 
> >  
> >  obj-y				+= notify/
> > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> > index 24ed99e2eca0..7752f9964280 100644
> > --- a/fs/crypto/Kconfig
> > +++ b/fs/crypto/Kconfig
> > @@ -9,6 +9,7 @@ config FS_ENCRYPTION
> >  	select CRYPTO_CTS
> >  	select CRYPTO_SHA256
> >  	select KEYS
> > +	select FS_READ_CALLBACKS if BLOCK
> >  	help
> >  	  Enable encryption of files and directories.  This
> >  	  feature is similar to ecryptfs, but it is more memory
> > diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> > index 82da2510721f..f677ff93d464 100644
> > --- a/fs/crypto/bio.c
> > +++ b/fs/crypto/bio.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/module.h>
> >  #include <linux/bio.h>
> >  #include <linux/namei.h>
> > +#include <linux/read_callbacks.h>
> >  #include "fscrypt_private.h"
> >  
> >  static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
> > @@ -68,6 +69,16 @@ void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx, struct bio *bio)
> >  }
> >  EXPORT_SYMBOL(fscrypt_enqueue_decrypt_bio);
> >  
> > +void fscrypt_decrypt_work(struct work_struct *work)
> > +{
> > +	struct read_callbacks_ctx *ctx =
> > +		container_of(work, struct read_callbacks_ctx, work);
> > +
> > +	fscrypt_decrypt_bio(ctx->bio);
> > +
> > +	read_callbacks(ctx);
> > +}
> > +
> 
> This seems like a layering violation, since it means that read_callbacks.c calls
> fs/crypto/ *and* fs/crypto/ calls read_callbacks.c.  I don't think fs/crypto/
> should be aware of read_callbacks at all.  Instead we should have a clear
> ordering between the components: the filesystem uses read_callbacks.c and
> fs/crypto/, and read_callbacks.c uses fs/crypto/.  So how about:
> 
> - Move fscrypt_decrypt_work(), fscrypt_decrypt_bh(), and fscrypt_decrypt_bio()
>   into fs/read_callbacks.c and remove the "fscrypt_" prefix from them.
> 
> - Instead of selecting FS_READ_CALLBACKS from FS_ENCRYPTION, select it from
>   EXT4_FS and F2FS_FS (if FS_ENCRYPTION).  I.e., it's really the *filesystems*
>   themselves that use read_callbacks, not fs/crypto/.
> 
> - Move the definition of read_callbacks_ctx into fs/read_callbacks.c, and make
>   read_callbacks() static, so these are private to the read_callbacks component.
> 
> >  int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
> >  				sector_t pblk, unsigned int len)
> >  {
> > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> > new file mode 100644
> > index 000000000000..a4196e3de05f
> > --- /dev/null
> > +++ b/fs/read_callbacks.c
> > @@ -0,0 +1,174 @@
> > +// 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/read_callbacks.h>
> > +
> > +#define NUM_PREALLOC_READ_CALLBACK_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,
> > +};
> > +
> > +static void put_read_callbacks_ctx(struct read_callbacks_ctx *ctx)
> > +{
> > +	mempool_free(ctx, read_callbacks_ctx_pool);
> > +}
> 
> Maybe call this free_read_callbacks_ctx(), so that it doesn't sound like it's
> doing reference counting.
> 
> > +
> > +static void end_read_callbacks_bio(struct bio *bio)
> > +{
> > +	struct read_callbacks_ctx *ctx;
> > +	struct page *page;
> > +	struct bio_vec *bv;
> > +	struct bvec_iter_all iter_all;
> > +
> > +	ctx = bio->bi_private;
> > +
> > +	bio_for_each_segment_all(bv, bio, iter_all) {
> > +		page = bv->bv_page;
> > +
> > +		if (bio->bi_status || PageError(page)) {
> > +			ClearPageUptodate(page);
> > +			SetPageError(page);
> > +		} else {
> > +			SetPageUptodate(page);
> > +		}
> > +
> > +		if (ctx->page_op)
> > +			ctx->page_op(bio, page);
> > +
> > +		unlock_page(page);
> > +	}
> > +
> > +	put_read_callbacks_ctx(ctx);
> > +
> > +	bio_put(bio);
> > +}
> 
> The filesystem itself (or fs/mpage.c) actually has to implement almost all this
> logic as well anyway, because CONFIG_FS_READ_CALLBACKS may be unset.  And the
> ->page_op() callback, which exists only because f2fs needs to update some
> counter, is very ugly.
> 
> IMO, it would be simpler to just make this whole function filesystem-specific,
> as a 'typedef void (*end_bio_func_t)(struct bio *bio);' which gets passed to the
> read_callbacks endio hook.
> 
> Of course, each end_bio_func_t would have to check PageError() to check whether
> any read_callbacks step failed.  But to make that a bit easier and to make it
> get compiled out when CONFIG_FS_READ_CALLBACKS is unset, there could be a helper
> function in read_callbacks.h:
> 
> 	#ifdef CONFIG_FS_READ_CALLBACKS
> 	static inline bool read_callbacks_failed(struct page *page)
> 	{
> 		return PageError(page);
> 	}
> 	#else
> 	static inline bool read_callbacks_failed(struct page *page)
> 	{
> 		return false;
> 	}
> 	#endif
> 
> > +
> > +/**
> > + * read_callbacks() - Execute the read callbacks state machine.
> > + * @ctx: The context structure tracking the current state.
> > + *
> > + * For each state, this function enqueues a work into appropriate subsystem's
> > + * work queue. After performing further processing of the data in the bio's
> > + * pages, the subsystem should invoke read_calbacks() to continue with the next
> > + * state in the state machine.
> > + */
> > +void read_callbacks(struct read_callbacks_ctx *ctx)
> > +{
> > +	/*
> > +	 * We use different work queues for decryption and for verity because
> > +	 * verity may require reading metadata pages that need decryption, and
> > +	 * we shouldn't recurse to the same workqueue.
> > +	 */
> > +	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;
> > +		}
> > +		ctx->cur_step++;
> > +		/* fall-through */
> > +	default:
> > +		end_read_callbacks_bio(ctx->bio);
> > +	}
> > +}
> > +EXPORT_SYMBOL(read_callbacks);
> 
> As I mentioned, I think the work functions should be defined in this file rather
> than in fs/crypto/ or fs/verity/, since they're specific to the read_callbacks.
> fs/crypto/ and fs/verity/ should not be aware of read_callbacks at all.
> Moreover, the 'read_callbacks()' function should be static.
> 
> > +
> > +/**
> > + * read_callbacks_end_bio() - Initiate the read callbacks state machine.
> > + * @bio: bio on which read I/O operation has just been completed.
> > + *
> > + * Initiates the execution of the read callbacks state machine when the read
> > + * operation has been completed successfully. If there was an error associated
> > + * with the bio, this function frees the read callbacks context structure stored
> > + * in bio->bi_private (if any).
> > + *
> > + * Return: 1 to indicate that the bio has been handled (including unlocking the
> > + * pages); 0 otherwise.
> > + */
> > +int read_callbacks_end_bio(struct bio *bio)
> > +{
> > +	if (!bio->bi_status && bio->bi_private) {
> > +		read_callbacks((struct read_callbacks_ctx *)(bio->bi_private));
> > +		return 1;
> > +	}
> > +
> > +	if (bio->bi_private)
> > +		put_read_callbacks_ctx((struct read_callbacks_ctx *)(bio->bi_private));
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(read_callbacks_end_bio);
> 
> Okay, several issues here...
> 
> First, the naming of this is really confusing, since it's actually *beginning*
> the read callbacks, not ending them; and it's basically the same name as
> end_read_callbacks_bio(), which actually *is* for ending the read callbacks.
> Since this is the endio hook, how about calling it read_callbacks_endio_bio(),
> and likewise read_callbacks_endio_bh()?
> 
> But more importantly, this doesn't need to have a return value, since the
> read_callbacks layer has to know how to end the bio (meaning unlock the pages
> and mark them uptodate or not) *anyway*, or at least know how to ask the
> filesystem to do it.  So it should just do it if needed, rather than returning 0
> and making the caller do it.
> 
> Also just assign 'ctx = bio->bi_private' at the start of the function; no need
> to access the field 4 times and have unnecessary casts.
> 
> So IMO, the below would be much better:
> 
> void read_callbacks_endio_bio(struct bio *bio, end_bio_func_t end_bio)
> {
> 	struct read_callbacks_ctx *ctx = bio->bi_private;
> 
> 	if (ctx) {
> 		if (!bio->bi_status) {
> 			ctx->end_bio = end_bio;
> 			read_callbacks(ctx);
> 			return;
> 		}
> 		free_read_callbacks_ctx(ctx);
> 	}
> 	end_bio(bio);
> }
> EXPORT_SYMBOL(read_callbacks_endio_bio);
> 
> And then the !CONFIG_FS_READ_CALLBACKS stub:
> 
> static inline void read_callbacks_endio_bio(struct bio *bio,
> 					    end_bio_func_t end_bio)
> {
> 	end_bio(bio);
> }
> 
> Similarly for the buffer_head versions.
> 
> > +
> > +/**
> > + * read_callbacks_setup() - Initialize the read callbacks state machine
> > + * @inode: The file on which read I/O is performed.
> > + * @bio: bio holding page cache pages on which read I/O is performed.
> > + * @page_op: Function to perform filesystem specific operations on a page.
> > + *
> > + * Based on the nature of the file's data (e.g. encrypted), this function
> > + * computes the steps that need to be performed after data is read of the
> > + * backing disk. This information is saved in a context structure. A pointer
> > + * to the context structure is then stored in bio->bi_private for later
> > + * usage.
> > + *
> > + * Return: 0 on success; An error code on failure.
> > + */
> > +int read_callbacks_setup(struct inode *inode, struct bio *bio,
> > +			end_page_op_t page_op)
> > +{
> > +	struct read_callbacks_ctx *ctx = NULL;
> > +	unsigned int enabled_steps = 0;
> > +
> > +	if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode))
> > +		enabled_steps |= 1 << STEP_DECRYPT;
> > +
> > +	if (enabled_steps) {
> > +		ctx = mempool_alloc(read_callbacks_ctx_pool, GFP_NOFS);
> > +		if (!ctx)
> > +			return -ENOMEM;
> > +		ctx->bio = bio;
> > +		ctx->inode = inode;
> > +		ctx->enabled_steps = enabled_steps;
> > +		ctx->cur_step = STEP_INITIAL;
> > +		ctx->page_op = page_op;
> > +		bio->bi_private = ctx;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(read_callbacks_setup);
> 
> Please call it:
> 
> 	read_callbacks_setup_bio()
> 
> Then when you add buffer_head support later in the patchset, rather than adding
> a buffer_head argument to this function, add a new function:
> 
> 	read_callbacks_setup_bh()
> 
> So that all the users don't have to pass both the buffer_head and bio arguments.
> 
> These can use a common function get_read_callbacks_ctx() that creates a
> read_callbacks_ctx for the inode.  E.g., the bio version could look like:
> 
> int read_callbacks_setup_bio(struct inode *inode, struct bio *bio)
> {
> 	struct read_callbacks_ctx *ctx = get_read_callbacks_ctx(inode);
> 
> 	if (ctx) {
> 		if (IS_ERR(ctx))
> 			return PTR_ERR(ctx);
> 		ctx->bio = bio;
> 		ctx->bh = NULL;
> 		bio->bi_private = ctx;
> 	}
> 	return 0;
> }
> EXPORT_SYMBOL(read_callbacks_setup_bio);
> 
> 
> Also, a nit: can you move the read_callbacks_setup_*() functions to near the top
> of the file, since they're called first (before the endio functions)?  Likewise
> in read_callbacks.h.  It would nice to keep the functions in a logical order.
> 
> > diff --git a/include/linux/read_callbacks.h b/include/linux/read_callbacks.h
> > new file mode 100644
> > index 000000000000..aa1ec8ed7a6a
> > --- /dev/null
> > +++ b/include/linux/read_callbacks.h
> > @@ -0,0 +1,38 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _READ_CALLBACKS_H
> > +#define _READ_CALLBACKS_H
> 
> Headers should be self-contained, but this is missing some prerequisite headers
> and forward declarations.  Try compiling a .c file with this header included
> first.
> 
> > +
> > +typedef void (*end_page_op_t)(struct bio *bio, struct page *page);
> > +
> > +struct read_callbacks_ctx {
> > +	struct bio *bio;
> > +	struct inode *inode;
> > +	struct work_struct work;
> > +	unsigned int cur_step;
> > +	unsigned int enabled_steps;
> > +	end_page_op_t page_op;
> > +};
> 
> As I mentioned, I don't think read_callbacks_ctx should be exposed to
> filesystems like this.  Instead just forward declare it here, and put the actual
> definition in fs/read_callbacks.c.
> 
> > +
> > +#ifdef CONFIG_FS_READ_CALLBACKS
> > +void read_callbacks(struct read_callbacks_ctx *ctx);
> > +int read_callbacks_end_bio(struct bio *bio);
> > +int read_callbacks_setup(struct inode *inode, struct bio *bio,
> > +			end_page_op_t page_op);
> > +#else
> > +static inline void read_callbacks(struct read_callbacks_ctx *ctx)
> > +{
> > +}
> > +
> > +static inline int read_callbacks_end_bio(struct bio *bio)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 
> This stub needs to return 0, otherwise it breaks fs/mpage.c and f2fs for
> everyone when CONFIG_FS_READ_CALLBACKS is unset.
> 
> But as I mentioned elsewhere, I think this should actually call an end_bio()
> callback itself and return void, which would also avoid this issue.
> 
> > +
> > +static inline int read_callbacks_setup(struct inode *inode,
> > +				struct bio *bio, end_page_op_t page_op)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> 
> Similarly here, this stub needs to return 0.
> 

I agree with all your comments. I will fix them up and post the next version.

-- 
chandan

Powered by blists - more mailing lists