Re: [PATCH v4 06/14] block: introduce dma map backed bio type

[Christoph Hellwig <hch@lst.de>]

> -	bio->bi_io_vec = bio_src->bi_io_vec;
> +
> +	if (op_is_dmabuf(bio->bi_opf)) {
> +		bio->bi_dmabuf_map = bio_src->bi_dmabuf_map;
> +	} else {
> +		bio->bi_io_vec = bio_src->bi_io_vec;
> +	}

No need for the braces.  But given that these are the fields why
even bother doing both sides and not rely on the union?

> +void bio_dmabuf_map_set(struct bio *bio, struct iov_iter *iter)
> +{
> +	WARN_ON_ONCE(bio->bi_max_vecs);
> +
> +	bio->bi_dmabuf_map = iter->dmabuf_map;
> +	bio->bi_vcnt = 0;
> +	bio->bi_iter.bi_offset = iter->iov_offset;
> +	bio->bi_iter.bi_size = iov_iter_count(iter);
> +	bio->bi_opf |= REQ_NOMERGE | REQ_DMABUF;

This seems to be largely copied from bio_iov_bvec_set, but misses
the REQ_CLONE there that we should probably set as well to indicate
that the data descriptor is not owned (although not setting it is
not really a bug).

What about merging these two into a single and easier to use interface
like this:

diff --git a/block/bio.c b/block/bio.c
index cc2bb9183c1a..25393bdd119d 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -860,12 +860,9 @@ static int __bio_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp)
 	bio->bi_write_hint = bio_src->bi_write_hint;
 	bio->bi_write_stream = bio_src->bi_write_stream;
 	bio->bi_iter = bio_src->bi_iter;
-
-	if (op_is_dmabuf(bio->bi_opf)) {
-		bio->bi_dmabuf_map = bio_src->bi_dmabuf_map;
-	} else {
-		bio->bi_io_vec = bio_src->bi_io_vec;
-	}
+	static_assert(offsetof(struct bio, bi_io_vec) ==
+		      offsetof(struct bio, bi_dmabuf_map));
+	bio->bi_io_vec = bio_src->bi_io_vec;
 
 	if (bio->bi_bdev) {
 		if (bio->bi_bdev == bio_src->bi_bdev &&
@@ -1186,26 +1183,21 @@ void __bio_release_pages(struct bio *bio, bool mark_dirty)
 }
 EXPORT_SYMBOL_GPL(__bio_release_pages);
 
-void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter)
+bool bio_iov_iter_set(struct bio *bio, const struct iov_iter *iter)
 {
 	WARN_ON_ONCE(bio->bi_max_vecs);
 
+	if (!iov_iter_is_bvec(iter) && !iov_iter_is_dmabuf_map(iter))
+		return false;
+
 	bio->bi_io_vec = (struct bio_vec *)iter->bvec;
 	bio->bi_iter.bi_idx = 0;
 	bio->bi_iter.bi_offset = iter->iov_offset;
 	bio->bi_iter.bi_size = iov_iter_count(iter);
 	bio_set_flag(bio, BIO_CLONED);
-}
-
-void bio_dmabuf_map_set(struct bio *bio, struct iov_iter *iter)
-{
-	WARN_ON_ONCE(bio->bi_max_vecs);
-
-	bio->bi_dmabuf_map = iter->dmabuf_map;
-	bio->bi_vcnt = 0;
-	bio->bi_iter.bi_offset = iter->iov_offset;
-	bio->bi_iter.bi_size = iov_iter_count(iter);
-	bio->bi_opf |= REQ_NOMERGE | REQ_DMABUF;
+	if (iov_iter_is_dmabuf_map(iter))
+		bio->bi_opf |= REQ_NOMERGE | REQ_DMABUF;
+	return true;
 }
 
 /*
@@ -1265,13 +1257,7 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
 	if (WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)))
 		return -EIO;
 
-	if (iov_iter_is_bvec(iter)) {
-		bio_iov_bvec_set(bio, iter);
-		iov_iter_advance(iter, bio->bi_iter.bi_size);
-		return 0;
-	}
-	if (iov_iter_is_dmabuf_map(iter)) {
-		bio_dmabuf_map_set(bio, iter);
+	if (bio_iov_iter_set(bio, iter)) {
 		iov_iter_advance(iter, bio->bi_iter.bi_size);
 		return 0;
 	}
diff --git a/block/blk-map.c b/block/blk-map.c
index d1d6bbe0ecf1..34816e6e64de 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -473,7 +473,7 @@ static int blk_rq_map_user_bvec(struct request *rq, const struct iov_iter *iter)
 	bio = blk_rq_map_bio_alloc(rq, 0, GFP_KERNEL);
 	if (!bio)
 		return -ENOMEM;
-	bio_iov_bvec_set(bio, iter);
+	bio_iov_iter_set(bio, iter);
 
 	ret = blk_rq_append_bio(rq, bio);
 	if (ret)
diff --git a/block/fops.c b/block/fops.c
index d83cdbab65a6..97fb124a19b2 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -340,17 +340,12 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
 	bio->bi_end_io = blkdev_bio_end_io_async;
 	bio->bi_ioprio = iocb->ki_ioprio;
 
-	if (iov_iter_is_bvec(iter)) {
-		/*
-		 * Users don't rely on the iterator being in any particular
-		 * state for async I/O returning -EIOCBQUEUED, hence we can
-		 * avoid expensive iov_iter_advance(). Bypass
-		 * bio_iov_iter_get_pages() and set the bvec directly.
-		 */
-		bio_iov_bvec_set(bio, iter);
-	} else if (iov_iter_is_dmabuf_map(iter)) {
-		bio_dmabuf_map_set(bio, iter);
-	} else {
+	/*
+	 * Users don't rely on the iterator being in any particular state for
+	 * async I/O returning -EIOCBQUEUED, hence we can avoid the expensive
+	 * iov_iter_advance() if we could set the bvec/dmabuf directly.
+	 */
+	if (!bio_iov_iter_set(bio, iter)) {
 		ret = blkdev_iov_iter_get_pages(bio, iter, bdev);
 		if (unlikely(ret))
 			goto out_bio_put;
diff --git a/include/linux/bio.h b/include/linux/bio.h
index c053e9444514..1eff02a843ef 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -484,8 +484,7 @@ int bdev_rw_virt(struct block_device *bdev, sector_t sector, void *data,
 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter,
 		unsigned len_align_mask);
 
-void bio_iov_bvec_set(struct bio *bio, const struct iov_iter *iter);
-void bio_dmabuf_map_set(struct bio *bio, struct iov_iter *iter);
+bool bio_iov_iter_set(struct bio *bio, const struct iov_iter *iter);
 void __bio_release_pages(struct bio *bio, bool mark_dirty);
 extern void bio_set_pages_dirty(struct bio *bio);
 extern void bio_check_pages_dirty(struct bio *bio);