From: Christoph Hellwig <hch@infradead.org>
To: Satya Tangirala <satyat@google.com>
Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
Barani Muthukumaran <bmuthuku@qti.qualcomm.com>,
Kuohong Wang <kuohong.wang@mediatek.com>,
Kim Boojin <boojin.kim@samsung.com>
Subject: Re: [PATCH v5 2/9] block: Add encryption context to struct bio
Date: Thu, 31 Oct 2019 11:16:13 -0700 [thread overview]
Message-ID: <20191031181613.GC23601@infradead.org> (raw)
In-Reply-To: <20191028072032.6911-3-satyat@google.com>
> +static int num_prealloc_crypt_ctxs = 128;
Where does that magic number come from?
> +struct bio_crypt_ctx *bio_crypt_alloc_ctx(gfp_t gfp_mask)
> +{
> + return mempool_alloc(bio_crypt_ctx_pool, gfp_mask);
> +}
> +EXPORT_SYMBOL(bio_crypt_alloc_ctx);
This isn't used by an modular code.
> +void bio_crypt_free_ctx(struct bio *bio)
> +{
> + mempool_free(bio->bi_crypt_context, bio_crypt_ctx_pool);
> + bio->bi_crypt_context = NULL;
> +}
> +EXPORT_SYMBOL(bio_crypt_free_ctx);
This one is called from modular code, but I think the usage in DM
is bogus, as the caller of the function eventually does a bio_put,
which ends up in bio_free and takes care of the freeing as well.
> +bool bio_crypt_should_process(struct bio *bio, struct request_queue *q)
> +{
> + if (!bio_has_crypt_ctx(bio))
> + return false;
> +
> + WARN_ON(!bio_crypt_has_keyslot(bio));
> + return q->ksm == bio->bi_crypt_context->processing_ksm;
> +}
Passing a struct request here and also adding the ->bio != NULL check
here would simplify the only caller in ufs a bit.
> +/*
> + * Checks that two bio crypt contexts are compatible - i.e. that
> + * they are mergeable except for data_unit_num continuity.
> + */
> +bool bio_crypt_ctx_compatible(struct bio *b_1, struct bio *b_2)
> +{
> + struct bio_crypt_ctx *bc1 = b_1->bi_crypt_context;
> + struct bio_crypt_ctx *bc2 = b_2->bi_crypt_context;
> +
> + if (bio_has_crypt_ctx(b_1) != bio_has_crypt_ctx(b_2))
> + return false;
> +
> + if (!bio_has_crypt_ctx(b_1))
> + return true;
> +
> + return bc1->keyslot == bc2->keyslot &&
> + bc1->data_unit_size_bits == bc2->data_unit_size_bits;
> +}
I think we'd normally call this bio_crypt_ctx_mergeable.
> + if (bio_crypt_clone(b, bio, gfp_mask) < 0) {
> + bio_put(b);
> + return NULL;
> + }
>
> - if (ret < 0) {
> - bio_put(b);
> - return NULL;
> - }
> + if (bio_integrity(bio) &&
> + bio_integrity_clone(b, bio, gfp_mask) < 0) {
> + bio_put(b);
> + return NULL;
Pleae use a goto to merge the common error handling path
> + if (!bio_crypt_ctx_back_mergeable(req->bio,
> + blk_rq_sectors(req),
> + next->bio)) {
> + return ELEVATOR_NO_MERGE;
> + }
No neef for the braces. And pretty weird alignment, normal Linux style
would be:
if (!bio_crypt_ctx_back_mergeable(req->bio,
blk_rq_sectors(req), next->bio))
return ELEVATOR_NO_MERGE;
> + if (!bio_crypt_ctx_back_mergeable(rq->bio,
> + blk_rq_sectors(rq), bio)) {
> + return ELEVATOR_NO_MERGE;
> + }
> return ELEVATOR_BACK_MERGE;
> - else if (blk_rq_pos(rq) - bio_sectors(bio) == bio->bi_iter.bi_sector)
> + } else if (blk_rq_pos(rq) - bio_sectors(bio) ==
> + bio->bi_iter.bi_sector) {
> + if (!bio_crypt_ctx_back_mergeable(bio,
> + bio_sectors(bio), rq->bio)) {
> + return ELEVATOR_NO_MERGE;
> + }
Same for these two.
> +++ b/block/bounce.c
> @@ -267,14 +267,15 @@ static struct bio *bounce_clone_bio(struct bio *bio_src, gfp_t gfp_mask,
> break;
> }
>
> - if (bio_integrity(bio_src)) {
> - int ret;
> + if (bio_crypt_clone(bio, bio_src, gfp_mask) < 0) {
> + bio_put(bio);
> + return NULL;
> + }
>
> - ret = bio_integrity_clone(bio, bio_src, gfp_mask);
> - if (ret < 0) {
> - bio_put(bio);
> - return NULL;
> - }
> + if (bio_integrity(bio_src) &&
> + bio_integrity_clone(bio, bio_src, gfp_mask) < 0) {
> + bio_put(bio);
> + return NULL;
Use a common error path with a goto, please.
> +static inline int bio_crypt_set_ctx(struct bio *bio,
> + const u8 *raw_key,
> + enum blk_crypto_mode_num crypto_mode,
> + u64 dun,
> + unsigned int dun_bits,
> + gfp_t gfp_mask)
Pleae just open code this in the only caller.
> +{
> + struct bio_crypt_ctx *crypt_ctx;
> +
> + crypt_ctx = bio_crypt_alloc_ctx(gfp_mask);
> + if (!crypt_ctx)
> + return -ENOMEM;
Also bio_crypt_alloc_ctx with a waiting mask will never return an
error. Changing this function and its call chain to void returns will
clean up a lot of code in this series.
> +static inline void bio_set_data_unit_num(struct bio *bio, u64 dun)
> +{
> + bio->bi_crypt_context->data_unit_num = dun;
> +}
This function is never used and can be removed.
> +static inline void bio_crypt_set_keyslot(struct bio *bio,
> + unsigned int keyslot,
> + struct keyslot_manager *ksm)
> +{
> + bio->bi_crypt_context->keyslot = keyslot;
> + bio->bi_crypt_context->processing_ksm = ksm;
> +}
Just adding these two lines to the only caller will be a lot cleaner.
> +static inline const u8 *bio_crypt_raw_key(struct bio *bio)
> +{
> + return bio->bi_crypt_context->raw_key;
> +}
Can be inlined into the only caller.
> +
> +static inline enum blk_crypto_mode_num bio_crypto_mode(struct bio *bio)
> +{
> + return bio->bi_crypt_context->crypto_mode;
> +}
Same here.
> +static inline u64 bio_crypt_sw_data_unit_num(struct bio *bio)
> +{
> + return bio->bi_crypt_context->sw_data_unit_num;
> +}
Same here.
next prev parent reply other threads:[~2019-10-31 18:16 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-28 7:20 [PATCH v5 0/9] Inline Encryption Support Satya Tangirala
2019-10-28 7:20 ` [PATCH v5 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala
2019-10-31 18:04 ` Christoph Hellwig
2019-10-28 7:20 ` [PATCH v5 2/9] block: Add encryption context to struct bio Satya Tangirala
2019-10-31 18:16 ` Christoph Hellwig [this message]
2019-10-28 7:20 ` [PATCH v5 3/9] block: blk-crypto for Inline Encryption Satya Tangirala
2019-10-31 17:57 ` Christoph Hellwig
2019-10-31 20:50 ` Theodore Y. Ts'o
2019-10-31 21:22 ` Christoph Hellwig
2019-11-05 2:01 ` Eric Biggers
2019-11-05 15:39 ` Christoph Hellwig
2019-10-28 7:20 ` [PATCH v5 4/9] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala
2019-10-28 7:20 ` [PATCH v5 5/9] scsi: ufs: UFS crypto API Satya Tangirala
2019-10-31 18:23 ` Christoph Hellwig
2019-10-28 7:20 ` [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala
2019-10-31 18:26 ` Christoph Hellwig
2019-10-28 7:20 ` [PATCH v5 7/9] fscrypt: add inline encryption support Satya Tangirala
2019-10-31 18:32 ` Christoph Hellwig
2019-10-31 20:21 ` Eric Biggers
2019-10-31 21:21 ` Christoph Hellwig
2019-10-31 22:25 ` Eric Biggers
2019-11-05 0:15 ` Christoph Hellwig
2019-11-05 1:03 ` Eric Biggers
2019-11-05 3:12 ` Eric Biggers
2019-10-28 7:20 ` [PATCH v5 8/9] f2fs: " Satya Tangirala
2019-10-31 17:14 ` Jaegeuk Kim
2019-10-28 7:20 ` [PATCH v5 9/9] ext4: " Satya Tangirala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191031181613.GC23601@infradead.org \
--to=hch@infradead.org \
--cc=bmuthuku@qti.qualcomm.com \
--cc=boojin.kim@samsung.com \
--cc=kuohong.wang@mediatek.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=satyat@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).