Re: [PATCH v3 04/10] block: introduce dma map backed bio type

[Christoph Hellwig <hch@lst.de>]

> +	if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
> +		bio->bi_io_vec = bio_src->bi_io_vec;
> +	} else {
> +		bio->dmabuf_map = bio_src->dmabuf_map;
> +		bio_set_flag(bio, BIO_DMABUF_MAP);
> +	}

This is backwards, please avoid pointless negations:

	if (bio_flagged(bio_src, BIO_DMABUF_MAP)) {
		bio->dmabuf_map = bio_src->dmabuf_map;
		bio_set_flag(bio, BIO_DMABUF_MAP);
	} else {
		bio->bi_io_vec = bio_src->bi_io_vec;
	}

> +	if (bio_flagged(bio, BIO_DMABUF_MAP)) {
> +		nsegs = 1;
> +
> +		if ((bio->bi_iter.bi_bvec_done & lim->dma_alignment) ||
> +		    (bio->bi_iter.bi_size & len_align_mask))
> +			return -EINVAL;
> +		if (bio->bi_iter.bi_size > max_bytes) {
> +			bytes = max_bytes;
> +			goto split;
> +		}

Please add a comment explaining why nsegs is always 1 here.

> @@ -424,7 +424,8 @@ static inline struct bio *__bio_split_to_limits(struct bio *bio,
>  	switch (bio_op(bio)) {
>  	case REQ_OP_READ:
>  	case REQ_OP_WRITE:
> -		if (bio_may_need_split(bio, lim))
> +		if (bio_may_need_split(bio, lim) ||
> +		    bio_flagged(bio, BIO_DMABUF_MAP))
>  			return bio_split_rw(bio, lim, nr_segs);

The BIO_DMABUF_MAP check should go into bio_may_need_split.

> +static inline void bio_advance_iter_dmabuf_map(struct bvec_iter *iter,
> +					       unsigned int bytes)
> +{
> +	iter->bi_bvec_done += bytes;
> +	iter->bi_size -= bytes;
> +}
> +
>  static inline void bio_advance_iter(const struct bio *bio,
>  				    struct bvec_iter *iter, unsigned int bytes)
>  {
>  	iter->bi_sector += bytes >> 9;
>  
> -	if (bio_no_advance_iter(bio))
> +	if (bio_no_advance_iter(bio)) {
>  		iter->bi_size -= bytes;
> -	else
> +	} else if (bio_flagged(bio, BIO_DMABUF_MAP)) {
> +		bio_advance_iter_dmabuf_map(iter, bytes);

This is a bit of a mess.  You're using bi_bvec_done for something that
is not bvec_done, which makes the naming very confusing.  That is even
more confusing than the existing usage, which isn't great.  Also we
add yet another conditional to heavily inlined code.  I'd suggest
the following:

 - add a prep patch to rename bi_bvec_done to bi_offset, as even for
   the existing usage it is the offset into the current bio_vec as
   much as it is the count of byes done, as those must be the same
   and it is used both ways
 - add a prep patch to also increase bi_offset for bio_no_advance_iter.
   It is not actually use there, but incrementing it is harmless and
   this will avoid a new special case
 - please also documet this new usage in the commet in struct bvec_iter.
 - then just add the dma buf mapping to the bio_no_advance_iter condition
 - figure out what to do about dm_bio_rewind_iter, which pokes into these
   things that really should be block layer internal

>  }
> @@ -391,7 +403,7 @@ static inline void bio_wouldblock_error(struct bio *bio)
>   */
>  static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
>  {
> -	if (iov_iter_is_bvec(iter))
> +	if (iov_iter_is_bvec(iter) || iov_iter_is_dmabuf_map(iter))
>  		return 0;
>  	return iov_iter_npages(iter, max_segs);
>  }

Please update the comment for this helper.

> @@ -322,6 +327,7 @@ enum {
>  	BIO_REMAPPED,
>  	BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */
>  	BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */
> +	BIO_DMABUF_MAP, /* Using premmaped dma buffers */

Shouldn't this be a REQ_ flag as we should never mix and match bios with
and without this flag in a single request?