Re: [PATCH v20 02/12] Add infrastructure for copy offload in block and request layer.

[Nitesh Shetty]

On 20/05/24 04:00PM, Bart Van Assche wrote:
> On 5/20/24 03:20, Nitesh Shetty wrote:
> > Upon arrival of source bio we merge these two bio's and send
> > corresponding request down to device driver.
>
> bios with different operation types must not be merged.
Copy is a composite operation which has two operation read and
write combined, so we chose two operation types which reflects that.

> > +static enum bio_merge_status bio_attempt_copy_offload_merge(struct request *req,
> > +							    struct bio *bio)
> > +{
> > +	if (req->__data_len != bio->bi_iter.bi_size)
> > +		return BIO_MERGE_FAILED;
> > +
> > +	req->biotail->bi_next = bio;
> > +	req->biotail = bio;
> > +	req->nr_phys_segments++;
> > +	req->__data_len += bio->bi_iter.bi_size;
> > +
> > +	return BIO_MERGE_OK;
> > +}
>
> This function appends a bio to a request. Hence, the name of this function is
> wrong.
We followed the naming convention from discard(bio_attempt_discard_merge)
which does similar thing.
But we are open to renaming, if overall community also feels the same.

> > @@ -1085,6 +1124,8 @@ static enum bio_merge_status blk_attempt_bio_merge(struct request_queue *q,
> >  		break;
> >  	case ELEVATOR_DISCARD_MERGE:
> >  		return bio_attempt_discard_merge(q, rq, bio);
> > +	case ELEVATOR_COPY_OFFLOAD_MERGE:
> > +		return bio_attempt_copy_offload_merge(rq, bio);
> >  	default:
> >  		return BIO_MERGE_NONE;
> >  	}
>
> Is any code added in this patch series that causes an I/O scheduler to return
> ELEVATOR_COPY_OFFLOAD_MERGE?
yes, blk_try_merge returns ELEVATOR_COPY_OFFLOAD_MERGE.

> > +static inline bool blk_copy_offload_mergable(struct request *req,
> > +					     struct bio *bio)
> > +{
> > +	return (req_op(req) == REQ_OP_COPY_DST &&
> > +		bio_op(bio) == REQ_OP_COPY_SRC);
> > +}
>
> bios with different operation types must not be merged. Please rename this function.
As described above we need two different opcodes.
As far as function renaming, we followed discard's naming. But open to
any suggestion.

> > +static inline bool op_is_copy(blk_opf_t op)
> > +{
> > +	return ((op & REQ_OP_MASK) == REQ_OP_COPY_SRC ||
> > +		(op & REQ_OP_MASK) == REQ_OP_COPY_DST);
> > +}
>
> The above function is not used in this patch. Please introduce new functions in the
> patch in which these are used for the first time.
Acked

Thank You
Nitesh Shetty