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

[Christoph Hellwig <hch@lst.de>]

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 :)