Re: [f2fs-dev] [PATCH V3 2/7] Integrate read callbacks into Ext4 and F2FS
On Saturday, June 22, 2019 2:38:01 AM IST Eric Biggers wrote:
> Hi Chandan,
>
> On Sun, Jun 16, 2019 at 09:38:08PM +0530, Chandan Rajendra wrote:
> > This commit gets Ext4 and F2FS to make use of read callbacks API to
> > perform decryption of file data read from the disk.
> > ---
> > fs/crypto/bio.c | 30 +
> > fs/crypto/crypto.c | 1 +
> > fs/crypto/fscrypt_private.h | 3 +
> > fs/ext4/readpage.c | 29 +++--
> > fs/f2fs/data.c | 124 +++-
> > fs/f2fs/super.c | 9 +--
> > fs/read_callbacks.c | 1 -
> > include/linux/fscrypt.h | 18 --
> > 8 files changed, 40 insertions(+), 175 deletions(-)
> >
>
> This patch changes many different components. It would be much easier to
> review, and might get more attention from the other ext4 and f2fs developers,
> if
> it were split into 3 patches:
>
> a. Convert ext4 to use read_callbacks.
> b. Convert f2fs to use read_callbacks.
> c. Remove the functions from fs/crypto/ that became unused as a result of
>patches (a) and (b). (Actually, this part probably should be merged with
> the
>patch that removes the fscrypt_ctx, and the patch renamed to something like
>"fscrypt: remove decryption I/O path helpers")
>
> Any reason why this wouldn't work? AFAICS, you couldn't do it only because
> you
> made this patch change fscrypt_enqueue_decrypt_work() to be responsible for
> initializing the work function. But as per my comments on patch 1, I don't
> think we should do that, since it would make much more sense to put the work
> function in read_callbacks.c.
Yes, you are right about that. I will make the changes suggested by you.
>
> However, since you're converting ext4 to use mpage_readpages() anyway, I don't
> think we should bother with the intermediate change to ext4_mpage_readpages().
> It's useless, and that intermediate state of the ext4 code inevitably won't
> get
> tested very well. So perhaps order the whole series as:
>
> - fs: introduce read_callbacks
> - fs/mpage.c: add decryption support via read_callbacks
> - fs/buffer.c: add decryption support via read_callbacks
> - f2fs: convert to use read_callbacks
> - ext4: convert to use mpage_readpages[s]
> - ext4: support encryption with subpage-sized blocks
> - fscrypt: remove decryption I/O path helpers
>
> That order would also give the flexibility to possibly apply the fs/ changes
> first, without having to update both ext4 and f2fs simultaneously with them.
>
> > @@ -557,8 +511,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 ret;
>
> Nit: 'err' rather than 'ret', since this is 0 or a -errno value.
>
> > -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);
> > -}
>
> Need to remove the declarations of these functions from fs/f2fs/f2fs.h to.
>
> > diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> > index a4196e3de05f..4b7fc2a349cd 100644
> > --- a/fs/read_callbacks.c
> > +++ b/fs/read_callbacks.c
> > @@ -76,7 +76,6 @@ void read_callbacks(struct read_callbacks_ctx *ctx)
> > switch (++ctx->cur_step) {
> > case STEP_DECRYPT:
> > if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
> > - INIT_WORK(&ctx->work, fscrypt_decrypt_work);
> > fscrypt_enqueue_decrypt_work(&ctx->work);
> > return;
> > }
>
> Again, I think the work initialization should remain here as:
>
> INIT_WORK(&ctx->work, decrypt_work);
>
> rather than moving it to fs/crypto/.
>
> Thanks!
>
> - Eric
>
--
chandan
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH V3 2/7] Integrate read callbacks into Ext4 and F2FS
Hi Chandan,
On Sun, Jun 16, 2019 at 09:38:08PM +0530, Chandan Rajendra wrote:
> This commit gets Ext4 and F2FS to make use of read callbacks API to
> perform decryption of file data read from the disk.
> ---
> fs/crypto/bio.c | 30 +
> fs/crypto/crypto.c | 1 +
> fs/crypto/fscrypt_private.h | 3 +
> fs/ext4/readpage.c | 29 +++--
> fs/f2fs/data.c | 124 +++-
> fs/f2fs/super.c | 9 +--
> fs/read_callbacks.c | 1 -
> include/linux/fscrypt.h | 18 --
> 8 files changed, 40 insertions(+), 175 deletions(-)
>
This patch changes many different components. It would be much easier to
review, and might get more attention from the other ext4 and f2fs developers, if
it were split into 3 patches:
a. Convert ext4 to use read_callbacks.
b. Convert f2fs to use read_callbacks.
c. Remove the functions from fs/crypto/ that became unused as a result of
patches (a) and (b). (Actually, this part probably should be merged with the
patch that removes the fscrypt_ctx, and the patch renamed to something like
"fscrypt: remove decryption I/O path helpers")
Any reason why this wouldn't work? AFAICS, you couldn't do it only because you
made this patch change fscrypt_enqueue_decrypt_work() to be responsible for
initializing the work function. But as per my comments on patch 1, I don't
think we should do that, since it would make much more sense to put the work
function in read_callbacks.c.
However, since you're converting ext4 to use mpage_readpages() anyway, I don't
think we should bother with the intermediate change to ext4_mpage_readpages().
It's useless, and that intermediate state of the ext4 code inevitably won't get
tested very well. So perhaps order the whole series as:
- fs: introduce read_callbacks
- fs/mpage.c: add decryption support via read_callbacks
- fs/buffer.c: add decryption support via read_callbacks
- f2fs: convert to use read_callbacks
- ext4: convert to use mpage_readpages[s]
- ext4: support encryption with subpage-sized blocks
- fscrypt: remove decryption I/O path helpers
That order would also give the flexibility to possibly apply the fs/ changes
first, without having to update both ext4 and f2fs simultaneously with them.
> @@ -557,8 +511,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 ret;
Nit: 'err' rather than 'ret', since this is 0 or a -errno value.
> -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);
> -}
Need to remove the declarations of these functions from fs/f2fs/f2fs.h to.
> diff --git a/fs/read_callbacks.c b/fs/read_callbacks.c
> index a4196e3de05f..4b7fc2a349cd 100644
> --- a/fs/read_callbacks.c
> +++ b/fs/read_callbacks.c
> @@ -76,7 +76,6 @@ void read_callbacks(struct read_callbacks_ctx *ctx)
> switch (++ctx->cur_step) {
> case STEP_DECRYPT:
> if (ctx->enabled_steps & (1 << STEP_DECRYPT)) {
> - INIT_WORK(&ctx->work, fscrypt_decrypt_work);
> fscrypt_enqueue_decrypt_work(&ctx->work);
> return;
> }
Again, I think the work initialization should remain here as:
INIT_WORK(&ctx->work, decrypt_work);
rather than moving it to fs/crypto/.
Thanks!
- Eric
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH V3 2/7] Integrate read callbacks into Ext4 and F2FS
This commit gets Ext4 and F2FS to make use of read callbacks API to
perform decryption of file data read from the disk.
---
fs/crypto/bio.c | 30 +
fs/crypto/crypto.c | 1 +
fs/crypto/fscrypt_private.h | 3 +
fs/ext4/readpage.c | 29 +++--
fs/f2fs/data.c | 124 +++-
fs/f2fs/super.c | 9 +--
fs/read_callbacks.c | 1 -
include/linux/fscrypt.h | 18 --
8 files changed, 40 insertions(+), 175 deletions(-)
diff --git a/fs/crypto/bio.c b/fs/crypto/bio.c
index f677ff93d464..4076d704e2e4 100644
--- a/fs/crypto/bio.c
+++ b/fs/crypto/bio.c
@@ -27,7 +27,7 @@
#include
#include "fscrypt_private.h"
-static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
+static void fscrypt_decrypt_bio(struct bio *bio)
{
struct bio_vec *bv;
struct bvec_iter_all iter_all;
@@ -38,37 +38,9 @@ static void __fscrypt_decrypt_bio(struct bio *bio, bool done)
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);
-
void fscrypt_decrypt_work(struct work_struct *work)
{
struct read_callbacks_ctx *ctx =
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 45c3d0427fb2..e0aa20cfcaa7 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -54,6 +54,7 @@ struct kmem_cache *fscrypt_info_cachep;
void fscrypt_enqueue_decrypt_work(struct work_struct *work)
{
+ INIT_WORK(work, fscrypt_decrypt_work);
queue_work(fscrypt_read_workqueue, work);
}
EXPORT_SYMBOL(fscrypt_enqueue_decrypt_work);
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 8978eec9d766..0bd65921efbf 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -113,6 +113,9 @@ static inline bool fscrypt_valid_enc_modes(u32
contents_mode,
return false;
}
+/* bio.c */
+void fscrypt_decrypt_work(struct work_struct *work);
+
/* crypto.c */
extern struct kmem_cache *fscrypt_info_cachep;
extern int fscrypt_initialize(unsigned int cop_flags);
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index c916017db334..652796b95545 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -44,6 +44,7 @@
#include
#include
#include
+#include
#include "ext4.h"
@@ -73,14 +74,9 @@ static void mpage_end_io(struct bio *bio)
struct bio_vec *bv;
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;
- }
- }
+ if (read_callbacks_end_bio(bio))
+ return;
+
bio_for_each_segment_all(bv, bio, iter_all) {
struct page *page = bv->bv_page;
@@ -241,24 +237,19 @@ int ext4_mpage_readpages(struct address_space *mapping,
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);
+ if (!bio)
+ goto set_error_page;
+
+ if (read_callbacks_setup(inode, bio, NULL)) {
+ bio_put(bio);
goto set_error_page;
}
+
bio_set_dev(bio, bdev);
bio->bi_iter.bi_sector = blocks[0] << (blkbits - 9);
bio->bi_end_io = mpage
