On 5/13/26 09:19, Christoph Hellwig wrote:
>> + 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:
I can flip it, but compilers tend to prefer the true branch. E.g. this
if (cond) A; else B;
C;
can get compiled into:
jmpcc cond B
A: ...
C:
return;
B: ...
jmp C;
>
> 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.
Ok
>> +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
I'll take a look
> - figure out what to do about dm_bio_rewind_iter, which pokes into these
> things that really should be block layer internal
Need to check what that is, but doesn't implement the interface and
is not supposed to ever see the dmabuf iterator.
>> }
>> @@ -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?
Do you mean adding both and propagating it from bio to req? submit_bio()
takes a bio, so we still need to set it there before it reaches blk-mq.
And there might be bio-based drivers using it in the future.
--
Pavel Begunkov