Re: [f2fs-dev] [PATCH V3 1/7] FS: Introduce read callbacks
On Saturday, June 22, 2019 1:33:57 AM IST Eric Biggers wrote:
> Hi Chandan,
>
> On Sun, Jun 16, 2019 at 09:38:07PM +0530, Chandan Rajendra wrote:
> > Read callbacks implements a state machine to be executed after a
> > buffered read I/O is completed. They help in further processing the file
> > data read from the backing store. Currently, decryption is the only post
> > processing step to be supported.
> >
> > The execution of the state machine is to be initiated by the endio
> > function associated with the read operation.
> >
> > Signed-off-by: Chandan Rajendra
> > ---
> > fs/Kconfig | 3 +
> > fs/Makefile| 2 +
> > fs/crypto/Kconfig | 1 +
> > fs/crypto/bio.c| 11 +++
> > fs/read_callbacks.c| 174 +
> > include/linux/fscrypt.h| 5 +
> > include/linux/read_callbacks.h | 38 +++
> > 7 files changed, 234 insertions(+)
> > create mode 100644 fs/read_callbacks.c
> > create mode 100644 include/linux/read_callbacks.h
> >
> > diff --git a/fs/Kconfig b/fs/Kconfig
> > index f1046cf6ad85..d869859c88da 100644
> > --- a/fs/Kconfig
> > +++ b/fs/Kconfig
> > @@ -21,6 +21,9 @@ if BLOCK
> > config FS_IOMAP
> > bool
> >
> > +config FS_READ_CALLBACKS
> > + bool
>
> This should be intended with a tab, not spaces.
>
> > +
> > source "fs/ext2/Kconfig"
> > source "fs/ext4/Kconfig"
> > source "fs/jbd2/Kconfig"
> > diff --git a/fs/Makefile b/fs/Makefile
> > index c9aea23aba56..a1a06f0db5c1 100644
> > --- a/fs/Makefile
> > +++ b/fs/Makefile
> > @@ -21,6 +21,8 @@ else
> > obj-y += no-block.o
> > endif
> >
> > +obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
> > +
> > obj-$(CONFIG_PROC_FS) += proc_namespace.o
>
> Nit: maybe move this to just below the line for iomap.o, to be consistent with
> where FS_READ_CALLBACKS is in the Kconfig file.
>
> >
> > obj-y += notify/
> > diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> > index 24ed99e2eca0..7752f9964280 100644
> > --- a/fs/crypto/Kconfig
> > +++ b/fs/crypto/Kconfig
> > @@ -9,6 +9,7 @@ config FS_ENCRYPTION
> > select CRYPTO_CTS
> > select CRYPTO_SHA256
> > select KEYS
> > + select FS_READ_CALLBACKS if BLOCK
> > help
> > Enable encryption of files and directories. This
> > feature is similar to ecryptfs, but it is more memory
> > diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> > index 82da2510721f..f677ff93d464 100644
> > --- a/fs/crypto/bio.c
> > +++ b/fs/crypto/bio.c
> > @@ -24,6 +24,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "fscrypt_private.h"
> >
> > static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
> > @@ -68,6 +69,16 @@ void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx
> > *ctx, struct bio *bio)
> > }
> > EXPORT_SYMBOL(fscrypt_enqueue_decrypt_bio);
> >
> > +void fscrypt_decrypt_work(struct work_struct *work)
> > +{
> > + struct read_callbacks_ctx *ctx =
> > + container_of(work, struct read_callbacks_ctx, work);
> > +
> > + fscrypt_decrypt_bio(ctx->bio);
> > +
> > + read_callbacks(ctx);
> > +}
> > +
>
> This seems like a layering violation, since it means that read_callbacks.c
> calls
> fs/crypto/ *and* fs/crypto/ calls read_callbacks.c. I don't think fs/crypto/
> should be aware of read_callbacks at all. Instead we should have a clear
> ordering between the components: the filesystem uses read_callbacks.c and
> fs/crypto/, and read_callbacks.c uses fs/crypto/. So how about:
>
> - Move fscrypt_decrypt_work(), fscrypt_decrypt_bh(), and fscrypt_decrypt_bio()
> into fs/read_callbacks.c and remove the "fscrypt_" prefix from them.
>
> - Instead of selecting FS_READ_CALLBACKS from FS_ENCRYPTION, select it from
> EXT4_FS and F2FS_FS (if FS_ENCRYPTION). I.e., it's really the *filesystems*
> themselves that use read_callbacks, not fs/crypto/.
>
> - Move the definition of read_callbacks_ctx into fs/read_callbacks.c, and make
> read_callbacks() static, so these are private to the read_callbacks
> component.
>
> > int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
> > sector_t pblk, unsigned int len)
> > {
> > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> > new file mode 100644
> > index ..a4196e3de05f
> > --- /dev/null
> > +++ b/fs/read_callbacks.c
> > @@ -0,0 +1,174 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * This file tracks the state machine that needs to be executed after
> > reading
> > + * data from files that are encrypted and/or have verity metadata
> > associated
> > + * with them.
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define NUM_PREALLOC_READ_CALLBACK_CTXS128
> > +
> > +static struct kmem_cache *read_callbacks_ctx_cache;
> > +static mempool_t *read_callbacks_ctx_pool;
> > +
> > +/* Read cal
Re: [f2fs-dev] [PATCH V3 1/7] FS: Introduce read callbacks
Hi Chandan,
On Sun, Jun 16, 2019 at 09:38:07PM +0530, Chandan Rajendra wrote:
> Read callbacks implements a state machine to be executed after a
> buffered read I/O is completed. They help in further processing the file
> data read from the backing store. Currently, decryption is the only post
> processing step to be supported.
>
> The execution of the state machine is to be initiated by the endio
> function associated with the read operation.
>
> Signed-off-by: Chandan Rajendra
> ---
> fs/Kconfig | 3 +
> fs/Makefile| 2 +
> fs/crypto/Kconfig | 1 +
> fs/crypto/bio.c| 11 +++
> fs/read_callbacks.c| 174 +
> include/linux/fscrypt.h| 5 +
> include/linux/read_callbacks.h | 38 +++
> 7 files changed, 234 insertions(+)
> create mode 100644 fs/read_callbacks.c
> create mode 100644 include/linux/read_callbacks.h
>
> diff --git a/fs/Kconfig b/fs/Kconfig
> index f1046cf6ad85..d869859c88da 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -21,6 +21,9 @@ if BLOCK
> config FS_IOMAP
> bool
>
> +config FS_READ_CALLBACKS
> + bool
This should be intended with a tab, not spaces.
> +
> source "fs/ext2/Kconfig"
> source "fs/ext4/Kconfig"
> source "fs/jbd2/Kconfig"
> diff --git a/fs/Makefile b/fs/Makefile
> index c9aea23aba56..a1a06f0db5c1 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -21,6 +21,8 @@ else
> obj-y += no-block.o
> endif
>
> +obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
> +
> obj-$(CONFIG_PROC_FS) += proc_namespace.o
Nit: maybe move this to just below the line for iomap.o, to be consistent with
where FS_READ_CALLBACKS is in the Kconfig file.
>
> obj-y+= notify/
> diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
> index 24ed99e2eca0..7752f9964280 100644
> --- a/fs/crypto/Kconfig
> +++ b/fs/crypto/Kconfig
> @@ -9,6 +9,7 @@ config FS_ENCRYPTION
> select CRYPTO_CTS
> select CRYPTO_SHA256
> select KEYS
> + select FS_READ_CALLBACKS if BLOCK
> help
> Enable encryption of files and directories. This
> feature is similar to ecryptfs, but it is more memory
> diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
> index 82da2510721f..f677ff93d464 100644
> --- a/fs/crypto/bio.c
> +++ b/fs/crypto/bio.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
> #include "fscrypt_private.h"
>
> static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
> @@ -68,6 +69,16 @@ void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx,
> struct bio *bio)
> }
> EXPORT_SYMBOL(fscrypt_enqueue_decrypt_bio);
>
> +void fscrypt_decrypt_work(struct work_struct *work)
> +{
> + struct read_callbacks_ctx *ctx =
> + container_of(work, struct read_callbacks_ctx, work);
> +
> + fscrypt_decrypt_bio(ctx->bio);
> +
> + read_callbacks(ctx);
> +}
> +
This seems like a layering violation, since it means that read_callbacks.c calls
fs/crypto/ *and* fs/crypto/ calls read_callbacks.c. I don't think fs/crypto/
should be aware of read_callbacks at all. Instead we should have a clear
ordering between the components: the filesystem uses read_callbacks.c and
fs/crypto/, and read_callbacks.c uses fs/crypto/. So how about:
- Move fscrypt_decrypt_work(), fscrypt_decrypt_bh(), and fscrypt_decrypt_bio()
into fs/read_callbacks.c and remove the "fscrypt_" prefix from them.
- Instead of selecting FS_READ_CALLBACKS from FS_ENCRYPTION, select it from
EXT4_FS and F2FS_FS (if FS_ENCRYPTION). I.e., it's really the *filesystems*
themselves that use read_callbacks, not fs/crypto/.
- Move the definition of read_callbacks_ctx into fs/read_callbacks.c, and make
read_callbacks() static, so these are private to the read_callbacks component.
> int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
> sector_t pblk, unsigned int len)
> {
> diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> new file mode 100644
> index ..a4196e3de05f
> --- /dev/null
> +++ b/fs/read_callbacks.c
> @@ -0,0 +1,174 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This file tracks the state machine that needs to be executed after reading
> + * data from files that are encrypted and/or have verity metadata associated
> + * with them.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define NUM_PREALLOC_READ_CALLBACK_CTXS 128
> +
> +static struct kmem_cache *read_callbacks_ctx_cache;
> +static mempool_t *read_callbacks_ctx_pool;
> +
> +/* Read callback state machine steps */
> +enum read_callbacks_step {
> + STEP_INITIAL = 0,
> + STEP_DECRYPT,
> +};
> +
> +static void put_read_callbacks_ctx(struct read_callbacks_ctx *ctx)
> +{
> + mempool_free(ctx, read_callbacks_ctx_pool);
> +}
Maybe call this free_read_callbacks_ctx(), so that it doesn't sound like it's
do
[f2fs-dev] [PATCH V3 1/7] FS: Introduce read callbacks
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
---
fs/Kconfig | 3 +
fs/Makefile| 2 +
fs/crypto/Kconfig | 1 +
fs/crypto/bio.c| 11 +++
fs/read_callbacks.c| 174 +
include/linux/fscrypt.h| 5 +
include/linux/read_callbacks.h | 38 +++
7 files changed, 234 insertions(+)
create mode 100644 fs/read_callbacks.c
create mode 100644 include/linux/read_callbacks.h
diff --git a/fs/Kconfig b/fs/Kconfig
index f1046cf6ad85..d869859c88da 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -21,6 +21,9 @@ if BLOCK
config FS_IOMAP
bool
+config FS_READ_CALLBACKS
+ bool
+
source "fs/ext2/Kconfig"
source "fs/ext4/Kconfig"
source "fs/jbd2/Kconfig"
diff --git a/fs/Makefile b/fs/Makefile
index c9aea23aba56..a1a06f0db5c1 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -21,6 +21,8 @@ else
obj-y += no-block.o
endif
+obj-$(CONFIG_FS_READ_CALLBACKS) += read_callbacks.o
+
obj-$(CONFIG_PROC_FS) += proc_namespace.o
obj-y += notify/
diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig
index 24ed99e2eca0..7752f9964280 100644
--- a/fs/crypto/Kconfig
+++ b/fs/crypto/Kconfig
@@ -9,6 +9,7 @@ config FS_ENCRYPTION
select CRYPTO_CTS
select CRYPTO_SHA256
select KEYS
+ select FS_READ_CALLBACKS if BLOCK
help
Enable encryption of files and directories. This
feature is similar to ecryptfs, but it is more memory
diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
index 82da2510721f..f677ff93d464 100644
--- a/fs/crypto/bio.c
+++ b/fs/crypto/bio.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include "fscrypt_private.h"
static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
@@ -68,6 +69,16 @@ void fscrypt_enqueue_decrypt_bio(struct fscrypt_ctx *ctx,
struct bio *bio)
}
EXPORT_SYMBOL(fscrypt_enqueue_decrypt_bio);
+void fscrypt_decrypt_work(struct work_struct *work)
+{
+ struct read_callbacks_ctx *ctx =
+ container_of(work, struct read_callbacks_ctx, work);
+
+ fscrypt_decrypt_bio(ctx->bio);
+
+ read_callbacks(ctx);
+}
+
int fscrypt_zeroout_range(const struct inode *inode, pgoff_t lblk,
sector_t pblk, unsigned int len)
{
diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
new file mode 100644
index ..a4196e3de05f
--- /dev/null
+++ b/fs/read_callbacks.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This file tracks the state machine that needs to be executed after reading
+ * data from files that are encrypted and/or have verity metadata associated
+ * with them.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define NUM_PREALLOC_READ_CALLBACK_CTXS128
+
+static struct kmem_cache *read_callbacks_ctx_cache;
+static mempool_t *read_callbacks_ctx_pool;
+
+/* Read callback state machine steps */
+enum read_callbacks_step {
+ STEP_INITIAL = 0,
+ STEP_DECRYPT,
+};
+
+static void put_read_callbacks_ctx(struct read_callbacks_ctx *ctx)
+{
+ mempool_free(ctx, read_callbacks_ctx_pool);
+}
+
+static void end_read_callbacks_bio(struct bio *bio)
+{
+ struct read_callbacks_ctx *ctx;
+ struct page *page;
+ struct bio_vec *bv;
+ struct bvec_iter_all iter_all;
+
+ ctx = bio->bi_private;
+
+ bio_for_each_segment_all(bv, bio, iter_all) {
+ page = bv->bv_page;
+
+ if (bio->bi_status || PageError(page)) {
+ ClearPageUptodate(page);
+ SetPageError(page);
+ } else {
+ SetPageUptodate(page);
+ }
+
+ if (ctx->page_op)
+ ctx->page_op(bio, page);
+
+ unlock_page(page);
+ }
+
+ put_read_callbacks_ctx(ctx);
+
+ bio_put(bio);
+}
+
+/**
+ * read_callbacks() - Execute the read callbacks state machine.
+ * @ctx: The context structure tracking the current state.
+ *
+ * For each state, this function enqueues a work into appropriate subsystem's
+ * work queue. After performing further processing of the data in the bio's
+ * pages, the subsystem should invoke read_calbacks() to continue with the next
+ * state in the state machine.
+ */
+void read_callbacks(struct read_callbacks_ctx *ctx)
+{
+ /*
+* We use different work queues for decryption and for verity because
+* verity may require reading metadata pages that need decryption, and
+* we shouldn't recurse to the same workqueue.
+
