Re: [PATCH v20 03/12] block: add copy offload support

Re: [PATCH v20 03/12] block: add copy offload support

[Christoph Hellwig]

On Tue, Jun 04, 2024 at 10:50:26AM +0000, Nitesh Shetty wrote:
>>> +	if (!cio)
>>> +		return -ENOMEM;
>>> +	atomic_set(&cio->refcount, 1);
>>> +	cio->waiter = current;
>>> +	cio->endio = endio;
>>> +	cio->private = private;
>>
>> For the main use this could be allocated on-stack.  Is there any good
>> reason to not let callers that really want an async version to implement
>> the async behavior themselves using suitable helpers?
>>
> We cannot do on-stack allocation of cio as we use it in endio handler.
> cio will be used to track partial IO completion as well.
> Callers requiring async implementation would need to manage all this
> bookkeeping themselves, leading to duplication of code. We felt it is
> better to do it here onetime.
> Do you see it any differently ?

We don't really to these async variants for other in-kernel I/O,
so unless we have a really good reason I'd not do here.  The usual
approach is to just have a submission helper for async in-kernel
special bios types, and the let the caller handle the rest.

[PATCH v20 00/12] Implement copy offload support

[Nitesh Shetty]

The patch series covers the points discussed in the past and most recently
in LSFMM'24[0].
We have covered the initial agreed requirements in this patch set and
further additional features suggested by the community.

This is next iteration of our previous patch set v19[1].
Copy offload is performed using two bio's -
1. Take a plug
2. The first bio containing destination info is prepared and sent,
        a request is formed.
3. This is followed by preparing and sending the second bio containing the
        source info.
4. This bio is merged with the request containing the destination info.
5. The plug is released, and the request containing source and destination
        bio's is sent to the driver.
This design helps to avoid putting payload (token) in the request,
as sending payload that is not data to the device is considered a bad
approach.

So copy offload works only for request based storage drivers.
We can make use of copy emulation in case copy offload capability is
absent.

Overall series supports:
========================
	1. Driver
		- NVMe Copy command (single NS, TP 4065), including support
		in nvme-target (for block and file back end).

	2. Block layer
		- Block-generic copy (REQ_OP_COPY_DST/SRC), operation with
                  interface accommodating two block-devs
                - Merging copy requests in request layer
		- Emulation, for in-kernel user when offload is natively
                absent
		- dm-linear support (for cases not requiring split)

	3. User-interface
		- copy_file_range

Testing
=======
	Copy offload can be tested on:
	a. QEMU: NVME simple copy (TP 4065). By setting nvme-ns
		parameters mssrl,mcl, msrc. For more info [2].
	b. Null block device
        c. NVMe Fabrics loopback.
	d. blktests[3]

	Emulation can be tested on any device.

	fio[4].

Infra and plumbing:
===================
        We populate copy_file_range callback in def_blk_fops.
        For devices that support copy-offload, use blkdev_copy_offload to
        achieve in-device copy.
        However for cases, where device doesn't support offload,
        use splice_copy_file_range.
        For in-kernel users (like NVMe fabrics), use blkdev_copy_offload
        if device is copy offload capable or else use emulation
        using blkdev_copy_emulation.
        Modify checks in copy_file_range to support block-device.

Blktests[3]
======================
	tests/block/035-040: Runs copy offload and emulation on null
                              block device.
	tests/block/050,055: Runs copy offload and emulation on test
                              nvme block device.
        tests/nvme/056-067: Create a loop backed fabrics device and
                              run copy offload and emulation.

Future Work
===========
	- loopback device copy offload support
	- upstream fio to use copy offload
	- upstream blktest to test copy offload
        - update man pages for copy_file_range
        - expand in-kernel users of copy offload

	These are to be taken up after this minimal series is agreed upon.

Additional links:
=================
	[0] https://lore.kernel.org/linux-nvme/CA+1E3rJ7BZ7LjQXXTdX+-0Edz=zT14mmPGMiVCzUgB33C60tbQ@mail.gmail.com/
            https://lore.kernel.org/linux-nvme/f0e19ae4-b37a-e9a3-2be7-a5afb334a5c3@nvidia.com/
            https://lore.kernel.org/linux-nvme/20230113094648.15614-1-nj.shetty@samsung.com/
	[1] https://lore.kernel.org/linux-nvme/20231222061313.12260-1-nj.shetty@samsung.com/
	[2] https://qemu-project.gitlab.io/qemu/system/devices/nvme.html#simple-copy
	[3] https://github.com/nitesh-shetty/blktests/tree/feat/copy_offload/v19
	[4] https://github.com/SamsungDS/fio/tree/copyoffload-3.35-v14

Changes since v19:
=================
        - block, nvme: update queue limits atomically
                Also remove reviewed by tag from Hannes and Luis for
                these patches. As we feel these patches changed
                significantly from the previous one.
        - vfs: generic_copy_file_range to splice_file_range
        - rebased to latest linux-next

Changes since v18:
=================
        - block, nvmet, null: change of copy dst/src request opcodes form
            19,21 to 18,19 (Keith Busch)
            Change the copy bio submission order to destination copy bio
            first, followed by source copy bio.

Changes since v17:
=================
        - block, nvmet: static error fixes (Dan Carpenter, kernel test robot)
        - nvmet: pass COPY_FILE_SPLICE flag for vfs_copy_file_range in case
                        file backed nvmet device

Changes since v16:
=================
        - block: fixed memory leaks and renamed function (Jinyoung Choi)
        - nvmet: static error fixes (kernel test robot)

Changes since v15:
=================
        - fs, nvmet: don't fallback to copy emulation for copy offload IO
                    failure, user can still use emulation by disabling
                    device offload (Hannes)
        - block: patch,function description changes (Hannes)
        - added Reviewed-by from Hannes and Luis.

Changes since v14:
=================
        - block: (Bart Van Assche)
            1. BLK_ prefix addition to COPY_MAX_BYES and COPY_MAX_SEGMENTS
            2. Improved function,patch,cover-letter description
            3. Simplified refcount updating.
        - null-blk, nvme:
            4. static warning fixes (kernel test robot)

Changes since v13:
=================
        - block:
            1. Simplified copy offload and emulation helpers, now
                  caller needs to decide between offload/emulation fallback
            2. src,dst bio order change (Christoph Hellwig)
            3. refcount changes similar to dio (Christoph Hellwig)
            4. Single outstanding IO for copy emulation (Christoph Hellwig)
            5. use copy_max_sectors to identify copy offload
                  capability and other reviews (Damien, Christoph)
            6. Return status in endio handler (Christoph Hellwig)
        - nvme-fabrics: fallback to emulation in case of partial
          offload completion
        - in kernel user addition (Ming lei)
        - indentation, documentation, minor fixes, misc changes (Damien,
          Christoph)
        - blktests changes to test kernel changes

Changes since v12:
=================
        - block,nvme: Replaced token based approach with request based
          single namespace capable approach (Christoph Hellwig)

Changes since v11:
=================
        - Documentation: Improved documentation (Damien Le Moal)
        - block,nvme: ssize_t return values (Darrick J. Wong)
        - block: token is allocated to SECTOR_SIZE (Matthew Wilcox)
        - block: mem leak fix (Maurizio Lombardi)

Changes since v10:
=================
        - NVMeOF: optimization in NVMe fabrics (Chaitanya Kulkarni)
        - NVMeOF: sparse warnings (kernel test robot)

Changes since v9:
=================
        - null_blk, improved documentation, minor fixes(Chaitanya Kulkarni)
        - fio, expanded testing and minor fixes (Vincent Fu)

Changes since v8:
=================
        - null_blk, copy_max_bytes_hw is made config fs parameter
          (Damien Le Moal)
        - Negative error handling in copy_file_range (Christian Brauner)
        - minor fixes, better documentation (Damien Le Moal)
        - fio upgraded to 3.34 (Vincent Fu)

Changes since v7:
=================
        - null block copy offload support for testing (Damien Le Moal)
        - adding direct flag check for copy offload to block device,
	  as we are using generic_copy_file_range for cached cases.
        - Minor fixes

Changes since v6:
=================
        - copy_file_range instead of ioctl for direct block device
        - Remove support for multi range (vectored) copy
        - Remove ioctl interface for copy.
        - Remove offload support in dm kcopyd.

Changes since v5:
=================
	- Addition of blktests (Chaitanya Kulkarni)
        - Minor fix for fabrics file backed path
        - Remove buggy zonefs copy file range implementation.

Changes since v4:
=================
	- make the offload and emulation design asynchronous (Hannes
	  Reinecke)
	- fabrics loopback support
	- sysfs naming improvements (Damien Le Moal)
	- use kfree() instead of kvfree() in cio_await_completion
	  (Damien Le Moal)
	- use ranges instead of rlist to represent range_entry (Damien
	  Le Moal)
	- change argument ordering in blk_copy_offload suggested (Damien
	  Le Moal)
	- removed multiple copy limit and merged into only one limit
	  (Damien Le Moal)
	- wrap overly long lines (Damien Le Moal)
	- other naming improvements and cleanups (Damien Le Moal)
	- correctly format the code example in description (Damien Le
	  Moal)
	- mark blk_copy_offload as static (kernel test robot)

Changes since v3:
=================
	- added copy_file_range support for zonefs
	- added documentation about new sysfs entries
	- incorporated review comments on v3
	- minor fixes

Changes since v2:
=================
	- fixed possible race condition reported by Damien Le Moal
	- new sysfs controls as suggested by Damien Le Moal
	- fixed possible memory leak reported by Dan Carpenter, lkp
	- minor fixes

Changes since v1:
=================
	- sysfs documentation (Greg KH)
        - 2 bios for copy operation (Bart Van Assche, Mikulas Patocka,
          Martin K. Petersen, Douglas Gilbert)
        - better payload design (Darrick J. Wong)

Anuj Gupta (1):
  fs/read_write: Enable copy_file_range for block device.

Nitesh Shetty (11):
  block: Introduce queue limits and sysfs for copy-offload support
  Add infrastructure for copy offload in block and request layer.
  block: add copy offload support
  block: add emulation for copy
  fs, block: copy_file_range for def_blk_ops for direct block device
  nvme: add copy offload support
  nvmet: add copy command support for bdev and file ns
  dm: Add support for copy offload
  dm: Enable copy offload for dm-linear target
  null: Enable trace capability for null block
  null_blk: add support for copy offload

 Documentation/ABI/stable/sysfs-block |  23 ++
 Documentation/block/null_blk.rst     |   5 +
 block/blk-core.c                     |   7 +
 block/blk-lib.c                      | 427 +++++++++++++++++++++++++++
 block/blk-merge.c                    |  41 +++
 block/blk-settings.c                 |  34 ++-
 block/blk-sysfs.c                    |  43 +++
 block/blk.h                          |  16 +
 block/elevator.h                     |   1 +
 block/fops.c                         |  26 ++
 drivers/block/null_blk/Makefile      |   2 -
 drivers/block/null_blk/main.c        | 105 ++++++-
 drivers/block/null_blk/null_blk.h    |   1 +
 drivers/block/null_blk/trace.h       |  25 ++
 drivers/block/null_blk/zoned.c       |   1 -
 drivers/md/dm-linear.c               |   1 +
 drivers/md/dm-table.c                |  37 +++
 drivers/md/dm.c                      |   7 +
 drivers/nvme/host/constants.c        |   1 +
 drivers/nvme/host/core.c             |  81 ++++-
 drivers/nvme/host/trace.c            |  19 ++
 drivers/nvme/target/admin-cmd.c      |   9 +-
 drivers/nvme/target/io-cmd-bdev.c    |  71 +++++
 drivers/nvme/target/io-cmd-file.c    |  50 ++++
 drivers/nvme/target/nvmet.h          |   1 +
 drivers/nvme/target/trace.c          |  19 ++
 fs/read_write.c                      |   8 +-
 include/linux/bio.h                  |   6 +-
 include/linux/blk_types.h            |  10 +
 include/linux/blkdev.h               |  23 ++
 include/linux/device-mapper.h        |   3 +
 include/linux/nvme.h                 |  43 ++-
 32 files changed, 1124 insertions(+), 22 deletions(-)


base-commit: dbd9e2e056d8577375ae4b31ada94f8aa3769e8a
-- 
2.17.1

[PATCH v20 03/12] block: add copy offload support

[Nitesh Shetty]

Introduce blkdev_copy_offload to perform copy offload.
Issue REQ_OP_COPY_DST with destination info along with taking a plug.
This flows till request layer and waits for src bio to arrive.
Issue REQ_OP_COPY_SRC with source info and this bio reaches request
layer and merges with dst request.
For any reason, if a request comes to the driver with only one of src/dst
bio, we fail the copy offload.

Larger copy will be divided, based on max_copy_sectors limit.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 block/blk-lib.c        | 204 +++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |   4 +
 2 files changed, 208 insertions(+)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 442da9dad042..e83461abb581 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -10,6 +10,22 @@
 
 #include "blk.h"
 
+/* Keeps track of all outstanding copy IO */
+struct blkdev_copy_io {
+	atomic_t refcount;
+	ssize_t copied;
+	int status;
+	struct task_struct *waiter;
+	void (*endio)(void *private, int status, ssize_t copied);
+	void *private;
+};
+
+/* Keeps track of single outstanding copy offload IO */
+struct blkdev_copy_offload_io {
+	struct blkdev_copy_io *cio;
+	loff_t offset;
+};
+
 static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
 {
 	unsigned int discard_granularity = bdev_discard_granularity(bdev);
@@ -103,6 +119,194 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 }
 EXPORT_SYMBOL(blkdev_issue_discard);
 
+static inline ssize_t blkdev_copy_sanity_check(struct block_device *bdev_in,
+					       loff_t pos_in,
+					       struct block_device *bdev_out,
+					       loff_t pos_out, size_t len)
+{
+	unsigned int align = max(bdev_logical_block_size(bdev_out),
+				 bdev_logical_block_size(bdev_in)) - 1;
+
+	if ((pos_in & align) || (pos_out & align) || (len & align) || !len ||
+	    len >= BLK_COPY_MAX_BYTES)
+		return -EINVAL;
+
+	return 0;
+}
+
+static inline void blkdev_copy_endio(struct blkdev_copy_io *cio)
+{
+	if (cio->endio) {
+		cio->endio(cio->private, cio->status, cio->copied);
+		kfree(cio);
+	} else {
+		struct task_struct *waiter = cio->waiter;
+
+		WRITE_ONCE(cio->waiter, NULL);
+		blk_wake_io_task(waiter);
+	}
+}
+
+/*
+ * 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 complete.
+ * Returns the length of bytes copied or error
+ */
+static ssize_t blkdev_copy_wait_for_completion_io(struct blkdev_copy_io *cio)
+{
+	ssize_t ret;
+
+	for (;;) {
+		__set_current_state(TASK_UNINTERRUPTIBLE);
+		if (!READ_ONCE(cio->waiter))
+			break;
+		blk_io_schedule();
+	}
+	__set_current_state(TASK_RUNNING);
+	ret = cio->copied;
+	kfree(cio);
+
+	return ret;
+}
+
+static void blkdev_copy_offload_src_endio(struct bio *bio)
+{
+	struct blkdev_copy_offload_io *offload_io = bio->bi_private;
+	struct blkdev_copy_io *cio = offload_io->cio;
+
+	if (bio->bi_status) {
+		cio->copied = min_t(ssize_t, offload_io->offset, cio->copied);
+		if (!cio->status)
+			cio->status = blk_status_to_errno(bio->bi_status);
+	}
+	bio_put(bio);
+	kfree(offload_io);
+
+	if (atomic_dec_and_test(&cio->refcount))
+		blkdev_copy_endio(cio);
+}
+
+/*
+ * @bdev:	block device
+ * @pos_in:	source offset
+ * @pos_out:	destination offset
+ * @len:	length in bytes to be copied
+ * @endio:	endio function to be called on completion of copy operation,
+ *		for synchronous operation this should be NULL
+ * @private:	endio function will be called with this private data,
+ *		for synchronous operation this should be NULL
+ * @gfp_mask:	memory allocation flags (for bio_alloc)
+ *
+ * For synchronous operation returns the length of bytes copied or error
+ * For asynchronous operation returns -EIOCBQUEUED or error
+ *
+ * Description:
+ *	Copy source offset to destination offset within block device, using
+ *	device's native copy offload feature.
+ *	We perform copy operation using 2 bio's.
+ *	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 dst bio to arrive.
+ *	2. We issue REQ_OP_COPY_SRC bio along with source sector, 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
+ *	This design works only for drivers with request queue.
+ */
+ssize_t blkdev_copy_offload(struct block_device *bdev, loff_t pos_in,
+			    loff_t pos_out, size_t len,
+			    void (*endio)(void *, int, ssize_t),
+			    void *private, gfp_t gfp)
+{
+	struct blkdev_copy_io *cio;
+	struct blkdev_copy_offload_io *offload_io;
+	struct bio *src_bio, *dst_bio;
+	size_t rem, chunk;
+	size_t max_copy_bytes = bdev_max_copy_sectors(bdev) << SECTOR_SHIFT;
+	ssize_t ret;
+	struct blk_plug plug;
+
+	if (!max_copy_bytes)
+		return -EOPNOTSUPP;
+
+	ret = blkdev_copy_sanity_check(bdev, pos_in, bdev, pos_out, len);
+	if (ret)
+		return ret;
+
+	cio = kzalloc(sizeof(*cio), gfp);
+	if (!cio)
+		return -ENOMEM;
+	atomic_set(&cio->refcount, 1);
+	cio->waiter = current;
+	cio->endio = endio;
+	cio->private = private;
+
+	/*
+	 * If there is a error, copied will be set to least successfully
+	 * completed copied length
+	 */
+	cio->copied = len;
+	for (rem = len; rem > 0; rem -= chunk) {
+		chunk = min(rem, max_copy_bytes);
+
+		offload_io = kzalloc(sizeof(*offload_io), gfp);
+		if (!offload_io)
+			goto err_free_cio;
+		offload_io->cio = cio;
+		/*
+		 * For partial completion, we use offload_io->offset to truncate
+		 * successful copy length
+		 */
+		offload_io->offset = len - rem;
+
+		dst_bio = bio_alloc(bdev, 0, REQ_OP_COPY_DST, gfp);
+		if (!dst_bio)
+			goto err_free_offload_io;
+		dst_bio->bi_iter.bi_size = chunk;
+		dst_bio->bi_iter.bi_sector = pos_out >> SECTOR_SHIFT;
+
+		blk_start_plug(&plug);
+		src_bio = blk_next_bio(dst_bio, bdev, 0, REQ_OP_COPY_SRC, gfp);
+		if (!src_bio)
+			goto err_free_dst_bio;
+		src_bio->bi_iter.bi_size = chunk;
+		src_bio->bi_iter.bi_sector = pos_in >> SECTOR_SHIFT;
+		src_bio->bi_end_io = blkdev_copy_offload_src_endio;
+		src_bio->bi_private = offload_io;
+
+		atomic_inc(&cio->refcount);
+		submit_bio(src_bio);
+		blk_finish_plug(&plug);
+		pos_in += chunk;
+		pos_out += chunk;
+	}
+
+	if (atomic_dec_and_test(&cio->refcount))
+		blkdev_copy_endio(cio);
+	if (endio)
+		return -EIOCBQUEUED;
+
+	return blkdev_copy_wait_for_completion_io(cio);
+
+err_free_dst_bio:
+	bio_put(dst_bio);
+err_free_offload_io:
+	kfree(offload_io);
+err_free_cio:
+	cio->copied = min_t(ssize_t, cio->copied, (len - rem));
+	cio->status = -ENOMEM;
+	if (rem == len) {
+		ret = cio->status;
+		kfree(cio);
+		return ret;
+	}
+	if (cio->endio)
+		return cio->status;
+
+	return blkdev_copy_wait_for_completion_io(cio);
+}
+EXPORT_SYMBOL_GPL(blkdev_copy_offload);
+
 static int __blkdev_issue_write_zeroes(struct block_device *bdev,
 		sector_t sector, sector_t nr_sects, gfp_t gfp_mask,
 		struct bio **biop, unsigned flags)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 109d9f905c3c..3b88dcd5c433 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1079,6 +1079,10 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp_mask, struct bio **biop);
 int blkdev_issue_secure_erase(struct block_device *bdev, sector_t sector,
 		sector_t nr_sects, gfp_t gfp);
+ssize_t blkdev_copy_offload(struct block_device *bdev, loff_t pos_in,
+			    loff_t pos_out, size_t len,
+			    void (*endio)(void *, int, ssize_t),
+			    void *private, gfp_t gfp_mask);
 
 #define BLKDEV_ZERO_NOUNMAP	(1 << 0)  /* do not free blocks */
 #define BLKDEV_ZERO_NOFALLBACK	(1 << 1)  /* don't write explicit zeroes */
-- 
2.17.1

Re: [PATCH v20 03/12] block: add copy offload support

[Christoph Hellwig]

> +/* Keeps track of all outstanding copy IO */
> +struct blkdev_copy_io {
> +	atomic_t refcount;
> +	ssize_t copied;
> +	int status;
> +	struct task_struct *waiter;
> +	void (*endio)(void *private, int status, ssize_t copied);
> +	void *private;
> +};
> +
> +/* Keeps track of single outstanding copy offload IO */
> +struct blkdev_copy_offload_io {
> +	struct blkdev_copy_io *cio;
> +	loff_t offset;
> +};

The structure names confuse me, and the comments make things even worse.

AFAICT:

blkdev_copy_io is a per-call structure, I'd name it blkdev_copy_ctx.
blkdev_copy_offload_io is per-bio pair, and something like blkdev_copy_chunk
might be a better idea.  Or we could just try to kill it entirely and add
a field to struct bio in the union currently holding the integrity
information.

I'm also quite confused what kind of offset this offset field is.  The
type and name suggest it is an offset in a file, which for a block device
based helper is pretty odd to start with.  blkdev_copy_offload
initializes it to len - rem, so it kind is an offset, but relative
to the operation and not to a file. blkdev_copy_offload_src_endio then
uses to set the ->copied field, but based on a min which means
->copied can only be decreased.  Something is really off there.

Taking about types and units: blkdev_copy_offload obviously can only
work in terms of LBAs.  Any reason to not make it work in terms of
512-byte block layer sectors instead of in bytes?

> +	if ((pos_in & align) || (pos_out & align) || (len & align) || !len ||
> +	    len >= BLK_COPY_MAX_BYTES)
> +		return -EINVAL;

This can be cleaned up an optimized a bit:

	if (!len || len >= BLK_COPY_MAX_BYTES)
		return -EINVAL;
	if ((pos_in | pos_out | len) & align)
		return -EINVAL;
	
> + *
> + * For synchronous operation returns the length of bytes copied or error
> + * For asynchronous operation returns -EIOCBQUEUED or error
> + *
> + * Description:
> + *	Copy source offset to destination offset within block device, using
> + *	device's native copy offload feature.
> + *	We perform copy operation using 2 bio's.
> + *	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 dst bio to arrive.
> + *	2. We issue REQ_OP_COPY_SRC bio along with source sector, 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
> + *	This design works only for drivers with request queue.

The wording with all the We here is a bit odd.  Much of this also seem
superfluous or at least misplaced in the kernel doc comment as it doesn't
document the API, but just what is done in the code below.

> +	cio = kzalloc(sizeof(*cio), gfp);
> +	if (!cio)
> +		return -ENOMEM;
> +	atomic_set(&cio->refcount, 1);
> +	cio->waiter = current;
> +	cio->endio = endio;
> +	cio->private = private;

For the main use this could be allocated on-stack.  Is there any good
reason to not let callers that really want an async version to implement
the async behavior themselves using suitable helpers?

> +		src_bio = blk_next_bio(dst_bio, bdev, 0, REQ_OP_COPY_SRC, gfp);

Please switch to use bio_chain_and_submit, which is a easier to
understand API.  I'm trying to phase out blk_next_bio in favour of
bio_chain_and_submit over the next few merge windows.

> +		if (!src_bio)
> +			goto err_free_dst_bio;
> +		src_bio->bi_iter.bi_size = chunk;
> +		src_bio->bi_iter.bi_sector = pos_in >> SECTOR_SHIFT;
> +		src_bio->bi_end_io = blkdev_copy_offload_src_endio;
> +		src_bio->bi_private = offload_io;
> +
> +		atomic_inc(&cio->refcount);
> +		submit_bio(src_bio);
> +		blk_finish_plug(&plug);

plugs should be hold over all  I/Os, submitted from the same caller,
which is the point of them.

Re: [PATCH v20 03/12] block: add copy offload support

[Nitesh Shetty]

[-- Attachment #1: Type: text/plain, Size: 5167 bytes --]

On 01/06/24 08:16AM, Christoph Hellwig wrote:
>> +/* Keeps track of all outstanding copy IO */
>> +struct blkdev_copy_io {
>> +	atomic_t refcount;
>> +	ssize_t copied;
>> +	int status;
>> +	struct task_struct *waiter;
>> +	void (*endio)(void *private, int status, ssize_t copied);
>> +	void *private;
>> +};
>> +
>> +/* Keeps track of single outstanding copy offload IO */
>> +struct blkdev_copy_offload_io {
>> +	struct blkdev_copy_io *cio;
>> +	loff_t offset;
>> +};
>
>The structure names confuse me, and the comments make things even worse.
>
>AFAICT:
>
>blkdev_copy_io is a per-call structure, I'd name it blkdev_copy_ctx.
>blkdev_copy_offload_io is per-bio pair, and something like blkdev_copy_chunk
Acked, your suggestion for structure name looks better.

>might be a better idea.  Or we could just try to kill it entirely and add
>a field to struct bio in the union currently holding the integrity
>information.
We will explore this.

>I'm also quite confused what kind of offset this offset field is.  The
>type and name suggest it is an offset in a file, which for a block device
>based helper is pretty odd to start with.  blkdev_copy_offload
>initializes it to len - rem, so it kind is an offset, but relative
>to the operation and not to a file. blkdev_copy_offload_src_endio then
>uses to set the ->copied field, but based on a min which means
>->copied can only be decreased.  Something is really off there.
>
Offset in this context, is with respect to the operation.
Overall idea was to handle partial copy, where in some of the split copy IO fails.
In this case we want to return minimum bytes copied.
We can try to store the offset in a temporary variable similar to
pos_out, pos_in instead of current (len - rem), to avoid the confusion.
We will update this in next version.

>Taking about types and units: blkdev_copy_offload obviously can only
>work in terms of LBAs.  Any reason to not make it work in terms of
>512-byte block layer sectors instead of in bytes?
>
Just that number of places where we need to sector shift were
comparatively more. We will update this to 512-byte sectors in next
version.

>> +	if ((pos_in & align) || (pos_out & align) || (len & align) || !len ||
>> +	    len >= BLK_COPY_MAX_BYTES)
>> +		return -EINVAL;
>
>This can be cleaned up an optimized a bit:
>
>	if (!len || len >= BLK_COPY_MAX_BYTES)
>		return -EINVAL;
>	if ((pos_in | pos_out | len) & align)
>		return -EINVAL;
>	
Acked.

>> + *
>> + * For synchronous operation returns the length of bytes copied or error
>> + * For asynchronous operation returns -EIOCBQUEUED or error
>> + *
>> + * Description:
>> + *	Copy source offset to destination offset within block device, using
>> + *	device's native copy offload feature.
>> + *	We perform copy operation using 2 bio's.
>> + *	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 dst bio to arrive.
>> + *	2. We issue REQ_OP_COPY_SRC bio along with source sector, 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
>> + *	This design works only for drivers with request queue.
>
>The wording with all the We here is a bit odd.  Much of this also seem
>superfluous or at least misplaced in the kernel doc comment as it doesn't
>document the API, but just what is done in the code below.
>
Since we were doing IO in unconventional way, we felt would be better to
document this, for easy followup.
We will remove this in next version and document just API.

>> +	cio = kzalloc(sizeof(*cio), gfp);
>> +	if (!cio)
>> +		return -ENOMEM;
>> +	atomic_set(&cio->refcount, 1);
>> +	cio->waiter = current;
>> +	cio->endio = endio;
>> +	cio->private = private;
>
>For the main use this could be allocated on-stack.  Is there any good
>reason to not let callers that really want an async version to implement
>the async behavior themselves using suitable helpers?
>
We cannot do on-stack allocation of cio as we use it in endio handler.
cio will be used to track partial IO completion as well.
Callers requiring async implementation would need to manage all this
bookkeeping themselves, leading to duplication of code. We felt it is
better to do it here onetime.
Do you see it any differently ?

>> +		src_bio = blk_next_bio(dst_bio, bdev, 0, REQ_OP_COPY_SRC, gfp);
>
>Please switch to use bio_chain_and_submit, which is a easier to
>understand API.  I'm trying to phase out blk_next_bio in favour of
>bio_chain_and_submit over the next few merge windows.
>
Acked

>> +		if (!src_bio)
>> +			goto err_free_dst_bio;
>> +		src_bio->bi_iter.bi_size = chunk;
>> +		src_bio->bi_iter.bi_sector = pos_in >> SECTOR_SHIFT;
>> +		src_bio->bi_end_io = blkdev_copy_offload_src_endio;
>> +		src_bio->bi_private = offload_io;
>> +
>> +		atomic_inc(&cio->refcount);
>> +		submit_bio(src_bio);
>> +		blk_finish_plug(&plug);
>
>plugs should be hold over all  I/Os, submitted from the same caller,
>which is the point of them.
>
Acked

Thank You,
Nitesh Shetty

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]