Re: [PATCH 6/9] blk-crypto: optimize bio splitting in blk_crypto_fallback_encrypt_bio

Christoph Hellwig
2025-11-14T05:56:21Z

On Thu, Nov 13, 2025 at 04:22:10PM -0800, Eric Biggers wrote:
> > --- a/block/blk-crypto-fallback.c
> > +++ b/block/blk-crypto-fallback.c
> > @@ -152,35 +152,26 @@ static void blk_crypto_fallback_encrypt_endio(struct bio *enc_bio)
> >
> > src_bio->bi_status = enc_bio->bi_status;
>
> There can now be multiple enc_bios completing for the same src_bio, so
> this needs something like:
>
> if (enc_bio->bi_status)
> cmpxchg(&src_bio->bi_status, 0, enc_bio->bi_status);

Yes.

> > -static struct bio *blk_crypto_fallback_clone_bio(struct bio *bio_src)
> > +static struct bio *blk_crypto_alloc_enc_bio(struct bio *bio_src,
> > + unsigned int nr_segs)
> > {
> > - unsigned int nr_segs = bio_segments(bio_src);
> > - struct bvec_iter iter;
> > - struct bio_vec bv;
> > struct bio *bio;
> >
> > - bio = bio_kmalloc(nr_segs, GFP_NOIO);
> > - if (!bio)
> > - return NULL;
> > - bio_init_inline(bio, bio_src->bi_bdev, nr_segs, bio_src->bi_opf);
> > + bio = bio_alloc_bioset(bio_src->bi_bdev, nr_segs, bio_src->bi_opf,
> > + GFP_NOIO, &crypto_bio_split);
>
> Rename crypto_bio_split => enc_bio_set?

Sure.

> > static bool blk_crypto_fallback_encrypt_bio(struct bio **bio_ptr)
> > {
>
> I don't think this patch makes sense by itself, since it leaves the
> bio_ptr argument that is used to return a single enc_bio. It does get
> updated later in the series, but it seems that additional change to how
> this function is called should go earlier in the series.

I'll look into it.

>
> > + /* Encrypt each page in the origin bio */
>
> Maybe origin => source, so that consistent terminology is used.

Ok.

>
> > + if (++enc_idx == enc_bio->bi_max_vecs) {
> > + /*
> > + * Each encrypted bio will call bio_endio in the
> > + * completion handler, so ensure the remaining count
> > + * matches the number of submitted bios.
> > + */
> > + bio_inc_remaining(src_bio);
> > + submit_bio(enc_bio);
>
> The above comment is a bit confusing and could be made clearer. When we
> get here for the first time for example, we increment remaining from 1
> to 2. It doesn't match the number of bios submitted so far, but rather
> is one more than it. The extra one pairs with the submit_bio() outside
> the loop. Maybe consider the following:
>
> /*
> * For each additional encrypted bio submitted,
> * increment the source bio's remaining count. Each
> * encrypted bio's completion handler calls bio_endio on
> * the source bio, so this keeps the source bio from
> * completing until the last encrypted bio does.
> */

Yeah. The comment is a leftover from a previous version that worked a
little differently.

> > +out_ioerror:
> > + while (enc_idx > 0)
> > + mempool_free(enc_bio->bi_io_vec[enc_idx--].bv_page,
> > + blk_crypto_bounce_page_pool);
> > + bio_put(enc_bio);
> > + src_bio->bi_status = BLK_STS_IOERR;
>
> This error path doesn't seem correct at all. It would need to free the
> full set of pages in enc_bio, not just the ones initialized so far.

As of this patch the pages are allocated as we go, so I think this is
correct.

> It
> would also need to use cmpxchg() to correctly set an error on the
> src_bio considering that blk_crypto_fallback_encrypt_endio() be trying
> to do it concurrently too, and then call bio_endio() on it.

Yeah.

> (It's annoying that encryption errors need to be handled at all. When I
> eventually convert this to use lib/crypto/, the encryption functions are
> just going to return void. But for now this is using the traditional
> API, which can fail, so technically errors need to be handled...)

I can't wait for the library conversion to happen :)