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.
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@infradead.org> To: Satya Tangirala <satyat@google.com> Cc: linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>, Kuohong Wang <kuohong.wang@mediatek.com>, Barani Muthukumaran <bmuthuku@qti.qualcomm.com>, linux-f2fs-devel@lists.sourceforge.net, linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [f2fs-dev] [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. _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2019-10-31 18:16 UTC|newest] Thread overview: 54+ 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 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-28 7:20 ` [PATCH v5 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:04 ` Christoph Hellwig 2019-10-31 18:04 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 2/9] block: Add encryption context to struct bio Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:16 ` Christoph Hellwig [this message] 2019-10-31 18:16 ` Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 3/9] block: blk-crypto for Inline Encryption Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 17:57 ` Christoph Hellwig 2019-10-31 17:57 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 20:50 ` Theodore Y. Ts'o 2019-10-31 20:50 ` [f2fs-dev] " Theodore Y. Ts'o 2019-10-31 21:22 ` Christoph Hellwig 2019-10-31 21:22 ` [f2fs-dev] " Christoph Hellwig 2019-11-05 2:01 ` Eric Biggers 2019-11-05 2:01 ` [f2fs-dev] " Eric Biggers 2019-11-05 15:39 ` Christoph Hellwig 2019-11-05 15:39 ` [f2fs-dev] " 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 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-28 7:20 ` [PATCH v5 5/9] scsi: ufs: UFS crypto API Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:23 ` Christoph Hellwig 2019-10-31 18:23 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:26 ` Christoph Hellwig 2019-10-31 18:26 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 7/9] fscrypt: add inline encryption support Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:32 ` Christoph Hellwig 2019-10-31 18:32 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 20:21 ` Eric Biggers 2019-10-31 20:21 ` [f2fs-dev] " Eric Biggers 2019-10-31 21:21 ` Christoph Hellwig 2019-10-31 21:21 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 22:25 ` Eric Biggers 2019-10-31 22:25 ` [f2fs-dev] " Eric Biggers 2019-11-05 0:15 ` Christoph Hellwig 2019-11-05 0:15 ` [f2fs-dev] " Christoph Hellwig 2019-11-05 1:03 ` Eric Biggers 2019-11-05 1:03 ` [f2fs-dev] " Eric Biggers 2019-11-05 3:12 ` Eric Biggers 2019-11-05 3:12 ` [f2fs-dev] " Eric Biggers 2019-10-28 7:20 ` [PATCH v5 8/9] f2fs: " Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 17:14 ` Jaegeuk Kim 2019-10-31 17:14 ` [f2fs-dev] " Jaegeuk Kim 2019-10-28 7:20 ` [PATCH v5 9/9] ext4: " Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.