Re: [PATCH v13 2/9] block: Add copy offload support infrastructure

[Nitesh Shetty]

On 23/06/28 03:45PM, Damien Le Moal wrote:
> On 6/28/23 03:36, Nitesh Shetty wrote:
> > Introduce blkdev_copy_offload which takes similar arguments as
> > copy_file_range and performs copy offload between two bdevs.
>
> I am confused... I thought it was discussed to only allow copy offload only
> within a single bdev for now... Did I missi something ?

Yes, you are right. copy is supported within single bdev only.
We will update this.

> > Introduce REQ_OP_COPY_DST, REQ_OP_COPY_SRC operation.
> > Issue REQ_OP_COPY_DST with destination info along with taking a plug.
> > This flows till request layer and waits for src bio to get merged.
> > Issue REQ_OP_COPY_SRC with source info and this bio reaches request
> > layer and merges with dst request.
> > For any reason, if request comes to driver with either only one of src/dst
> > info we fail the copy offload.
> >
> > Larger copy will be divided, based on max_copy_sectors limit.
> >
> > Suggested-by: Christoph Hellwig <hch@xxxxxx>
> > Signed-off-by: Anuj Gupta <anuj20.g@xxxxxxxxxxx>
> > Signed-off-by: Nitesh Shetty <nj.shetty@xxxxxxxxxxx>
> > ---
> >  block/blk-core.c          |   5 ++
> >  block/blk-lib.c           | 177 ++++++++++++++++++++++++++++++++++++++
> >  block/blk-merge.c         |  21 +++++
> >  block/blk.h               |   9 ++
> >  block/elevator.h          |   1 +
> >  include/linux/bio.h       |   4 +-
> >  include/linux/blk_types.h |  21 +++++
> >  include/linux/blkdev.h    |   4 +
> >  8 files changed, 241 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 99d8b9812b18..e6714391c93f 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -796,6 +796,11 @@ void submit_bio_noacct(struct bio *bio)
> >  		if (!q->limits.max_write_zeroes_sectors)
> >  			goto not_supported;
> >  		break;
> > +	case REQ_OP_COPY_SRC:
> > +	case REQ_OP_COPY_DST:
> > +		if (!blk_queue_copy(q))
> > +			goto not_supported;
> > +		break;
> >  	default:
> >  		break;
> >  	}
> > diff --git a/block/blk-lib.c b/block/blk-lib.c
> > index e59c3069e835..10c3eadd5bf6 100644
> > --- a/block/blk-lib.c
> > +++ b/block/blk-lib.c
> > @@ -115,6 +115,183 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
> >  }
> >  EXPORT_SYMBOL(blkdev_issue_discard);
> >
> > +/*
> > + * For synchronous copy offload/emulation, wait and process all in-flight BIOs.
> > + * This must only be called once all bios have been issued so that the refcount
> > + * can only decrease. This just waits for all bios to make it through
> > + * blkdev_copy_(offload/emulate)_(read/write)_endio.
> > + */
> > +static ssize_t blkdev_copy_wait_io_completion(struct cio *cio)
> > +{
> > +	ssize_t ret;
> > +
> > +	if (cio->endio)
> > +		return 0;
> > +
> > +	if (atomic_read(&cio->refcount)) {
> > +		__set_current_state(TASK_UNINTERRUPTIBLE);
> > +		blk_io_schedule();
> > +	}
> > +
> > +	ret = cio->comp_len;
> > +	kfree(cio);
> > +
> > +	return ret;
> > +}
> > +
> > +static void blkdev_copy_offload_read_endio(struct bio *bio)
> > +{
> > +	struct cio *cio = bio->bi_private;
> > +	sector_t clen;
> > +
> > +	if (bio->bi_status) {
> > +		clen = (bio->bi_iter.bi_sector << SECTOR_SHIFT) - cio->pos_out;
> > +		cio->comp_len = min_t(sector_t, clen, cio->comp_len);
> > +	}
> > +	bio_put(bio);
> > +
> > +	if (!atomic_dec_and_test(&cio->refcount))
> > +		return;
> > +	if (cio->endio) {
> > +		cio->endio(cio->private, cio->comp_len);
> > +		kfree(cio);
> > +	} else
> > +		blk_wake_io_task(cio->waiter);
>
> Curly brackets around else missing.

Acked.

> > +}
> > +
> > +/*
> > + * __blkdev_copy_offload	- Use device's native copy offload feature.
> > + * we perform copy operation by sending 2 bio.
> > + * 1. We take a plug and send a REQ_OP_COPY_DST bio along with destination
> > + * sector and length. Once this bio reaches request layer, we form a request and
> > + * wait for src bio to arrive.
> > + * 2. We issue REQ_OP_COPY_SRC bio along with source sector and length. Once
> > + * this bio reaches request layer and find a request with previously sent
> > + * destination info we merge the source bio and return.
> > + * 3. Release the plug and request is sent to driver
> > + *
> > + * Returns the length of bytes copied or error if encountered
> > + */
> > +static ssize_t __blkdev_copy_offload(
> > +		struct block_device *bdev_in, loff_t pos_in,
> > +		struct block_device *bdev_out, loff_t pos_out,
> > +		size_t len, cio_iodone_t endio, void *private, gfp_t gfp_mask)
> > +{
> > +	struct cio *cio;
> > +	struct bio *read_bio, *write_bio;
> > +	sector_t rem, copy_len, max_copy_len;
> > +	struct blk_plug plug;
> > +
> > +	cio = kzalloc(sizeof(struct cio), GFP_KERNEL);
> > +	if (!cio)
> > +		return -ENOMEM;
> > +	atomic_set(&cio->refcount, 0);
> > +	cio->waiter = current;
> > +	cio->endio = endio;
> > +	cio->private = private;
> > +
> > +	max_copy_len = min(bdev_max_copy_sectors(bdev_in),
> > +			bdev_max_copy_sectors(bdev_out)) << SECTOR_SHIFT;
>
> According to patch 1, this can end up being 0, so the loop below will be infinite.

Agreed. As you suggested earlier, once we remove copy_offload parameter
and checking copy_max_sector to identify copy offload capabilty should
solve this.

Thank you,
Nitesh Shetty