linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 0/8] Consolidate FS read I/O callbacks code
@ 2019-08-16  6:17 Chandan Rajendra
  2019-08-16  6:17 ` [PATCH V4 1/8] buffer_head: Introduce BH_Read_Cb flag Chandan Rajendra
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:17 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, tytso, adilger.kernel, ebiggers, jaegeuk, yuchao0, hch

This patchset moves the "FS read I/O callbacks" code into a file of its
own (i.e. fs/read_callbacks.c) and modifies the generic
do_mpage_readpge() to make use of the functionality provided.

"FS read I/O callbacks" code implements the state machine that needs
to be executed after reading data from files that are encrypted and/or
have verity metadata associated with them.

With these changes in place, the patchset changes Ext4 to use
mpage_readpage[s] instead of its own custom ext4_readpage[s]()
functions. This is done to reduce duplication of code across
filesystems. Also, "FS read I/O callbacks" source files will be built
only if CONFIG_FS_ENCRYPTION is enabled.

The patchset also modifies fs/buffer.c to get file
encryption/decryption to work with subpage-sized blocks.

The patches can also be obtained from
https://github.com/chandanr/linux.git at branch subpage-encryption-v4.

Changelog:
V3 -> V4:
   1. A new buffer_head flag (i.e. BH_Read_Cb) is introduced to reliably
      check if a buffer head's content has to be decrypted.
   2. Fix layering violation. Now the code flow for decryption happens as shown below,
      FS => read callbacks => fscrypt
   3. Select FS_READ_CALLBACKS from FS specific kconfig file if FS_ENCRYPTION
      is enabled.
   4. Make 'struct read_callbacks_ctx' an opaque structure.
   5. Make use of FS' endio function rather than implementing one in read
      callbacks.
   6. Make read_callbacks.h self-contained.
   7. Split patchset to separate out ext4 and f2fs changes.
   
V2 -> V3:
1. Split the V2 patch "Consolidate 'read callbacks' into a new file" into
   three patches,
   - Introduce the read_callbacks functionality.
   - Convert encryption to use read_callbacks.
   - Remove union from struct fscrypt_context.
2. fs/Kconfig
   Do not explicitly set the default value of 'n' for FS_READ_CALLBACKS.
3. fs/crypto/Kconfig
   Select CONFIG_FS_READ_CALLBACKS only if CONFIG_BLOCK is selected.
4. Remove verity associated code in read_callbacks code.
5. Introduce a callback argument to read_callbacks_setup() function
   which gets invoked for each page for bio. F2FS uses this to perform
   custom operations like decrementing the value of f2fs_sb_info->nr_pages[].
6. Encapsulate the details of "read callbacks" (e.g. Usage of "struct
   read_callbacks *ctx") within its own functions. When CONFIG_FS_READ_CALLBACKS
   is set to 'n', the corresponding stub functions return approriate error
   values.
7. Split fscrypt_decrypt() function into fscrypt_decrypt_bio() and
   fscrypt_decrypt_bh().
8. Split end_read_callbacks() function into end_read_callbacks_bio() and
   end_read_callbacks_bh().

V1 -> V2:
1. Removed the phrase "post_read_process" from file names and
   functions. Instead we now use the phrase "read_callbacks" in its
   place.
2. When performing changes associated with (1), the changes made by
   the patch "Remove the term 'bio' from post read processing" are
   made in the earlier patch "Consolidate 'read callbacks' into a new
   file". Hence the patch "Remove the term 'bio' from post read
   processing" is removed from the patchset.

RFC V2 -> V1:
1. Test and verify FS_CFLG_OWN_PAGES subset of fscrypt_encrypt_page()
   code by executing fstests on UBIFS.
2. Implement F2fs function call back to check if the contents of a
   page holding a verity file's data needs to be verified.

RFC V1 -> RFC V2:
1. Describe the purpose of "Post processing code" in the cover letter.
2. Fix build errors when CONFIG_FS_VERITY is enabled.

Chandan Rajendra (8):
  buffer_head: Introduce BH_Read_Cb flag
  FS: Introduce read callbacks
  fs/mpage.c: Integrate read callbacks
  fs/buffer.c: add decryption support via read_callbacks
  f2fs: Use read_callbacks for decrypting file data
  ext4: Wire up ext4_readpage[s] to use mpage_readpage[s]
  ext4: Enable encryption for subpage-sized blocks
  fscrypt: remove struct fscrypt_ctx

 Documentation/filesystems/fscrypt.rst |   4 +-
 fs/Kconfig                            |   3 +
 fs/Makefile                           |   1 +
 fs/buffer.c                           |  33 ++-
 fs/crypto/bio.c                       |  43 ----
 fs/crypto/crypto.c                    |  89 +-------
 fs/crypto/fscrypt_private.h           |   3 -
 fs/ext4/Kconfig                       |   1 +
 fs/ext4/Makefile                      |   2 +-
 fs/ext4/inode.c                       |   5 +-
 fs/ext4/readpage.c                    | 295 --------------------------
 fs/ext4/super.c                       |   7 -
 fs/f2fs/Kconfig                       |   1 +
 fs/f2fs/data.c                        | 109 +---------
 fs/f2fs/f2fs.h                        |   2 -
 fs/f2fs/super.c                       |   9 +-
 fs/mpage.c                            |  24 ++-
 fs/read_callbacks.c                   | 285 +++++++++++++++++++++++++
 include/linux/buffer_head.h           |   2 +
 include/linux/fscrypt.h               |  38 ----
 include/linux/read_callbacks.h        |  48 +++++
 21 files changed, 402 insertions(+), 602 deletions(-)
 delete mode 100644 fs/ext4/readpage.c
 create mode 100644 fs/read_callbacks.c
 create mode 100644 include/linux/read_callbacks.h

-- 
2.19.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH V4 1/8] buffer_head: Introduce BH_Read_Cb flag
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
@ 2019-08-16  6:17 ` Chandan Rajendra
  2019-08-16  6:17 ` [PATCH V4 2/8] FS: Introduce read callbacks Chandan Rajendra
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:17 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

Decryption of file content encrypted using fscrypt relies on
bio->bi_private holding a pointer to an encryption context
i.e. Decryption operation is not performed for bios having a NULL value
at bio->bi_private.

The same logic cannot be used on buffer heads because,
1. In Btrfs, write_dev_supers() sets bh->b_private to 'struct
   btrfs_device' pointer and submits the buffer head for a write
   operation.
   1. In btrfs/146 test, the write operation fails and hence the
      endio function clears the BH_Uptodate flag.
   2. A read operation initiated later will submit the buffer head to
      the block layer. During endio processing, bh_>b_private would have a
      non-NULL value.

2. Another instance is when an Ext4 metadata block with BH_Uptodate set and
   also part of the in-memory JBD list undergoes the following,
   1. A sync() syscall is invoked by the userspace and the write
      operation on the metadata block is initiated.
   2. Due to an I/O failure, the BH_Uptodate flag is cleared by
      end_buffer_async_write(). The bh->b_private member would be
      pointing to a journal head structure.
   3. In such a case, a read operation invoked on the block mapped by the
      buffer head will initiate a read from the disk since the buffer head is
      missing the BH_Uptodate flag.
   4. After the read I/O request is submitted, end_buffer_async_read()
      will find a non-NULL value at bh->b_private.
   This scenario was observed when executing generic/475 test case.

Hence this commit introduces a new buffer head flag to reliably check for
decryption of a buffer head's contents after the block has been read
from the disk.

Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
---
 include/linux/buffer_head.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 7b73ef7f902d..08f217ba8114 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -38,6 +38,7 @@ enum bh_state_bits {
 	BH_Meta,	/* Buffer contains metadata */
 	BH_Prio,	/* Buffer should be submitted with REQ_PRIO */
 	BH_Defer_Completion, /* Defer AIO completion to workqueue */
+	BH_Read_Cb,	     /* Block's contents needs to be decrypted */
 
 	BH_PrivateStart,/* not a state bit, but the first bit available
 			 * for private allocation by other entities
@@ -134,6 +135,7 @@ BUFFER_FNS(Unwritten, unwritten)
 BUFFER_FNS(Meta, meta)
 BUFFER_FNS(Prio, prio)
 BUFFER_FNS(Defer_Completion, defer_completion)
+BUFFER_FNS(Read_Cb, read_cb)
 
 #define bh_offset(bh)		((unsigned long)(bh)->b_data & ~PAGE_MASK)
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 2/8] FS: Introduce read callbacks
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
  2019-08-16  6:17 ` [PATCH V4 1/8] buffer_head: Introduce BH_Read_Cb flag Chandan Rajendra
@ 2019-08-16  6:17 ` Chandan Rajendra
  2019-08-16  6:17 ` [PATCH V4 3/8] fs/mpage.c: Integrate " Chandan Rajendra
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:17 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

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@linux.ibm.com>
---
 fs/Kconfig                     |   3 +
 fs/Makefile                    |   1 +
 fs/ext4/Kconfig                |   1 +
 fs/f2fs/Kconfig                |   1 +
 fs/read_callbacks.c            | 285 +++++++++++++++++++++++++++++++++
 include/linux/read_callbacks.h |  48 ++++++
 6 files changed, 339 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 3e6d3101f3ff..2d96a58d7418 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -20,6 +20,9 @@ if BLOCK
 config FS_IOMAP
 	bool
 
+config FS_READ_CALLBACKS
+	bool
+
 source "fs/ext2/Kconfig"
 source "fs/ext4/Kconfig"
 source "fs/jbd2/Kconfig"
diff --git a/fs/Makefile b/fs/Makefile
index 427fec226fae..942808f83472 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_SYSCTL)		+= drop_caches.o
 
 obj-$(CONFIG_FHANDLE)		+= fhandle.o
 obj-$(CONFIG_FS_IOMAP)		+= iomap.o
+obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
 
 obj-y				+= quota/
 
diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
index 06f77ca7f36e..2e24df67f085 100644
--- a/fs/ext4/Kconfig
+++ b/fs/ext4/Kconfig
@@ -38,6 +38,7 @@ config EXT4_FS
 	select CRYPTO
 	select CRYPTO_CRC32C
 	select FS_IOMAP
+	select FS_READ_CALLBACKS if FS_ENCRYPTION
 	help
 	  This is the next generation of the ext3 filesystem.
 
diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
index e57cc754d543..1e1424940d1b 100644
--- a/fs/f2fs/Kconfig
+++ b/fs/f2fs/Kconfig
@@ -4,6 +4,7 @@ config F2FS_FS
 	select CRYPTO
 	select CRYPTO_CRC32
 	select F2FS_FS_XATTR if FS_ENCRYPTION
+	select FS_READ_CALLBACKS if FS_ENCRYPTION
 	help
 	  F2FS is based on Log-structured File System (LFS), which supports
 	  versatile "flash-friendly" features. The design has been focused on
diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
new file mode 100644
index 000000000000..32d9b8d17964
--- /dev/null
+++ b/fs/read_callbacks.c
@@ -0,0 +1,285 @@
+// 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/buffer_head.h>
+#include <linux/fscrypt.h>
+#include <linux/read_callbacks.h>
+#include <linux/blk_types.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,
+};
+
+struct read_callbacks_ctx {
+	struct inode *inode;
+	struct bio *bio;
+	struct buffer_head *bh;
+	union {
+		end_bio_func_t end_bio;
+
+		struct {
+			end_bh_func_t end_bh;
+			int bh_uptodate;
+		};
+	};
+	struct work_struct work;
+	unsigned int enabled_steps;
+	enum read_callbacks_step cur_step;
+};
+
+static void read_callbacks(struct read_callbacks_ctx *ctx);
+
+static void free_read_callbacks_ctx(struct read_callbacks_ctx *ctx)
+{
+	mempool_free(ctx, read_callbacks_ctx_pool);
+}
+
+static struct read_callbacks_ctx *get_read_callbacks_ctx(struct inode *inode)
+{
+	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 ERR_PTR(-ENOMEM);
+
+		ctx->inode = inode;
+		ctx->enabled_steps = enabled_steps;
+		ctx->cur_step = STEP_INITIAL;
+	}
+
+	return ctx;
+}
+
+static void decrypt_bio(struct bio *bio)
+{
+	struct bio_vec *bv;
+	int i;
+	struct bvec_iter_all iter_all;
+
+	bio_for_each_segment_all(bv, bio, i, iter_all) {
+		struct page *page = bv->bv_page;
+		int ret = fscrypt_decrypt_pagecache_blocks(page, bv->bv_len,
+							   bv->bv_offset);
+		if (ret)
+			SetPageError(page);
+	}
+}
+
+static void decrypt_bh(struct buffer_head *bh)
+{
+	struct page *page;
+	int ret;
+
+	page = bh->b_page;
+
+	ret = fscrypt_decrypt_pagecache_blocks(page, bh->b_size,
+					bh_offset(bh));
+	if (ret)
+		SetPageError(page);
+}
+
+static void decrypt_work(struct work_struct *work)
+{
+	struct read_callbacks_ctx *ctx =
+		container_of(work, struct read_callbacks_ctx, work);
+
+	if (ctx->bio)
+		decrypt_bio(ctx->bio);
+	else
+		decrypt_bh(ctx->bh);
+
+	read_callbacks(ctx);
+}
+
+static 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, decrypt_work);
+			fscrypt_enqueue_decrypt_work(&ctx->work);
+			return;
+		}
+		ctx->cur_step++;
+		/* fall-through */
+	default:
+		if (ctx->bio)
+			ctx->end_bio(ctx->bio);
+		else
+			ctx->end_bh(ctx->bh, ctx->bh_uptodate);
+
+		mempool_free(ctx, read_callbacks_ctx_pool);
+	}
+}
+
+/**
+ * read_callbacks_setup_bio() - 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.
+ * @bh: Buffer head 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_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);
+
+/**
+ * read_callbacks_setup_bh() - 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.
+ * @bh: Buffer head 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_bh(struct inode *inode, struct buffer_head *bh)
+{
+	struct read_callbacks_ctx *ctx = get_read_callbacks_ctx(inode);
+
+	if (ctx) {
+		if (IS_ERR(ctx))
+			return PTR_ERR(ctx);
+		ctx->bio = NULL;
+		ctx->bh = bh;
+		bh->b_private = ctx;
+		set_buffer_read_cb(bh);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(read_callbacks_setup_bh);
+
+/**
+ * read_callbacks_endio_bio() - Initiate the read callbacks state machine.
+ * @bio: bio on which read I/O operation has just been completed.
+ * @end_bio: Callback to invoke on @bio after state machine completion.
+ *
+ * Initiates the execution of the read callbacks state machine when the read
+ * operation has been completed sucessfully. If there was an error associated
+ * with the bio, this function frees the read callbacks context structure stored
+ * in bio->bi_private (if any).
+ *
+ * If read callbacks state machine isn't executed, we end up invoking the
+ * @end_bio function passed in as an argument.
+ */
+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);
+
+/**
+ * read_callbacks_endio_bh() - Initiate the read callbacks state machine.
+ * @bh: buffer head on which read I/O operation has just been completed.
+ * @uptodate: Buffer head's I/O status.
+ * @end_bh: Callback to invoke on on @bh after state machine completion.
+ *
+ * Initiates the execution of the read callbacks state machine when the read
+ * operation has been completed sucessfully. If there was an error associated
+ * with the buffer head, this function frees the read callbacks context
+ * structure stored in bh->b_private (if any).
+ *
+ * If read callbacks state machine isn't executed, we end up invoking the
+ * @end_bh function passed in as an argument.
+ */
+void read_callbacks_endio_bh(struct buffer_head *bh, int uptodate, end_bh_func_t end_bh)
+{
+	struct read_callbacks_ctx *ctx = bh->b_private;
+
+	if (buffer_read_cb(bh)) {
+		clear_buffer_read_cb(bh);
+		if (uptodate) {
+			ctx->end_bh = end_bh;
+			ctx->bh_uptodate = uptodate;
+			read_callbacks(ctx);
+			return;
+		}
+
+		free_read_callbacks_ctx(ctx);
+	}
+
+	end_bh(bh, uptodate);
+}
+EXPORT_SYMBOL(read_callbacks_endio_bh);
+
+static int __init init_read_callbacks(void)
+{
+	read_callbacks_ctx_cache = KMEM_CACHE(read_callbacks_ctx, 0);
+	if (!read_callbacks_ctx_cache)
+		goto fail;
+	read_callbacks_ctx_pool =
+		mempool_create_slab_pool(NUM_PREALLOC_READ_CALLBACK_CTXS,
+					 read_callbacks_ctx_cache);
+	if (!read_callbacks_ctx_pool)
+		goto fail_free_cache;
+	return 0;
+
+fail_free_cache:
+	kmem_cache_destroy(read_callbacks_ctx_cache);
+fail:
+	return -ENOMEM;
+}
+
+fs_initcall(init_read_callbacks);
diff --git a/include/linux/read_callbacks.h b/include/linux/read_callbacks.h
new file mode 100644
index 000000000000..0d709dd81b4e
--- /dev/null
+++ b/include/linux/read_callbacks.h
@@ -0,0 +1,48 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _READ_CALLBACKS_H
+#define _READ_CALLBACKS_H
+
+#include <linux/blk_types.h>
+
+typedef void (*end_bio_func_t)(struct bio *bio);
+typedef void (*end_bh_func_t)(struct buffer_head *bh, int uptodate);
+
+#ifdef CONFIG_FS_READ_CALLBACKS
+int read_callbacks_setup_bio(struct inode *inode, struct bio *bio);
+int read_callbacks_setup_bh(struct inode *inode, struct buffer_head *bh);
+void read_callbacks_endio_bio(struct bio *bio, end_bio_func_t end_bio);
+void read_callbacks_endio_bh(struct buffer_head *bh, int uptodate, end_bh_func_t end_bh);
+
+static inline bool read_callbacks_failed(struct page *page)
+{
+	return PageError(page);
+}
+#else
+static inline int read_callbacks_setup_bio(struct inode *inode, struct bio *bio)
+{
+	return 0;
+}
+
+static inline int read_callbacks_setup_bh(struct inode *inode, struct buffer_head *bh)
+{
+	return 0;
+}
+
+static inline void read_callbacks_endio_bio(struct bio *bio,
+					end_bio_func_t end_bio)
+{
+	end_bio(bio);
+}
+
+static inline void read_callbacks_endio_bh(struct buffer_head *bh, int uptodate, end_bh_func_t end_bh)
+{
+	end_bh(bh, uptodate);
+}
+
+static inline bool read_callbacks_failed(struct page *page)
+{
+	return false;
+}
+#endif
+
+#endif	/* _READ_CALLBACKS_H */
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 3/8] fs/mpage.c: Integrate read callbacks
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
  2019-08-16  6:17 ` [PATCH V4 1/8] buffer_head: Introduce BH_Read_Cb flag Chandan Rajendra
  2019-08-16  6:17 ` [PATCH V4 2/8] FS: Introduce read callbacks Chandan Rajendra
@ 2019-08-16  6:17 ` Chandan Rajendra
  2019-08-16  6:18 ` [PATCH V4 4/8] fs/buffer.c: add decryption support via read_callbacks Chandan Rajendra
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:17 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

This commit adds code to make do_mpage_readpage() to be "read callbacks"
aware i.e. for files requiring decryption, do_mpage_readpage() now
sets up the read callbacks state machine when allocating a bio and later
starts execution of the state machine after file data is read from the
underlying disk.

Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
---
 fs/mpage.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index 3f19da75178b..65e7165644e2 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -30,6 +30,7 @@
 #include <linux/backing-dev.h>
 #include <linux/pagevec.h>
 #include <linux/cleancache.h>
+#include <linux/read_callbacks.h>
 #include "internal.h"
 
 /*
@@ -44,7 +45,7 @@
  * status of that page is hard.  See end_buffer_async_read() for the details.
  * There is no point in duplicating all that complexity.
  */
-static void mpage_end_io(struct bio *bio)
+static void end_bio(struct bio *bio)
 {
 	struct bio_vec *bv;
 	int i;
@@ -52,13 +53,24 @@ static void mpage_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bv, bio, i, iter_all) {
 		struct page *page = bv->bv_page;
-		page_endio(page, bio_op(bio),
-			   blk_status_to_errno(bio->bi_status));
+		int err;
+
+		err = blk_status_to_errno(bio->bi_status);
+
+		if (!err && read_callbacks_failed(page))
+			err = -EIO;
+
+		page_endio(page, bio_op(bio), err);
 	}
 
 	bio_put(bio);
 }
 
+static void mpage_end_io(struct bio *bio)
+{
+	read_callbacks_endio_bio(bio, end_bio);
+}
+
 static struct bio *mpage_bio_submit(int op, int op_flags, struct bio *bio)
 {
 	bio->bi_end_io = mpage_end_io;
@@ -310,6 +322,12 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args)
 					gfp);
 		if (args->bio == NULL)
 			goto confused;
+
+		if (read_callbacks_setup_bio(inode, args->bio)) {
+			bio_put(args->bio);
+			args->bio = NULL;
+			goto confused;
+		}
 	}
 
 	length = first_hole << blkbits;
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 4/8] fs/buffer.c: add decryption support via read_callbacks
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
                   ` (2 preceding siblings ...)
  2019-08-16  6:17 ` [PATCH V4 3/8] fs/mpage.c: Integrate " Chandan Rajendra
@ 2019-08-16  6:18 ` Chandan Rajendra
  2019-08-16  6:18 ` [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data Chandan Rajendra
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:18 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

This commit sets up read_callbacks context for buffer heads whose
contents need to be decrypted on endio.

Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
---
 fs/buffer.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index ce357602f471..96c4c9840746 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -45,6 +45,7 @@
 #include <linux/bit_spinlock.h>
 #include <linux/pagevec.h>
 #include <linux/sched/mm.h>
+#include <linux/read_callbacks.h>
 #include <trace/events/block.h>
 
 static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);
@@ -245,11 +246,7 @@ __find_get_block_slow(struct block_device *bdev, sector_t block)
 	return ret;
 }
 
-/*
- * I/O completion handler for block_read_full_page() - pages
- * which come unlocked at the end of I/O.
- */
-static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
+void end_buffer_async_read(struct buffer_head *bh, int uptodate)
 {
 	unsigned long flags;
 	struct buffer_head *first;
@@ -257,8 +254,6 @@ static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
 	struct page *page;
 	int page_uptodate = 1;
 
-	BUG_ON(!buffer_async_read(bh));
-
 	page = bh->b_page;
 	if (uptodate) {
 		set_buffer_uptodate(bh);
@@ -306,6 +301,17 @@ static void end_buffer_async_read(struct buffer_head *bh, int uptodate)
 	return;
 }
 
+/*
+ * I/O completion handler for block_read_full_page().  Pages are unlocked
+ * after the I/O completes and the read callbacks (if any) have executed.
+ */
+static void __end_buffer_async_read(struct buffer_head *bh, int uptodate)
+{
+	BUG_ON(!buffer_async_read(bh));
+
+	read_callbacks_endio_bh(bh, uptodate, end_buffer_async_read);
+}
+
 /*
  * Completion handler for block_write_full_page() - pages which are unlocked
  * during I/O, and which have PageWriteback cleared upon I/O completion.
@@ -378,7 +384,7 @@ EXPORT_SYMBOL(end_buffer_async_write);
  */
 static void mark_buffer_async_read(struct buffer_head *bh)
 {
-	bh->b_end_io = end_buffer_async_read;
+	bh->b_end_io = __end_buffer_async_read;
 	set_buffer_async_read(bh);
 }
 
@@ -2293,10 +2299,15 @@ int block_read_full_page(struct page *page, get_block_t *get_block)
 	 */
 	for (i = 0; i < nr; i++) {
 		bh = arr[i];
-		if (buffer_uptodate(bh))
-			end_buffer_async_read(bh, 1);
-		else
+		if (buffer_uptodate(bh)) {
+			__end_buffer_async_read(bh, 1);
+		} else {
+			if (WARN_ON(read_callbacks_setup_bh(inode, bh))) {
+				__end_buffer_async_read(bh, 0);
+				continue;
+			}
 			submit_bh(REQ_OP_READ, 0, bh);
+		}
 	}
 	return 0;
 }
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
                   ` (3 preceding siblings ...)
  2019-08-16  6:18 ` [PATCH V4 4/8] fs/buffer.c: add decryption support via read_callbacks Chandan Rajendra
@ 2019-08-16  6:18 ` Chandan Rajendra
  2019-08-18 13:45   ` [f2fs-dev] " Chao Yu
  2019-08-20  5:05   ` Chandan Rajendra
  2019-08-16  6:18 ` [PATCH V4 6/8] ext4: Wire up ext4_readpage[s] to use mpage_readpage[s] Chandan Rajendra
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:18 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

F2FS has a copy of "post read processing" code using which encrypted
file data is decrypted. This commit replaces it to make use of the
generic read_callbacks facility.

Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
---
 fs/f2fs/data.c  | 109 ++++--------------------------------------------
 fs/f2fs/f2fs.h  |   2 -
 fs/f2fs/super.c |   9 +---
 3 files changed, 11 insertions(+), 109 deletions(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 757f050c650a..3cf1eca2ece9 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -18,6 +18,7 @@
 #include <linux/uio.h>
 #include <linux/cleancache.h>
 #include <linux/sched/signal.h>
+#include <linux/read_callbacks.h>
 
 #include "f2fs.h"
 #include "node.h"
@@ -25,11 +26,6 @@
 #include "trace.h"
 #include <trace/events/f2fs.h>
 
-#define NUM_PREALLOC_POST_READ_CTXS	128
-
-static struct kmem_cache *bio_post_read_ctx_cache;
-static mempool_t *bio_post_read_ctx_pool;
-
 static bool __is_cp_guaranteed(struct page *page)
 {
 	struct address_space *mapping = page->mapping;
@@ -69,19 +65,6 @@ static enum count_type __read_io_type(struct page *page)
 	return F2FS_RD_DATA;
 }
 
-/* postprocessing steps for read bios */
-enum bio_post_read_step {
-	STEP_INITIAL = 0,
-	STEP_DECRYPT,
-};
-
-struct bio_post_read_ctx {
-	struct bio *bio;
-	struct work_struct work;
-	unsigned int cur_step;
-	unsigned int enabled_steps;
-};
-
 static void __read_end_io(struct bio *bio)
 {
 	struct page *page;
@@ -93,7 +76,7 @@ static void __read_end_io(struct bio *bio)
 		page = bv->bv_page;
 
 		/* PG_error was set if any post_read step failed */
-		if (bio->bi_status || PageError(page)) {
+		if (bio->bi_status || read_callbacks_failed(page)) {
 			ClearPageUptodate(page);
 			/* will re-read again later */
 			ClearPageError(page);
@@ -103,42 +86,8 @@ static void __read_end_io(struct bio *bio)
 		dec_page_count(F2FS_P_SB(page), __read_io_type(page));
 		unlock_page(page);
 	}
-	if (bio->bi_private)
-		mempool_free(bio->bi_private, bio_post_read_ctx_pool);
-	bio_put(bio);
-}
 
-static void bio_post_read_processing(struct bio_post_read_ctx *ctx);
-
-static void decrypt_work(struct work_struct *work)
-{
-	struct bio_post_read_ctx *ctx =
-		container_of(work, struct bio_post_read_ctx, work);
-
-	fscrypt_decrypt_bio(ctx->bio);
-
-	bio_post_read_processing(ctx);
-}
-
-static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
-{
-	switch (++ctx->cur_step) {
-	case STEP_DECRYPT:
-		if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
-			INIT_WORK(&ctx->work, decrypt_work);
-			fscrypt_enqueue_decrypt_work(&ctx->work);
-			return;
-		}
-		ctx->cur_step++;
-		/* fall-through */
-	default:
-		__read_end_io(ctx->bio);
-	}
-}
-
-static bool f2fs_bio_post_read_required(struct bio *bio)
-{
-	return bio->bi_private && !bio->bi_status;
+	bio_put(bio);
 }
 
 static void f2fs_read_end_io(struct bio *bio)
@@ -149,15 +98,7 @@ static void f2fs_read_end_io(struct bio *bio)
 		bio->bi_status = BLK_STS_IOERR;
 	}
 
-	if (f2fs_bio_post_read_required(bio)) {
-		struct bio_post_read_ctx *ctx = bio->bi_private;
-
-		ctx->cur_step = STEP_INITIAL;
-		bio_post_read_processing(ctx);
-		return;
-	}
-
-	__read_end_io(bio);
+	read_callbacks_endio_bio(bio, __read_end_io);
 }
 
 static void f2fs_write_end_io(struct bio *bio)
@@ -556,8 +497,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 err;
 
 	if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC))
 		return ERR_PTR(-EFAULT);
@@ -569,17 +509,10 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
 	bio->bi_end_io = f2fs_read_end_io;
 	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
 
-	if (f2fs_encrypted_file(inode))
-		post_read_steps |= 1 << STEP_DECRYPT;
-	if (post_read_steps) {
-		ctx = mempool_alloc(bio_post_read_ctx_pool, GFP_NOFS);
-		if (!ctx) {
-			bio_put(bio);
-			return ERR_PTR(-ENOMEM);
-		}
-		ctx->bio = bio;
-		ctx->enabled_steps = post_read_steps;
-		bio->bi_private = ctx;
+	err = read_callbacks_setup_bio(inode, bio);
+	if (err) {
+		bio_put(bio);
+		return ERR_PTR(err);
 	}
 
 	return bio;
@@ -2860,27 +2793,3 @@ void f2fs_clear_page_cache_dirty_tag(struct page *page)
 						PAGECACHE_TAG_DIRTY);
 	xa_unlock_irqrestore(&mapping->i_pages, flags);
 }
-
-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);
-}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 87f75ebd2fd6..cea79321b794 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3125,8 +3125,6 @@ void f2fs_destroy_checkpoint_caches(void);
 /*
  * data.c
  */
-int f2fs_init_post_read_processing(void);
-void f2fs_destroy_post_read_processing(void);
 void f2fs_submit_merged_write(struct f2fs_sb_info *sbi, enum page_type type);
 void f2fs_submit_merged_write_cond(struct f2fs_sb_info *sbi,
 				struct inode *inode, struct page *page,
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 11b3a039a188..d7bbb4f1fdb3 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3597,15 +3597,11 @@ static int __init init_f2fs_fs(void)
 	err = register_filesystem(&f2fs_fs_type);
 	if (err)
 		goto free_shrinker;
+
 	f2fs_create_root_stats();
-	err = f2fs_init_post_read_processing();
-	if (err)
-		goto free_root_stats;
+
 	return 0;
 
-free_root_stats:
-	f2fs_destroy_root_stats();
-	unregister_filesystem(&f2fs_fs_type);
 free_shrinker:
 	unregister_shrinker(&f2fs_shrinker_info);
 free_sysfs:
@@ -3626,7 +3622,6 @@ static int __init init_f2fs_fs(void)
 
 static void __exit exit_f2fs_fs(void)
 {
-	f2fs_destroy_post_read_processing();
 	f2fs_destroy_root_stats();
 	unregister_filesystem(&f2fs_fs_type);
 	unregister_shrinker(&f2fs_shrinker_info);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 6/8] ext4: Wire up ext4_readpage[s] to use mpage_readpage[s]
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
                   ` (4 preceding siblings ...)
  2019-08-16  6:18 ` [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data Chandan Rajendra
@ 2019-08-16  6:18 ` Chandan Rajendra
  2019-08-16  6:18 ` [PATCH V4 7/8] ext4: Enable encryption for subpage-sized blocks Chandan Rajendra
  2019-08-16  6:18 ` [PATCH V4 8/8] fscrypt: remove struct fscrypt_ctx Chandan Rajendra
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:18 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

Now that do_mpage_readpage() is "post read process" aware, this commit
gets ext4_readpage[s] to use mpage_readpage[s] and deletes ext4's
readpage.c since the associated functionality is not required anymore.
---
 fs/ext4/Makefile   |   2 +-
 fs/ext4/inode.c    |   5 +-
 fs/ext4/readpage.c | 295 ---------------------------------------------
 3 files changed, 3 insertions(+), 299 deletions(-)
 delete mode 100644 fs/ext4/readpage.c

diff --git a/fs/ext4/Makefile b/fs/ext4/Makefile
index 8fdfcd3c3e04..7c38803a808d 100644
--- a/fs/ext4/Makefile
+++ b/fs/ext4/Makefile
@@ -8,7 +8,7 @@ obj-$(CONFIG_EXT4_FS) += ext4.o
 ext4-y	:= balloc.o bitmap.o block_validity.o dir.o ext4_jbd2.o extents.o \
 		extents_status.o file.o fsmap.o fsync.o hash.o ialloc.o \
 		indirect.o inline.o inode.o ioctl.o mballoc.o migrate.o \
-		mmp.o move_extent.o namei.o page-io.o readpage.o resize.o \
+		mmp.o move_extent.o namei.o page-io.o resize.o \
 		super.o symlink.o sysfs.o xattr.o xattr_trusted.o xattr_user.o
 
 ext4-$(CONFIG_EXT4_FS_POSIX_ACL)	+= acl.o
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 58597db621e1..a1136faed9d3 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3361,8 +3361,7 @@ static int ext4_readpage(struct file *file, struct page *page)
 		ret = ext4_readpage_inline(inode, page);
 
 	if (ret == -EAGAIN)
-		return ext4_mpage_readpages(page->mapping, NULL, page, 1,
-						false);
+		return mpage_readpage(page, ext4_get_block);
 
 	return ret;
 }
@@ -3377,7 +3376,7 @@ ext4_readpages(struct file *file, struct address_space *mapping,
 	if (ext4_has_inline_data(inode))
 		return 0;
 
-	return ext4_mpage_readpages(mapping, pages, NULL, nr_pages, true);
+	return mpage_readpages(mapping, pages, nr_pages, ext4_get_block);
 }
 
 static void ext4_invalidatepage(struct page *page, unsigned int offset,
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
deleted file mode 100644
index 75cef6af6080..000000000000
--- a/fs/ext4/readpage.c
+++ /dev/null
@@ -1,295 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * linux/fs/ext4/readpage.c
- *
- * Copyright (C) 2002, Linus Torvalds.
- * Copyright (C) 2015, Google, Inc.
- *
- * This was originally taken from fs/mpage.c
- *
- * The intent is the ext4_mpage_readpages() function here is intended
- * to replace mpage_readpages() in the general case, not just for
- * encrypted files.  It has some limitations (see below), where it
- * will fall back to read_block_full_page(), but these limitations
- * should only be hit when page_size != block_size.
- *
- * This will allow us to attach a callback function to support ext4
- * encryption.
- *
- * If anything unusual happens, such as:
- *
- * - encountering a page which has buffers
- * - encountering a page which has a non-hole after a hole
- * - encountering a page with non-contiguous blocks
- *
- * then this code just gives up and calls the buffer_head-based read function.
- * It does handle a page which has holes at the end - that is a common case:
- * the end-of-file on blocksize < PAGE_SIZE setups.
- *
- */
-
-#include <linux/kernel.h>
-#include <linux/export.h>
-#include <linux/mm.h>
-#include <linux/kdev_t.h>
-#include <linux/gfp.h>
-#include <linux/bio.h>
-#include <linux/fs.h>
-#include <linux/buffer_head.h>
-#include <linux/blkdev.h>
-#include <linux/highmem.h>
-#include <linux/prefetch.h>
-#include <linux/mpage.h>
-#include <linux/writeback.h>
-#include <linux/backing-dev.h>
-#include <linux/pagevec.h>
-#include <linux/cleancache.h>
-
-#include "ext4.h"
-
-static inline bool ext4_bio_encrypted(struct bio *bio)
-{
-#ifdef CONFIG_FS_ENCRYPTION
-	return unlikely(bio->bi_private != NULL);
-#else
-	return false;
-#endif
-}
-
-/*
- * I/O completion handler for multipage BIOs.
- *
- * The mpage code never puts partial pages into a BIO (except for end-of-file).
- * If a page does not map to a contiguous run of blocks then it simply falls
- * back to block_read_full_page().
- *
- * Why is this?  If a page's completion depends on a number of different BIOs
- * which can complete in any order (or at the same time) then determining the
- * status of that page is hard.  See end_buffer_async_read() for the details.
- * There is no point in duplicating all that complexity.
- */
-static void mpage_end_io(struct bio *bio)
-{
-	struct bio_vec *bv;
-	int i;
-	struct bvec_iter_all iter_all;
-
-	if (ext4_bio_encrypted(bio)) {
-		if (bio->bi_status) {
-			fscrypt_release_ctx(bio->bi_private);
-		} else {
-			fscrypt_enqueue_decrypt_bio(bio->bi_private, bio);
-			return;
-		}
-	}
-	bio_for_each_segment_all(bv, bio, i, iter_all) {
-		struct page *page = bv->bv_page;
-
-		if (!bio->bi_status) {
-			SetPageUptodate(page);
-		} else {
-			ClearPageUptodate(page);
-			SetPageError(page);
-		}
-		unlock_page(page);
-	}
-
-	bio_put(bio);
-}
-
-int ext4_mpage_readpages(struct address_space *mapping,
-			 struct list_head *pages, struct page *page,
-			 unsigned nr_pages, bool is_readahead)
-{
-	struct bio *bio = NULL;
-	sector_t last_block_in_bio = 0;
-
-	struct inode *inode = mapping->host;
-	const unsigned blkbits = inode->i_blkbits;
-	const unsigned blocks_per_page = PAGE_SIZE >> blkbits;
-	const unsigned blocksize = 1 << blkbits;
-	sector_t block_in_file;
-	sector_t last_block;
-	sector_t last_block_in_file;
-	sector_t blocks[MAX_BUF_PER_PAGE];
-	unsigned page_block;
-	struct block_device *bdev = inode->i_sb->s_bdev;
-	int length;
-	unsigned relative_block = 0;
-	struct ext4_map_blocks map;
-
-	map.m_pblk = 0;
-	map.m_lblk = 0;
-	map.m_len = 0;
-	map.m_flags = 0;
-
-	for (; nr_pages; nr_pages--) {
-		int fully_mapped = 1;
-		unsigned first_hole = blocks_per_page;
-
-		prefetchw(&page->flags);
-		if (pages) {
-			page = lru_to_page(pages);
-			list_del(&page->lru);
-			if (add_to_page_cache_lru(page, mapping, page->index,
-				  readahead_gfp_mask(mapping)))
-				goto next_page;
-		}
-
-		if (page_has_buffers(page))
-			goto confused;
-
-		block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits);
-		last_block = block_in_file + nr_pages * blocks_per_page;
-		last_block_in_file = (i_size_read(inode) + blocksize - 1) >> blkbits;
-		if (last_block > last_block_in_file)
-			last_block = last_block_in_file;
-		page_block = 0;
-
-		/*
-		 * Map blocks using the previous result first.
-		 */
-		if ((map.m_flags & EXT4_MAP_MAPPED) &&
-		    block_in_file > map.m_lblk &&
-		    block_in_file < (map.m_lblk + map.m_len)) {
-			unsigned map_offset = block_in_file - map.m_lblk;
-			unsigned last = map.m_len - map_offset;
-
-			for (relative_block = 0; ; relative_block++) {
-				if (relative_block == last) {
-					/* needed? */
-					map.m_flags &= ~EXT4_MAP_MAPPED;
-					break;
-				}
-				if (page_block == blocks_per_page)
-					break;
-				blocks[page_block] = map.m_pblk + map_offset +
-					relative_block;
-				page_block++;
-				block_in_file++;
-			}
-		}
-
-		/*
-		 * Then do more ext4_map_blocks() calls until we are
-		 * done with this page.
-		 */
-		while (page_block < blocks_per_page) {
-			if (block_in_file < last_block) {
-				map.m_lblk = block_in_file;
-				map.m_len = last_block - block_in_file;
-
-				if (ext4_map_blocks(NULL, inode, &map, 0) < 0) {
-				set_error_page:
-					SetPageError(page);
-					zero_user_segment(page, 0,
-							  PAGE_SIZE);
-					unlock_page(page);
-					goto next_page;
-				}
-			}
-			if ((map.m_flags & EXT4_MAP_MAPPED) == 0) {
-				fully_mapped = 0;
-				if (first_hole == blocks_per_page)
-					first_hole = page_block;
-				page_block++;
-				block_in_file++;
-				continue;
-			}
-			if (first_hole != blocks_per_page)
-				goto confused;		/* hole -> non-hole */
-
-			/* Contiguous blocks? */
-			if (page_block && blocks[page_block-1] != map.m_pblk-1)
-				goto confused;
-			for (relative_block = 0; ; relative_block++) {
-				if (relative_block == map.m_len) {
-					/* needed? */
-					map.m_flags &= ~EXT4_MAP_MAPPED;
-					break;
-				} else if (page_block == blocks_per_page)
-					break;
-				blocks[page_block] = map.m_pblk+relative_block;
-				page_block++;
-				block_in_file++;
-			}
-		}
-		if (first_hole != blocks_per_page) {
-			zero_user_segment(page, first_hole << blkbits,
-					  PAGE_SIZE);
-			if (first_hole == 0) {
-				SetPageUptodate(page);
-				unlock_page(page);
-				goto next_page;
-			}
-		} else if (fully_mapped) {
-			SetPageMappedToDisk(page);
-		}
-		if (fully_mapped && blocks_per_page == 1 &&
-		    !PageUptodate(page) && cleancache_get_page(page) == 0) {
-			SetPageUptodate(page);
-			goto confused;
-		}
-
-		/*
-		 * This page will go to BIO.  Do we need to send this
-		 * BIO off first?
-		 */
-		if (bio && (last_block_in_bio != blocks[0] - 1)) {
-		submit_and_realloc:
-			submit_bio(bio);
-			bio = NULL;
-		}
-		if (bio == NULL) {
-			struct fscrypt_ctx *ctx = NULL;
-
-			if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode)) {
-				ctx = fscrypt_get_ctx(GFP_NOFS);
-				if (IS_ERR(ctx))
-					goto set_error_page;
-			}
-			bio = bio_alloc(GFP_KERNEL,
-				min_t(int, nr_pages, BIO_MAX_PAGES));
-			if (!bio) {
-				if (ctx)
-					fscrypt_release_ctx(ctx);
-				goto set_error_page;
-			}
-			bio_set_dev(bio, bdev);
-			bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9);
-			bio->bi_end_io = mpage_end_io;
-			bio->bi_private = ctx;
-			bio_set_op_attrs(bio, REQ_OP_READ,
-						is_readahead ? REQ_RAHEAD : 0);
-		}
-
-		length = first_hole << blkbits;
-		if (bio_add_page(bio, page, length, 0) < length)
-			goto submit_and_realloc;
-
-		if (((map.m_flags & EXT4_MAP_BOUNDARY) &&
-		     (relative_block == map.m_len)) ||
-		    (first_hole != blocks_per_page)) {
-			submit_bio(bio);
-			bio = NULL;
-		} else
-			last_block_in_bio = blocks[blocks_per_page - 1];
-		goto next_page;
-	confused:
-		if (bio) {
-			submit_bio(bio);
-			bio = NULL;
-		}
-		if (!PageUptodate(page))
-			block_read_full_page(page, ext4_get_block);
-		else
-			unlock_page(page);
-	next_page:
-		if (pages)
-			put_page(page);
-	}
-	BUG_ON(pages && !list_empty(pages));
-	if (bio)
-		submit_bio(bio);
-	return 0;
-}
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 7/8] ext4: Enable encryption for subpage-sized blocks
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
                   ` (5 preceding siblings ...)
  2019-08-16  6:18 ` [PATCH V4 6/8] ext4: Wire up ext4_readpage[s] to use mpage_readpage[s] Chandan Rajendra
@ 2019-08-16  6:18 ` Chandan Rajendra
  2019-08-16  6:18 ` [PATCH V4 8/8] fscrypt: remove struct fscrypt_ctx Chandan Rajendra
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:18 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

Now that we have the code to support encryption for subpage-sized
blocks, this commit removes the conditional check in filesystem mount
code.

The commit also changes the support statement in
Documentation/filesystems/fscrypt.rst to reflect the fact that
encryption of filesystems with blocksize less than page size now works.

Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
---
 Documentation/filesystems/fscrypt.rst | 4 ++--
 fs/ext4/super.c                       | 7 -------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst
index 08c23b60e016..c3efe86bf2b2 100644
--- a/Documentation/filesystems/fscrypt.rst
+++ b/Documentation/filesystems/fscrypt.rst
@@ -213,8 +213,8 @@ Contents encryption
 -------------------
 
 For file contents, each filesystem block is encrypted independently.
-Currently, only the case where the filesystem block size is equal to
-the system's page size (usually 4096 bytes) is supported.
+Starting from Linux kernel 5.4, encryption of filesystems with block
+size less than system's page size is supported.
 
 Each block's IV is set to the logical block number within the file as
 a little endian number, except that:
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 5b92054bf8ea..d580e71ad9c7 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4322,13 +4322,6 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		}
 	}
 
-	if ((DUMMY_ENCRYPTION_ENABLED(sbi) || ext4_has_feature_encrypt(sb)) &&
-	    (blocksize != PAGE_SIZE)) {
-		ext4_msg(sb, KERN_ERR,
-			 "Unsupported blocksize for fs encryption");
-		goto failed_mount_wq;
-	}
-
 	if (DUMMY_ENCRYPTION_ENABLED(sbi) && !sb_rdonly(sb) &&
 	    !ext4_has_feature_encrypt(sb)) {
 		ext4_set_feature_encrypt(sb);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH V4 8/8] fscrypt: remove struct fscrypt_ctx
  2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
                   ` (6 preceding siblings ...)
  2019-08-16  6:18 ` [PATCH V4 7/8] ext4: Enable encryption for subpage-sized blocks Chandan Rajendra
@ 2019-08-16  6:18 ` Chandan Rajendra
  7 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-16  6:18 UTC (permalink / raw)
  To: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt
  Cc: Chandan Rajendra, chandanrmail, tytso, adilger.kernel, ebiggers,
	jaegeuk, yuchao0, hch

Commit "fscrypt: remove the 'write' part of struct fscrypt_ctx" reduced
"struct fscrypt_ctx" to be used only for decryption. With "read
callbacks" being integrated into Ext4 and F2FS, we don't use "struct
fscrypt_ctx" anymore. Hence this commit removes the structure and the
associated code.

While at it, this commit also removes definitions of
__fscrypt_decrypt_bio() and fscrypt_decrypt_bio() since we have to now
use the APIs provided by read_callbacks facility.

Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
---
 fs/crypto/bio.c             | 43 ------------------
 fs/crypto/crypto.c          | 89 +------------------------------------
 fs/crypto/fscrypt_private.h |  3 --
 include/linux/fscrypt.h     | 38 ----------------
 4 files changed, 2 insertions(+), 171 deletions(-)

diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
index b4f47b98ee6d..f65cc3059e5c 100644
--- a/fs/crypto/bio.c
+++ b/fs/crypto/bio.c
@@ -26,49 +26,6 @@
 #include <linux/namei.h>
 #include "fscrypt_private.h"
 
-static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
-{
-	struct bio_vec *bv;
-	int i;
-	struct bvec_iter_all iter_all;
-
-	bio_for_each_segment_all(bv, bio, i, iter_all) {
-		struct page *page = bv->bv_page;
-		int ret = fscrypt_decrypt_pagecache_blocks(page, bv->bv_len,
-							   bv->bv_offset);
-		if (ret)
-			SetPageError(page);
-		else if (done)
-			SetPageUptodate(page);
-		if (done)
-			unlock_page(page);
-	}
-}
-
-void fscrypt_decrypt_bio(struct bio *bio)
-{
-	__fscrypt_decrypt_bio(bio, false);
-}
-EXPORT_SYMBOL(fscrypt_decrypt_bio);
-
-static void completion_pages(struct work_struct *work)
-{
-	struct fscrypt_ctx *ctx = container_of(work, struct fscrypt_ctx, work);
-	struct bio *bio = ctx->bio;
-
-	__fscrypt_decrypt_bio(bio, true);
-	fscrypt_release_ctx(ctx);
-	bio_put(bio);
-}
-
-void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx, struct bio *bio)
-{
-	INIT_WORK(&ctx->work, completion_pages);
-	ctx->bio = bio;
-	fscrypt_enqueue_decrypt_work(&ctx->work);
-}
-EXPORT_SYMBOL(fscrypt_enqueue_decrypt_bio);
-
 int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 				sector_t pblk, unsigned int len)
 {
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index dcf630d7e446..2e5245f5639f 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -31,24 +31,16 @@
 #include "fscrypt_private.h"
 
 static unsigned int num_prealloc_crypto_pages = 32;
-static unsigned int num_prealloc_crypto_ctxs = 128;
 
 module_param(num_prealloc_crypto_pages, uint, 0444);
 MODULE_PARM_DESC(num_prealloc_crypto_pages,
 		"Number of crypto pages to preallocate");
-module_param(num_prealloc_crypto_ctxs, uint, 0444);
-MODULE_PARM_DESC(num_prealloc_crypto_ctxs,
-		"Number of crypto contexts to preallocate");
 
 static mempool_t *fscrypt_bounce_page_pool = NULL;
 
-static LIST_HEAD(fscrypt_free_ctxs);
-static DEFINE_SPINLOCK(fscrypt_ctx_lock);
-
 static struct workqueue_struct *fscrypt_read_workqueue;
 static DEFINE_MUTEX(fscrypt_init_mutex);
 
-static struct kmem_cache *fscrypt_ctx_cachep;
 struct kmem_cache *fscrypt_info_cachep;
 
 void fscrypt_enqueue_decrypt_work(struct work_struct *work)
@@ -57,62 +49,6 @@ void fscrypt_enqueue_decrypt_work(struct work_struct *work)
 }
 EXPORT_SYMBOL(fscrypt_enqueue_decrypt_work);
 
-/**
- * fscrypt_release_ctx() - Release a decryption context
- * @ctx: The decryption context to release.
- *
- * If the decryption context was allocated from the pre-allocated pool, return
- * it to that pool.  Else, free it.
- */
-void fscrypt_release_ctx(struct fscrypt_ctx *ctx)
-{
-	unsigned long flags;
-
-	if (ctx->flags & FS_CTX_REQUIRES_FREE_ENCRYPT_FL) {
-		kmem_cache_free(fscrypt_ctx_cachep, ctx);
-	} else {
-		spin_lock_irqsave(&fscrypt_ctx_lock, flags);
-		list_add(&ctx->free_list, &fscrypt_free_ctxs);
-		spin_unlock_irqrestore(&fscrypt_ctx_lock, flags);
-	}
-}
-EXPORT_SYMBOL(fscrypt_release_ctx);
-
-/**
- * fscrypt_get_ctx() - Get a decryption context
- * @gfp_flags:   The gfp flag for memory allocation
- *
- * Allocate and initialize a decryption context.
- *
- * Return: A new decryption context on success; an ERR_PTR() otherwise.
- */
-struct fscrypt_ctx *fscrypt_get_ctx(gfp_t gfp_flags)
-{
-	struct fscrypt_ctx *ctx;
-	unsigned long flags;
-
-	/*
-	 * First try getting a ctx from the free list so that we don't have to
-	 * call into the slab allocator.
-	 */
-	spin_lock_irqsave(&fscrypt_ctx_lock, flags);
-	ctx = list_first_entry_or_null(&fscrypt_free_ctxs,
-					struct fscrypt_ctx, free_list);
-	if (ctx)
-		list_del(&ctx->free_list);
-	spin_unlock_irqrestore(&fscrypt_ctx_lock, flags);
-	if (!ctx) {
-		ctx = kmem_cache_zalloc(fscrypt_ctx_cachep, gfp_flags);
-		if (!ctx)
-			return ERR_PTR(-ENOMEM);
-		ctx->flags |= FS_CTX_REQUIRES_FREE_ENCRYPT_FL;
-	} else {
-		ctx->flags &= ~FS_CTX_REQUIRES_FREE_ENCRYPT_FL;
-	}
-	return ctx;
-}
-EXPORT_SYMBOL(fscrypt_get_ctx);
-
 struct page *fscrypt_alloc_bounce_page(gfp_t gfp_flags)
 {
 	return mempool_alloc(fscrypt_bounce_page_pool, gfp_flags);
@@ -399,11 +335,6 @@ const struct dentry_operations fscrypt_d_ops = {
 
 static void fscrypt_destroy(void)
 {
-	struct fscrypt_ctx *pos, *n;
-
-	list_for_each_entry_safe(pos, n, &fscrypt_free_ctxs, free_list)
-		kmem_cache_free(fscrypt_ctx_cachep, pos);
-	INIT_LIST_HEAD(&fscrypt_free_ctxs);
 	mempool_destroy(fscrypt_bounce_page_pool);
 	fscrypt_bounce_page_pool = NULL;
 }
@@ -419,7 +350,7 @@ static void fscrypt_destroy(void)
  */
 int fscrypt_initialize(unsigned int cop_flags)
 {
-	int i, res = -ENOMEM;
+	int res = -ENOMEM;
 
 	/* No need to allocate a bounce page pool if this FS won't use it. */
 	if (cop_flags & FS_CFLG_OWN_PAGES)
@@ -429,15 +360,6 @@ int fscrypt_initialize(unsigned int cop_flags)
 	if (fscrypt_bounce_page_pool)
 		goto already_initialized;
 
-	for (i = 0; i < num_prealloc_crypto_ctxs; i++) {
-		struct fscrypt_ctx *ctx;
-
-		ctx = kmem_cache_zalloc(fscrypt_ctx_cachep, GFP_NOFS);
-		if (!ctx)
-			goto fail;
-		list_add(&ctx->free_list, &fscrypt_free_ctxs);
-	}
-
 	fscrypt_bounce_page_pool =
 		mempool_create_page_pool(num_prealloc_crypto_pages, 0);
 	if (!fscrypt_bounce_page_pool)
@@ -492,18 +414,12 @@ static int __init fscrypt_init(void)
 	if (!fscrypt_read_workqueue)
 		goto fail;
 
-	fscrypt_ctx_cachep = KMEM_CACHE(fscrypt_ctx, SLAB_RECLAIM_ACCOUNT);
-	if (!fscrypt_ctx_cachep)
-		goto fail_free_queue;
-
 	fscrypt_info_cachep = KMEM_CACHE(fscrypt_info, SLAB_RECLAIM_ACCOUNT);
 	if (!fscrypt_info_cachep)
-		goto fail_free_ctx;
+		goto fail_free_queue;
 
 	return 0;
 
-fail_free_ctx:
-	kmem_cache_destroy(fscrypt_ctx_cachep);
 fail_free_queue:
 	destroy_workqueue(fscrypt_read_workqueue);
 fail:
@@ -520,7 +436,6 @@ static void __exit fscrypt_exit(void)
 
 	if (fscrypt_read_workqueue)
 		destroy_workqueue(fscrypt_read_workqueue);
-	kmem_cache_destroy(fscrypt_ctx_cachep);
 	kmem_cache_destroy(fscrypt_info_cachep);
 
 	fscrypt_essiv_cleanup();
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8565536feb2b..702403dc7614 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -93,9 +93,6 @@ typedef enum {
 	FS_ENCRYPT,
 } fscrypt_direction_t;
 
-#define FS_CTX_REQUIRES_FREE_ENCRYPT_FL		0x00000001
-#define FS_CTX_HAS_BOUNCE_BUFFER_FL		0x00000002
-
 static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
 					   u32 filenames_mode)
 {
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 4d6528351f25..ce3b15fd9fba 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -19,7 +19,6 @@
 
 #define FS_CRYPTO_BLOCK_SIZE		16
 
-struct fscrypt_ctx;
 struct fscrypt_info;
 
 struct fscrypt_str {
@@ -63,18 +62,6 @@ struct fscrypt_operations {
 	unsigned int max_namelen;
 };
 
-/* Decryption work */
-struct fscrypt_ctx {
-	union {
-		struct {
-			struct bio *bio;
-			struct work_struct work;
-		};
-		struct list_head free_list;	/* Free list */
-	};
-	u8 flags;				/* Flags */
-};
-
 static inline bool fscrypt_has_encryption_key(const struct inode *inode)
 {
 	/* pairs with cmpxchg_release() in fscrypt_get_encryption_info() */
@@ -101,8 +88,6 @@ static inline void fscrypt_handle_d_move(struct dentry *dentry)
 
 /* crypto.c */
 extern void fscrypt_enqueue_decrypt_work(struct work_struct *);
-extern struct fscrypt_ctx *fscrypt_get_ctx(gfp_t);
-extern void fscrypt_release_ctx(struct fscrypt_ctx *);
 
 extern struct page *fscrypt_encrypt_pagecache_blocks(struct page *page,
 						     unsigned int len,
@@ -232,9 +217,6 @@ static inline bool fscrypt_match_name(const struct fscrypt_name *fname,
 }
 
 /* bio.c */
-extern void fscrypt_decrypt_bio(struct bio *);
-extern void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx,
-					struct bio *bio);
 extern int fscrypt_zeroout_range(const struct inode *, pgoff_t, sector_t,
 				 unsigned int);
 
@@ -279,16 +261,6 @@ static inline void fscrypt_enqueue_decrypt_work(struct work_struct *work)
 {
 }
 
-static inline struct fscrypt_ctx *fscrypt_get_ctx(gfp_t gfp_flags)
-{
-	return ERR_PTR(-EOPNOTSUPP);
-}
-
-static inline void fscrypt_release_ctx(struct fscrypt_ctx *ctx)
-{
-	return;
-}
-
 static inline struct page *fscrypt_encrypt_pagecache_blocks(struct page *page,
 							    unsigned int len,
 							    unsigned int offs,
@@ -425,16 +397,6 @@ static inline bool fscrypt_match_name(const struct fscrypt_name *fname,
 	return !memcmp(de_name, fname->disk_name.name, fname->disk_name.len);
 }
 
-/* bio.c */
-static inline void fscrypt_decrypt_bio(struct bio *bio)
-{
-}
-
-static inline void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx,
-					       struct bio *bio)
-{
-}
-
 static inline int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
 					sector_t pblk, unsigned int len)
 {
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [f2fs-dev] [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-16  6:18 ` [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data Chandan Rajendra
@ 2019-08-18 13:45   ` Chao Yu
  2019-08-19 13:33     ` Chandan Rajendra
  2019-08-20  5:05   ` Chandan Rajendra
  1 sibling, 1 reply; 20+ messages in thread
From: Chao Yu @ 2019-08-18 13:45 UTC (permalink / raw)
  To: Chandan Rajendra, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-fscrypt
  Cc: hch, tytso, ebiggers, adilger.kernel, chandanrmail, jaegeuk

Hi Chandan,

On 2019-8-16 14:18, Chandan Rajendra wrote:
> F2FS has a copy of "post read processing" code using which encrypted
> file data is decrypted. This commit replaces it to make use of the
> generic read_callbacks facility.

I remember that previously Jaegeuk had mentioned f2fs will support compression
later, and it needs to reuse 'post read processing' fwk.

There is very initial version of compression feature in below link:

https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=compression

So my concern is how can we uplift the most common parts of this fwk into vfs,
and meanwhile keeping the ability and flexibility when introducing private
feature/step in specified filesytem(now f2fs)?

According to current f2fs compression's requirement, maybe we can expand to

- support callback to let filesystem set the function for the flow in
decompression/verity/decryption step.
- support to use individual/common workqueue according the parameter.

Any thoughts?

Thanks,

> 
> Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
> ---
>  fs/f2fs/data.c  | 109 ++++--------------------------------------------
>  fs/f2fs/f2fs.h  |   2 -
>  fs/f2fs/super.c |   9 +---
>  3 files changed, 11 insertions(+), 109 deletions(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 757f050c650a..3cf1eca2ece9 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -18,6 +18,7 @@
>  #include <linux/uio.h>
>  #include <linux/cleancache.h>
>  #include <linux/sched/signal.h>
> +#include <linux/read_callbacks.h>
>  
>  #include "f2fs.h"
>  #include "node.h"
> @@ -25,11 +26,6 @@
>  #include "trace.h"
>  #include <trace/events/f2fs.h>
>  
> -#define NUM_PREALLOC_POST_READ_CTXS	128
> -
> -static struct kmem_cache *bio_post_read_ctx_cache;
> -static mempool_t *bio_post_read_ctx_pool;
> -
>  static bool __is_cp_guaranteed(struct page *page)
>  {
>  	struct address_space *mapping = page->mapping;
> @@ -69,19 +65,6 @@ static enum count_type __read_io_type(struct page *page)
>  	return F2FS_RD_DATA;
>  }
>  
> -/* postprocessing steps for read bios */
> -enum bio_post_read_step {
> -	STEP_INITIAL = 0,
> -	STEP_DECRYPT,
> -};
> -
> -struct bio_post_read_ctx {
> -	struct bio *bio;
> -	struct work_struct work;
> -	unsigned int cur_step;
> -	unsigned int enabled_steps;
> -};
> -
>  static void __read_end_io(struct bio *bio)
>  {
>  	struct page *page;
> @@ -93,7 +76,7 @@ static void __read_end_io(struct bio *bio)
>  		page = bv->bv_page;
>  
>  		/* PG_error was set if any post_read step failed */
> -		if (bio->bi_status || PageError(page)) {
> +		if (bio->bi_status || read_callbacks_failed(page)) {
>  			ClearPageUptodate(page);
>  			/* will re-read again later */
>  			ClearPageError(page);
> @@ -103,42 +86,8 @@ static void __read_end_io(struct bio *bio)
>  		dec_page_count(F2FS_P_SB(page), __read_io_type(page));
>  		unlock_page(page);
>  	}
> -	if (bio->bi_private)
> -		mempool_free(bio->bi_private, bio_post_read_ctx_pool);
> -	bio_put(bio);
> -}
>  
> -static void bio_post_read_processing(struct bio_post_read_ctx *ctx);
> -
> -static void decrypt_work(struct work_struct *work)
> -{
> -	struct bio_post_read_ctx *ctx =
> -		container_of(work, struct bio_post_read_ctx, work);
> -
> -	fscrypt_decrypt_bio(ctx->bio);
> -
> -	bio_post_read_processing(ctx);
> -}
> -
> -static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
> -{
> -	switch (++ctx->cur_step) {
> -	case STEP_DECRYPT:
> -		if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
> -			INIT_WORK(&ctx->work, decrypt_work);
> -			fscrypt_enqueue_decrypt_work(&ctx->work);
> -			return;
> -		}
> -		ctx->cur_step++;
> -		/* fall-through */
> -	default:
> -		__read_end_io(ctx->bio);
> -	}
> -}
> -
> -static bool f2fs_bio_post_read_required(struct bio *bio)
> -{
> -	return bio->bi_private && !bio->bi_status;
> +	bio_put(bio);
>  }
>  
>  static void f2fs_read_end_io(struct bio *bio)
> @@ -149,15 +98,7 @@ static void f2fs_read_end_io(struct bio *bio)
>  		bio->bi_status = BLK_STS_IOERR;
>  	}
>  
> -	if (f2fs_bio_post_read_required(bio)) {
> -		struct bio_post_read_ctx *ctx = bio->bi_private;
> -
> -		ctx->cur_step = STEP_INITIAL;
> -		bio_post_read_processing(ctx);
> -		return;
> -	}
> -
> -	__read_end_io(bio);
> +	read_callbacks_endio_bio(bio, __read_end_io);
>  }
>  
>  static void f2fs_write_end_io(struct bio *bio)
> @@ -556,8 +497,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 err;
>  
>  	if (!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC))
>  		return ERR_PTR(-EFAULT);
> @@ -569,17 +509,10 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
>  	bio->bi_end_io = f2fs_read_end_io;
>  	bio_set_op_attrs(bio, REQ_OP_READ, op_flag);
>  
> -	if (f2fs_encrypted_file(inode))
> -		post_read_steps |= 1 << STEP_DECRYPT;
> -	if (post_read_steps) {
> -		ctx = mempool_alloc(bio_post_read_ctx_pool, GFP_NOFS);
> -		if (!ctx) {
> -			bio_put(bio);
> -			return ERR_PTR(-ENOMEM);
> -		}
> -		ctx->bio = bio;
> -		ctx->enabled_steps = post_read_steps;
> -		bio->bi_private = ctx;
> +	err = read_callbacks_setup_bio(inode, bio);
> +	if (err) {
> +		bio_put(bio);
> +		return ERR_PTR(err);
>  	}
>  
>  	return bio;
> @@ -2860,27 +2793,3 @@ void f2fs_clear_page_cache_dirty_tag(struct page *page)
>  						PAGECACHE_TAG_DIRTY);
>  	xa_unlock_irqrestore(&mapping->i_pages, flags);
>  }
> -
> -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);
> -}
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 87f75ebd2fd6..cea79321b794 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3125,8 +3125,6 @@ void f2fs_destroy_checkpoint_caches(void);
>  /*
>   * data.c
>   */
> -int f2fs_init_post_read_processing(void);
> -void f2fs_destroy_post_read_processing(void);
>  void f2fs_submit_merged_write(struct f2fs_sb_info *sbi, enum page_type type);
>  void f2fs_submit_merged_write_cond(struct f2fs_sb_info *sbi,
>  				struct inode *inode, struct page *page,
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 11b3a039a188..d7bbb4f1fdb3 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3597,15 +3597,11 @@ static int __init init_f2fs_fs(void)
>  	err = register_filesystem(&f2fs_fs_type);
>  	if (err)
>  		goto free_shrinker;
> +
>  	f2fs_create_root_stats();
> -	err = f2fs_init_post_read_processing();
> -	if (err)
> -		goto free_root_stats;
> +
>  	return 0;
>  
> -free_root_stats:
> -	f2fs_destroy_root_stats();
> -	unregister_filesystem(&f2fs_fs_type);
>  free_shrinker:
>  	unregister_shrinker(&f2fs_shrinker_info);
>  free_sysfs:
> @@ -3626,7 +3622,6 @@ static int __init init_f2fs_fs(void)
>  
>  static void __exit exit_f2fs_fs(void)
>  {
> -	f2fs_destroy_post_read_processing();
>  	f2fs_destroy_root_stats();
>  	unregister_filesystem(&f2fs_fs_type);
>  	unregister_shrinker(&f2fs_shrinker_info);
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [f2fs-dev] [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-18 13:45   ` [f2fs-dev] " Chao Yu
@ 2019-08-19 13:33     ` Chandan Rajendra
  2019-08-20  7:43       ` Chao Yu
  0 siblings, 1 reply; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-19 13:33 UTC (permalink / raw)
  To: Chao Yu
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt, hch,
	tytso, ebiggers, adilger.kernel, chandanrmail, jaegeuk

On Sunday, August 18, 2019 7:15:42 PM IST Chao Yu wrote:
> Hi Chandan,
> 
> On 2019-8-16 14:18, Chandan Rajendra wrote:
> > F2FS has a copy of "post read processing" code using which encrypted
> > file data is decrypted. This commit replaces it to make use of the
> > generic read_callbacks facility.
> 
> I remember that previously Jaegeuk had mentioned f2fs will support compression
> later, and it needs to reuse 'post read processing' fwk.
> 
> There is very initial version of compression feature in below link:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=compression
> 
> So my concern is how can we uplift the most common parts of this fwk into vfs,
> and meanwhile keeping the ability and flexibility when introducing private
> feature/step in specified filesytem(now f2fs)?
> 
> According to current f2fs compression's requirement, maybe we can expand to
> 
> - support callback to let filesystem set the function for the flow in
> decompression/verity/decryption step.
> - support to use individual/common workqueue according the parameter.
> 
> Any thoughts?
>

Hi,

F2FS can be made to use fscrypt's queue for decryption and hence can reuse
"read callbacks" code for decrypting data.

For decompression, we could have a STEP_MISC where we invoke a FS provided
callback function for FS specific post read processing? 

Something like the following can be implemented in read_callbacks(),
	  case STEP_MISC:
		  if (ctx->enabled_steps & (1 << STEP_MISC)) {
			  /*
			    ctx->fs_misc() must process bio in a workqueue
			    and later invoke read_callbacks() with
			    bio->bi_private's value as an argument.
			  */
			  ctx->fs_misc(ctx->bio);
			  return;
		  }
		  ctx->cur_step++;

The fs_misc() callback can be passed in by the filesystem when invoking
read_callbacks_setup_bio().

-- 
chandan




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-16  6:18 ` [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data Chandan Rajendra
  2019-08-18 13:45   ` [f2fs-dev] " Chao Yu
@ 2019-08-20  5:05   ` Chandan Rajendra
  2019-08-20  5:12     ` Gao Xiang
  2019-08-20 16:38     ` Theodore Y. Ts'o
  1 sibling, 2 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-20  5:05 UTC (permalink / raw)
  To: tytso, ebiggers
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt,
	chandanrmail, adilger.kernel, jaegeuk, yuchao0, hch

On Friday, August 16, 2019 11:48 AM Chandan Rajendra wrote:
> F2FS has a copy of "post read processing" code using which encrypted
> file data is decrypted. This commit replaces it to make use of the
> generic read_callbacks facility.
> 
> Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>

Hi Eric and Ted,

Looks like F2FS requires a lot more flexiblity than what can be offered by
read callbacks i.e.

1. F2FS wants to make use of its own workqueue for decryption, verity and
   decompression.
2. F2FS' decompression code is not an FS independent entity like fscrypt and
   fsverity. Hence they would need Filesystem specific callback functions to
   be invoked from "read callbacks". 

Hence I would suggest that we should drop F2FS changes made in this
patchset. Please let me know your thoughts on this.

-- 
chandan




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20  5:05   ` Chandan Rajendra
@ 2019-08-20  5:12     ` Gao Xiang
  2019-08-20  5:16       ` Gao Xiang
  2019-08-20 16:25       ` Theodore Y. Ts'o
  2019-08-20 16:38     ` Theodore Y. Ts'o
  1 sibling, 2 replies; 20+ messages in thread
From: Gao Xiang @ 2019-08-20  5:12 UTC (permalink / raw)
  To: Chandan Rajendra
  Cc: tytso, ebiggers, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-fscrypt, chandanrmail, adilger.kernel, jaegeuk, yuchao0,
	hch

Hi Chandan,

On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote:
> On Friday, August 16, 2019 11:48 AM Chandan Rajendra wrote:
> > F2FS has a copy of "post read processing" code using which encrypted
> > file data is decrypted. This commit replaces it to make use of the
> > generic read_callbacks facility.
> > 
> > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
> 
> Hi Eric and Ted,
> 
> Looks like F2FS requires a lot more flexiblity than what can be offered by
> read callbacks i.e.
> 
> 1. F2FS wants to make use of its own workqueue for decryption, verity and
>    decompression.
> 2. F2FS' decompression code is not an FS independent entity like fscrypt and
>    fsverity. Hence they would need Filesystem specific callback functions to
>    be invoked from "read callbacks". 
> 
> Hence I would suggest that we should drop F2FS changes made in this
> patchset. Please let me know your thoughts on this.

Add a word, I have some little concern about post read procession order
a bit as I mentioned before, because I'd like to move common EROFS
decompression code out in the future as well for other fses to use
after we think it's mature enough.

It seems the current code mainly addresses eliminating duplicated code,
therefore I have no idea about that...

Thanks,
Gao Xiang

> 
> -- 
> chandan
> 
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20  5:12     ` Gao Xiang
@ 2019-08-20  5:16       ` Gao Xiang
  2019-08-20 16:25       ` Theodore Y. Ts'o
  1 sibling, 0 replies; 20+ messages in thread
From: Gao Xiang @ 2019-08-20  5:16 UTC (permalink / raw)
  To: Chandan Rajendra
  Cc: tytso, ebiggers, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-fscrypt, chandanrmail, adilger.kernel, jaegeuk, yuchao0,
	hch

On Tue, Aug 20, 2019 at 01:12:36PM +0800, Gao Xiang wrote:
> Hi Chandan,
> 
> On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote:
> > On Friday, August 16, 2019 11:48 AM Chandan Rajendra wrote:
> > > F2FS has a copy of "post read processing" code using which encrypted
> > > file data is decrypted. This commit replaces it to make use of the
> > > generic read_callbacks facility.
> > > 
> > > Signed-off-by: Chandan Rajendra <chandan@linux.ibm.com>
> > 
> > Hi Eric and Ted,
> > 
> > Looks like F2FS requires a lot more flexiblity than what can be offered by
> > read callbacks i.e.
> > 
> > 1. F2FS wants to make use of its own workqueue for decryption, verity and
> >    decompression.
> > 2. F2FS' decompression code is not an FS independent entity like fscrypt and
> >    fsverity. Hence they would need Filesystem specific callback functions to
> >    be invoked from "read callbacks". 
> > 
> > Hence I would suggest that we should drop F2FS changes made in this
> > patchset. Please let me know your thoughts on this.
> 
> Add a word, I have some little concern about post read procession order

FYI. Just a minor concern about its flexibility, not big though.
https://lore.kernel.org/r/20190808042640.GA28630@138/

Thanks,
Gao Xiang

> a bit as I mentioned before, because I'd like to move common EROFS
> decompression code out in the future as well for other fses to use
> after we think it's mature enough.
> 
> It seems the current code mainly addresses eliminating duplicated code,
> therefore I have no idea about that...
> 
> Thanks,
> Gao Xiang
> 
> > 
> > -- 
> > chandan
> > 
> > 
> > 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [f2fs-dev] [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-19 13:33     ` Chandan Rajendra
@ 2019-08-20  7:43       ` Chao Yu
  0 siblings, 0 replies; 20+ messages in thread
From: Chao Yu @ 2019-08-20  7:43 UTC (permalink / raw)
  To: Chandan Rajendra, Chao Yu
  Cc: linux-fsdevel, linux-ext4, linux-f2fs-devel, linux-fscrypt, hch,
	tytso, ebiggers, adilger.kernel, chandanrmail, jaegeuk

On 2019/8/19 21:33, Chandan Rajendra wrote:
> On Sunday, August 18, 2019 7:15:42 PM IST Chao Yu wrote:
>> Hi Chandan,
>>
>> On 2019-8-16 14:18, Chandan Rajendra wrote:
>>> F2FS has a copy of "post read processing" code using which encrypted
>>> file data is decrypted. This commit replaces it to make use of the
>>> generic read_callbacks facility.
>>
>> I remember that previously Jaegeuk had mentioned f2fs will support compression
>> later, and it needs to reuse 'post read processing' fwk.
>>
>> There is very initial version of compression feature in below link:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/chao/linux.git/log/?h=compression
>>
>> So my concern is how can we uplift the most common parts of this fwk into vfs,
>> and meanwhile keeping the ability and flexibility when introducing private
>> feature/step in specified filesytem(now f2fs)?
>>
>> According to current f2fs compression's requirement, maybe we can expand to
>>
>> - support callback to let filesystem set the function for the flow in
>> decompression/verity/decryption step.
>> - support to use individual/common workqueue according the parameter.
>>
>> Any thoughts?
>>
> 
> Hi,
> 
> F2FS can be made to use fscrypt's queue for decryption and hence can reuse
> "read callbacks" code for decrypting data.
> 
> For decompression, we could have a STEP_MISC where we invoke a FS provided
> callback function for FS specific post read processing? 
> 
> Something like the following can be implemented in read_callbacks(),
> 	  case STEP_MISC:
> 		  if (ctx->enabled_steps & (1 << STEP_MISC)) {
> 			  /*
> 			    ctx->fs_misc() must process bio in a workqueue
> 			    and later invoke read_callbacks() with
> 			    bio->bi_private's value as an argument.
> 			  */
> 			  ctx->fs_misc(ctx->bio);
> 			  return;
> 		  }
> 		  ctx->cur_step++;
> 
> The fs_misc() callback can be passed in by the filesystem when invoking
> read_callbacks_setup_bio().

Hi,

Yes, something like this, can we just use STEP_DECOMPRESS and fs_decompress(),
not sure, I doubt this interface may has potential user which has compression
feature.

One more concern is that to avoid more context switch, maybe we can merge all
background works into one workqueue if there is no conflict when call wants to.

static void bio_post_read_processing(struct bio_post_read_ctx *ctx)
 {
	switch (++ctx->cur_step) {
	case STEP_DECRYPT:
		if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
...
			if (ctx->separated_wq)
				return;
		}
		ctx->cur_step++;
		/* fall-through */
	case STEP_DECOMPRESS:
...
	default:
		__read_end_io(ctx->bio);

> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20  5:12     ` Gao Xiang
  2019-08-20  5:16       ` Gao Xiang
@ 2019-08-20 16:25       ` Theodore Y. Ts'o
  2019-08-20 17:07         ` Gao Xiang
  1 sibling, 1 reply; 20+ messages in thread
From: Theodore Y. Ts'o @ 2019-08-20 16:25 UTC (permalink / raw)
  To: Gao Xiang
  Cc: Chandan Rajendra, ebiggers, linux-fsdevel, linux-ext4,
	linux-f2fs-devel, linux-fscrypt, chandanrmail, adilger.kernel,
	jaegeuk, yuchao0, hch

On Tue, Aug 20, 2019 at 01:12:36PM +0800, Gao Xiang wrote:
> Add a word, I have some little concern about post read procession order
> a bit as I mentioned before, because I'd like to move common EROFS
> decompression code out in the future as well for other fses to use
> after we think it's mature enough.
> 
> It seems the current code mainly addresses eliminating duplicated code,
> therefore I have no idea about that...

Actually, we should chat.  I was actually thinking about "borrowing"
code from erofs to provide ext4-specific compression.  I was really
impressed with the efficiency goals in the erofs design[1] when I
reviewed the Usenix ATC paper, and as the saying goes, the best
artists know how to steal from the best.  :-)

[1] https://www.usenix.org/conference/atc19/presentation/gao

My original specific thinking was to do code reuse by copy and paste,
simply because it was simpler, and I have limited time to work on it.
But if you are interested in making the erofs pipeline reusable by
other file systems, and have the time to do the necessary code
refactoring, I'd love to work with you on that.

It should be noted that the f2fs developers have been working on their
own compression scheme that was going to be f2fs-specific, unlike the
file system generic approach used with fsverity and fscrypt.

My expectation is that we will need to modify the read pipeling code
to support compression.  That's true whether we are looking at the
existing file system-specific code used by ext4 and f2fs or in some
combined work such as what Chandan has proposed.

Cheers,

					- Ted

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20  5:05   ` Chandan Rajendra
  2019-08-20  5:12     ` Gao Xiang
@ 2019-08-20 16:38     ` Theodore Y. Ts'o
  2019-08-20 17:31       ` Jaegeuk Kim
  1 sibling, 1 reply; 20+ messages in thread
From: Theodore Y. Ts'o @ 2019-08-20 16:38 UTC (permalink / raw)
  To: Chandan Rajendra
  Cc: ebiggers, linux-fsdevel, linux-ext4, linux-f2fs-devel,
	linux-fscrypt, chandanrmail, adilger.kernel, jaegeuk, yuchao0,
	hch

On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote:
> Looks like F2FS requires a lot more flexiblity than what can be offered by
> read callbacks i.e.
> 
> 1. F2FS wants to make use of its own workqueue for decryption, verity and
>    decompression.
> 2. F2FS' decompression code is not an FS independent entity like fscrypt and
>    fsverity. Hence they would need Filesystem specific callback functions to
>    be invoked from "read callbacks". 
> 
> Hence I would suggest that we should drop F2FS changes made in this
> patchset. Please let me know your thoughts on this.

That's probably the best way to go for now.  My one concern is that it
means that only ext4 will be using your framework.  I could imagine
that some people might argue that should just move the callback scheme
into ext4 code as opposed to leaving it in fscrypt --- at least until
we can find other file systems where we can show that it will be
useful for those other file systems.

(Perhaps a useful experiment would be to have someone implement patches
to support fscrypt and fsverity in ext2 --- the patch might or might
not be accepted for upstream inclusion, but it would be useful to
demonstrate how easy it is to add fscrypt and fsverity.)

The other thing to consider is that there has been some discussion
about adding generalized support for I/O submission to the iomap
library.  It might be that if that work is accepted, support for
fscrypt and fsverity would be a requirement for ext4 to use that
portion of iomap's functionality.  So in that eventuality, it might be
that we'll want to move your read callbacks code into iomap, or we'll
need to rework the read callbacks code so it can work with iomap.

But this is all work for the future.  I'm a firm believe that the
perfect should not be the enemy of the good, and that none of this
should be a fundamental obstacle in having your code upstream.

Cheers,

					- Ted



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20 16:25       ` Theodore Y. Ts'o
@ 2019-08-20 17:07         ` Gao Xiang
  0 siblings, 0 replies; 20+ messages in thread
From: Gao Xiang @ 2019-08-20 17:07 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Gao Xiang, Chandan Rajendra, ebiggers, linux-fsdevel, linux-ext4,
	linux-f2fs-devel, linux-fscrypt, chandanrmail, adilger.kernel,
	jaegeuk, yuchao0, hch

Hi Ted,

On Tue, Aug 20, 2019 at 12:25:10PM -0400, Theodore Y. Ts'o wrote:
> On Tue, Aug 20, 2019 at 01:12:36PM +0800, Gao Xiang wrote:
> > Add a word, I have some little concern about post read procession order
> > a bit as I mentioned before, because I'd like to move common EROFS
> > decompression code out in the future as well for other fses to use
> > after we think it's mature enough.
> > 
> > It seems the current code mainly addresses eliminating duplicated code,
> > therefore I have no idea about that...
> 
> Actually, we should chat.  I was actually thinking about "borrowing"
> code from erofs to provide ext4-specific compression.  I was really
> impressed with the efficiency goals in the erofs design[1] when I
> reviewed the Usenix ATC paper, and as the saying goes, the best
> artists know how to steal from the best.  :-)
> 
> [1] https://www.usenix.org/conference/atc19/presentation/gao

I also guessed it's you reviewed our work as well from some written words :)
(even though it's analymous...) and I personally think there are some
useful stuffs in our EROFS effort.

> 
> My original specific thinking was to do code reuse by copy and paste,
> simply because it was simpler, and I have limited time to work on it.
> But if you are interested in making the erofs pipeline reusable by
> other file systems, and have the time to do the necessary code
> refactoring, I'd love to work with you on that.

Yes, I have interest in making the erofs pipeline for generic fses.
Now I'm still investigating sequential read on very high speed NVME
(like SAMSUNG 970pro, one thread seq read >3GB/s), it seems it still
has some optimization space.

And then I will do that work for generic fses as well... (but the first
thing I want to do is getting erofs out of staging, as Greg said [1])

Metadata should be designed for each fs like ext4, but maybe not flexible
and compacted as EROFS, therefore it could be some extra metadata
overhead than EROFS.

[1] https://lore.kernel.org/lkml/20190618064523.GA6015@kroah.com/

> 
> It should be noted that the f2fs developers have been working on their
> own compression scheme that was going to be f2fs-specific, unlike the
> file system generic approach used with fsverity and fscrypt.
> 
> My expectation is that we will need to modify the read pipeling code
> to support compression.  That's true whether we are looking at the
> existing file system-specific code used by ext4 and f2fs or in some
> combined work such as what Chandan has proposed.

I think either form is fine with me. :) But it seems that is some minor
which tree we will work on (Maybe Chandan's work will be merged then).

The first thing I need to do is to tidy up the code, and making it more
general, and then it will be very easy for fses to integrate :)

Thanks,
Gao Xiang


> 
> Cheers,
> 
> 					- Ted

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20 16:38     ` Theodore Y. Ts'o
@ 2019-08-20 17:31       ` Jaegeuk Kim
  2019-08-21  2:04         ` Chandan Rajendra
  0 siblings, 1 reply; 20+ messages in thread
From: Jaegeuk Kim @ 2019-08-20 17:31 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Chandan Rajendra, ebiggers, linux-fsdevel, linux-ext4,
	linux-f2fs-devel, linux-fscrypt, chandanrmail, adilger.kernel,
	yuchao0, hch

Hi Chandan,

On 08/20, Theodore Y. Ts'o wrote:
> On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote:
> > Looks like F2FS requires a lot more flexiblity than what can be offered by
> > read callbacks i.e.
> > 
> > 1. F2FS wants to make use of its own workqueue for decryption, verity and
> >    decompression.
> > 2. F2FS' decompression code is not an FS independent entity like fscrypt and
> >    fsverity. Hence they would need Filesystem specific callback functions to
> >    be invoked from "read callbacks". 
> > 
> > Hence I would suggest that we should drop F2FS changes made in this
> > patchset. Please let me know your thoughts on this.
> 
> That's probably the best way to go for now.  My one concern is that it
> means that only ext4 will be using your framework.  I could imagine
> that some people might argue that should just move the callback scheme
> into ext4 code as opposed to leaving it in fscrypt --- at least until
> we can find other file systems where we can show that it will be
> useful for those other file systems.

I also have to raise a flag on this. Doesn't this patch series try to get rid
of redundant work? What'd be the rationale, if it only supports ext4?

How about generalizing the framework to support generic_post_read and per-fs
post_read for fscrypt/fsverity/... selectively?

Thanks,

> 
> (Perhaps a useful experiment would be to have someone implement patches
> to support fscrypt and fsverity in ext2 --- the patch might or might
> not be accepted for upstream inclusion, but it would be useful to
> demonstrate how easy it is to add fscrypt and fsverity.)
> 
> The other thing to consider is that there has been some discussion
> about adding generalized support for I/O submission to the iomap
> library.  It might be that if that work is accepted, support for
> fscrypt and fsverity would be a requirement for ext4 to use that
> portion of iomap's functionality.  So in that eventuality, it might be
> that we'll want to move your read callbacks code into iomap, or we'll
> need to rework the read callbacks code so it can work with iomap.
> 
> But this is all work for the future.  I'm a firm believe that the
> perfect should not be the enemy of the good, and that none of this
> should be a fundamental obstacle in having your code upstream.
> 
> Cheers,
> 
> 					- Ted
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data
  2019-08-20 17:31       ` Jaegeuk Kim
@ 2019-08-21  2:04         ` Chandan Rajendra
  0 siblings, 0 replies; 20+ messages in thread
From: Chandan Rajendra @ 2019-08-21  2:04 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Theodore Y. Ts'o, ebiggers, linux-fsdevel, linux-ext4,
	linux-f2fs-devel, linux-fscrypt, chandanrmail, adilger.kernel,
	yuchao0, hch

On Tuesday, August 20, 2019 11:01 PM Jaegeuk Kim wrote:
> Hi Chandan,
> 
> On 08/20, Theodore Y. Ts'o wrote:
> > On Tue, Aug 20, 2019 at 10:35:29AM +0530, Chandan Rajendra wrote:
> > > Looks like F2FS requires a lot more flexiblity than what can be offered by
> > > read callbacks i.e.
> > > 
> > > 1. F2FS wants to make use of its own workqueue for decryption, verity and
> > >    decompression.
> > > 2. F2FS' decompression code is not an FS independent entity like fscrypt and
> > >    fsverity. Hence they would need Filesystem specific callback functions to
> > >    be invoked from "read callbacks". 
> > > 
> > > Hence I would suggest that we should drop F2FS changes made in this
> > > patchset. Please let me know your thoughts on this.
> > 
> > That's probably the best way to go for now.  My one concern is that it
> > means that only ext4 will be using your framework.  I could imagine
> > that some people might argue that should just move the callback scheme
> > into ext4 code as opposed to leaving it in fscrypt --- at least until
> > we can find other file systems where we can show that it will be
> > useful for those other file systems.
> 
> I also have to raise a flag on this. Doesn't this patch series try to get rid
> of redundant work? What'd be the rationale, if it only supports ext4?

This patchset gets encryption working with subpage blocksize by making
relevant changes in the generic code (i.e. do_mpage_readpage() and
block_read_full_page()) and removing duplicate code from ext4
(i.e. ext4_readpage() and friends). Without these changes the only way to get
subpage blocksize support was to add more duplicate code into Ext4 i.e. import
a copy of block_read_full_page() into Ext4 and make necessary edits to support
encryption.

So this patchset actually does help in removing exiting duplicate code in Ext4
and also prevents addition of more such code.

> 
> How about generalizing the framework to support generic_post_read and per-fs
> post_read for fscrypt/fsverity/... selectively?

Quoting what I had said earlier,
> > > 1. F2FS wants to make use of its own workqueue for decryption, verity and
> > >    decompression.
> > > 2. F2FS' decompression code is not an FS independent entity like fscrypt and
> > >    fsverity. Hence they would need Filesystem specific callback functions to
> > >    be invoked from "read callbacks". 

I am not sure if read callbacks can be made flexible enough to support the
above use cases. fscrypt and fsverity already provide workqueues and any new
post processing code added should follow the same convention. I see
that F2FS use case is special since,
1. It uses its own workqueues.
2. Decompression code inside F2FS isn't written as an FS independent subsystem
   like how fscrypt and fsverity are implemented.

To summarize, I believe the users of read callbacks should follow the
conventions set by fscrypt/fsverity and new post processing code that needs to
be plugged into read callbacks should provide APIs similar to
fscrypt/fsverity. Otherwise the state machine logic implemented by read
callbacks will get complex/convoluted.

> 
> Thanks,
> 
> > 
> > (Perhaps a useful experiment would be to have someone implement patches
> > to support fscrypt and fsverity in ext2 --- the patch might or might
> > not be accepted for upstream inclusion, but it would be useful to
> > demonstrate how easy it is to add fscrypt and fsverity.)
> > 
> > The other thing to consider is that there has been some discussion
> > about adding generalized support for I/O submission to the iomap
> > library.  It might be that if that work is accepted, support for
> > fscrypt and fsverity would be a requirement for ext4 to use that
> > portion of iomap's functionality.  So in that eventuality, it might be
> > that we'll want to move your read callbacks code into iomap, or we'll
> > need to rework the read callbacks code so it can work with iomap.
> > 
> > But this is all work for the future.  I'm a firm believe that the
> > perfect should not be the enemy of the good, and that none of this
> > should be a fundamental obstacle in having your code upstream.
> > 
> > Cheers,
> > 
> > 					- Ted
> > 
> 

-- 
chandan




^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-08-21  2:02 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16  6:17 [PATCH V4 0/8] Consolidate FS read I/O callbacks code Chandan Rajendra
2019-08-16  6:17 ` [PATCH V4 1/8] buffer_head: Introduce BH_Read_Cb flag Chandan Rajendra
2019-08-16  6:17 ` [PATCH V4 2/8] FS: Introduce read callbacks Chandan Rajendra
2019-08-16  6:17 ` [PATCH V4 3/8] fs/mpage.c: Integrate " Chandan Rajendra
2019-08-16  6:18 ` [PATCH V4 4/8] fs/buffer.c: add decryption support via read_callbacks Chandan Rajendra
2019-08-16  6:18 ` [PATCH V4 5/8] f2fs: Use read_callbacks for decrypting file data Chandan Rajendra
2019-08-18 13:45   ` [f2fs-dev] " Chao Yu
2019-08-19 13:33     ` Chandan Rajendra
2019-08-20  7:43       ` Chao Yu
2019-08-20  5:05   ` Chandan Rajendra
2019-08-20  5:12     ` Gao Xiang
2019-08-20  5:16       ` Gao Xiang
2019-08-20 16:25       ` Theodore Y. Ts'o
2019-08-20 17:07         ` Gao Xiang
2019-08-20 16:38     ` Theodore Y. Ts'o
2019-08-20 17:31       ` Jaegeuk Kim
2019-08-21  2:04         ` Chandan Rajendra
2019-08-16  6:18 ` [PATCH V4 6/8] ext4: Wire up ext4_readpage[s] to use mpage_readpage[s] Chandan Rajendra
2019-08-16  6:18 ` [PATCH V4 7/8] ext4: Enable encryption for subpage-sized blocks Chandan Rajendra
2019-08-16  6:18 ` [PATCH V4 8/8] fscrypt: remove struct fscrypt_ctx Chandan Rajendra

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).