| Message ID | 20230906163844.18754-10-nj.shetty@samsung.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | Implement copy offload support | expand |
On 9/6/23 18:38, Nitesh Shetty wrote: > Before enabling copy for dm target, check if underlying devices and > dm target support copy. Avoid split happening inside dm target. > Fail early if the request needs split, currently splitting copy > request is not supported. > And here is where I would have expected the emulation to take place; didn't you have it in one of the earlier iterations? After all, device-mapper already has the infrastructure for copying data between devices, so adding a copy-offload emulation for device-mapper should be trivial. Cheers, Hannes
On Fri, Sep 08, 2023 at 08:13:37AM +0200, Hannes Reinecke wrote: > On 9/6/23 18:38, Nitesh Shetty wrote: > > Before enabling copy for dm target, check if underlying devices and > > dm target support copy. Avoid split happening inside dm target. > > Fail early if the request needs split, currently splitting copy > > request is not supported. > > > And here is where I would have expected the emulation to take place; > didn't you have it in one of the earlier iterations? No, but it was the other way round. In dm-kcopyd we used device offload, if that was possible, before using default dm-mapper copy. It was dropped in the current series, to streamline the patches and make the series easier to review. > After all, device-mapper already has the infrastructure for copying > data between devices, so adding a copy-offload emulation for device-mapper > should be trivial. I did not understand this, can you please elaborate ? Thank you, Nitesh Shetty -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On 9/11/23 09:07, Nitesh Shetty wrote: > On Fri, Sep 08, 2023 at 08:13:37AM +0200, Hannes Reinecke wrote: >> On 9/6/23 18:38, Nitesh Shetty wrote: >>> Before enabling copy for dm target, check if underlying devices and >>> dm target support copy. Avoid split happening inside dm target. >>> Fail early if the request needs split, currently splitting copy >>> request is not supported. >>> >> And here is where I would have expected the emulation to take place; >> didn't you have it in one of the earlier iterations? > > No, but it was the other way round. > In dm-kcopyd we used device offload, if that was possible, before using default > dm-mapper copy. It was dropped in the current series, > to streamline the patches and make the series easier to review. > >> After all, device-mapper already has the infrastructure for copying >> data between devices, so adding a copy-offload emulation for device-mapper >> should be trivial. > I did not understand this, can you please elaborate ? > Please see my comments to patch 04. We should only implement copy-offload if there is a dedicated infrastructure in place. But we should not have a 'generic' copy-offload emulation. Problem is that 'real' copy-offload functionalities (ie for NVMe or SCSI) are riddled with corner-cases where copy-offload does _not_ work, and where commands might fail if particular conditions are not met. Falling back to a generic implementation will cause applications to assume that copy-offload worked, and that it gained performance as the application just had to issue a single command. Whereas in fact the opposite is true; it wasn't a single command, and the application might have performed better by issuing the commands itself. Returning -EOPNOTSUPP in these cases will inform the application that the attempt didn't work, and that it will have to fall back to the 'normal' copy. Cheers, Hannes
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 7d208b2b1a19..a192c19b68e4 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -1862,6 +1862,38 @@ static bool dm_table_supports_nowait(struct dm_table *t) return true; } +static int device_not_copy_capable(struct dm_target *ti, struct dm_dev *dev, + sector_t start, sector_t len, void *data) +{ + struct request_queue *q = bdev_get_queue(dev->bdev); + + return !q->limits.max_copy_sectors; +} + +static bool dm_table_supports_copy(struct dm_table *t) +{ + struct dm_target *ti; + unsigned int i; + + for (i = 0; i < t->num_targets; i++) { + ti = dm_table_get_target(t, i); + + if (!ti->copy_offload_supported) + return false; + + /* + * target provides copy support (as implied by setting + * 'copy_offload_supported') + * and it relies on _all_ data devices having copy support. + */ + if (!ti->type->iterate_devices || + ti->type->iterate_devices(ti, device_not_copy_capable, NULL)) + return false; + } + + return true; +} + static int device_not_discard_capable(struct dm_target *ti, struct dm_dev *dev, sector_t start, sector_t len, void *data) { @@ -1944,6 +1976,11 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q, q->limits.discard_misaligned = 0; } + if (!dm_table_supports_copy(t)) { + q->limits.max_copy_sectors = 0; + q->limits.max_copy_hw_sectors = 0; + } + if (!dm_table_supports_secure_erase(t)) q->limits.max_secure_erase_sectors = 0; diff --git a/drivers/md/dm.c b/drivers/md/dm.c index f0f118ab20fa..f9d6215e6d4d 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1732,6 +1732,13 @@ static blk_status_t __split_and_process_bio(struct clone_info *ci) if (unlikely(ci->is_abnormal_io)) return __process_abnormal_io(ci, ti); + if ((unlikely(op_is_copy(ci->bio->bi_opf)) && + max_io_len(ti, ci->sector) < ci->sector_count)) { + DMERR("Error, IO size(%u) > max target size(%llu)\n", + ci->sector_count, max_io_len(ti, ci->sector)); + return BLK_STS_IOERR; + } + /* * Only support bio polling for normal IO, and the target io is * exactly inside the dm_io instance (verified in dm_poll_dm_io) diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h index 69d0435c7ebb..98db52d1c773 100644 --- a/include/linux/device-mapper.h +++ b/include/linux/device-mapper.h @@ -396,6 +396,9 @@ struct dm_target { * bio_set_dev(). NOTE: ideally a target should _not_ need this. */ bool needs_bio_set_dev:1; + + /* copy offload is supported */ + bool copy_offload_supported:1; }; void *dm_per_bio_data(struct bio *bio, size_t data_size);
Before enabling copy for dm target, check if underlying devices and dm target support copy. Avoid split happening inside dm target. Fail early if the request needs split, currently splitting copy request is not supported. Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com> --- drivers/md/dm-table.c | 37 +++++++++++++++++++++++++++++++++++ drivers/md/dm.c | 7 +++++++ include/linux/device-mapper.h | 3 +++ 3 files changed, 47 insertions(+)