Hi Chao,
On Tue, Apr 17, 2018 at 05:13:12PM +0800, Chao Yu wrote:
> > +
> > +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);
> > + }
>
> How about introducing __bio_post_read_processing()
>
> switch (step) {
> case STEP_DECRYPT:
> ...
> break;
> case STEP_COMPRESS:
> ...
> break;
> case STEP_GENERIC:
> __read_end_io;
> break;
> ...
> }
>
> Then we can customize flexible read processes like:
>
> bio_post_read_processing()
> {
> if (encrypt_enabled)
> __bio_post_read_processing(, STEP_DECRYPT);
> if (compress_enabled)
> __bio_post_read_processing(, STEP_COMPRESS);
> __bio_post_read_processing(, STEP_GENERIC);
> }
>
> Or other flow.
If I understand correctly, you're suggesting that all the steps be done in a
single workqueue item? The problem with that is that the verity work will
require I/O to the file to read hashes, which may need STEP_DECRYPT. Hence,
decryption and verity will need separate workqueues.
> > @@ -481,29 +537,33 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr,
> > unsigned nr_pages)
> > {
> > struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > - struct fscrypt_ctx *ctx = NULL;
> > struct bio *bio;
> > -
> > - if (f2fs_encrypted_file(inode)) {
> > - ctx = fscrypt_get_ctx(inode, GFP_NOFS);
> > - if (IS_ERR(ctx))
> > - return ERR_CAST(ctx);
> > -
> > - /* wait the page to be moved by cleaning */
> > - f2fs_wait_on_block_writeback(sbi, blkaddr);
> > - }
> > + struct bio_post_read_ctx *ctx;
> > + unsigned int post_read_steps = 0;
> >
> > bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false);
> > - if (!bio) {
> > - if (ctx)
> > - fscrypt_release_ctx(ctx);
> > + if (!bio)
> > return ERR_PTR(-ENOMEM);
> > - }
> > f2fs_target_device(sbi, blkaddr, bio);
> > bio->bi_end_io = f2fs_read_end_io;
> > - bio->bi_private = ctx;
>
> bio->bi_private = NULL;
>
I don't see why. ->bi_private is NULL by default.
> > + bio_post_read_ctx_pool =
> > + mempool_create_slab_pool(128, bio_post_read_ctx_cache);
>
> #define MAX_POST_READ_CACHE_SIZE 128
>
Yes, that makes sense.
- Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|