Hi Eric, On 2018/4/18 1:42, Eric Biggers wrote: > 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 Yup, > require I/O to the file to read hashes, which may need STEP_DECRYPT. Hence, > decryption and verity will need separate workqueues. For decryption and verity, the needs separated data, I agree that we can not merge the work into one workqueue. As you mentioned in commit message, it can be used by compression later, so I just thought that for decryption and decompression, maybe we can do those work sequentially in one workqueue? > >>> @@ -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. As we will check bi_private in read_end_io anyway, if it is not NULL, we will parse it as an ctx, am I missing something? Thanks, > >>> + 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
![]() |