On 2020/06/24 14:27, Eric Biggers wrote: > On Wed, Jun 24, 2020 at 05:21:24AM +0000, Damien Le Moal wrote: >>>> @@ -1458,13 +1459,18 @@ static void crypt_alloc_req_skcipher(struct >>>> crypt_config *cc, >>>> >>>> skcipher_request_set_tfm(ctx->r.req, cc->cipher_tfm.tfms[key_index]); >>>> >>>> - /* >>>> - * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs >>>> - * requests if driver request queue is full. >>>> - */ >>>> - skcipher_request_set_callback(ctx->r.req, >>>> - CRYPTO_TFM_REQ_MAY_BACKLOG, >>>> - kcryptd_async_done, dmreq_of_req(cc, ctx->r.req)); >>>> + if (test_bit(DM_CRYPT_FORCE_INLINE, &cc->flags)) >>>> + /* make sure we zero important fields of the request */ >>>> + skcipher_request_set_callback(ctx->r.req, >>>> + 0, NULL, NULL); >>>> + else >>>> + /* >>>> + * Use REQ_MAY_BACKLOG so a cipher driver internally backlogs >>>> + * requests if driver request queue is full. >>>> + */ >>>> + skcipher_request_set_callback(ctx->r.req, >>>> + CRYPTO_TFM_REQ_MAY_BACKLOG, >>>> + kcryptd_async_done, dmreq_of_req(cc, ctx->r.req)); >>>> } >>> >>> This looks wrong. Unless type=0 and mask=CRYPTO_ALG_ASYNC are passed to >>> crypto_alloc_skcipher(), the skcipher implementation can still be >>> asynchronous, >>> in which case providing a callback is required. >>> >>> Do you intend that the "force_inline" option forces the use of a synchronous >>> skcipher (alongside the other things it does)? Or should it still allow >>> asynchronous ones? >>> >>> We may not actually have a choice in that matter, since xts-aes-aesni has >>> the >>> CRYPTO_ALG_ASYNC bit set (as I mentioned) despite being synchronous in most >>> cases; thus, the crypto API won't give you it if you ask for a synchronous >>> cipher. So I think you still need to allow async skciphers? That means a >>> callback is still always required. >> >> Arg... So it means that some skciphers will not be OK at all for SMR writes. >> I >> was not aware of these differences (tested with aes-xts-plain64 only). The >> ugly >> way to support async ciphers would be to just wait inline for the crypto API >> to >> complete using a completion for instance. But that is very ugly. Back to >> brainstorming, and need to learn more about the crypto API... >> > > It's easy to wait for crypto API requests to complete if you need to -- > just use crypto_wait_req().
OK. Thanks for the information. I will look into this and the performance implications. A quick grep shows that a lot of different accelerators for different architectures have CRYPTO_ALG_ASYNC set. So definitely something that needs to be checked for SMR, and for Ignat inline patch. > We do this in fs/crypto/, for example. (Not many people are using fscrypt > with > crypto API based accelerators, so there hasn't yet been much need to support > the > complexity of issuing multiple async crypto requests like dm-crypt supports.) Zonefs fscrypt support is on my to do list too :) Thanks ! > > - Eric > -- Damien Le Moal Western Digital Research
