| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2082148f-890f-e5f4-c304-b99212aa377e@suse.de>
Date: Wed, 27 Apr 2022 12:29:15 +0200
From: Hannes Reinecke <hare@...e.de>
To: Nitesh Shetty <nj.shetty@...sung.com>
Cc: chaitanyak@...dia.com, linux-block@...r.kernel.org,
linux-scsi@...r.kernel.org, dm-devel@...hat.com,
linux-nvme@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
axboe@...nel.dk, msnitzer@...hat.com, bvanassche@....org,
martin.petersen@...cle.com, kbusch@...nel.org, hch@....de,
Frederick.Knight@...app.com, osandov@...com,
lsf-pc@...ts.linux-foundation.org, djwong@...nel.org,
josef@...icpanda.com, clm@...com, dsterba@...e.com, tytso@....edu,
jack@...e.com, nitheshshetty@...il.com, gost.dev@...sung.com,
Arnav Dawn <arnav.dawn@...sung.com>,
Alasdair Kergon <agk@...hat.com>,
Mike Snitzer <snitzer@...nel.org>,
Sagi Grimberg <sagi@...mberg.me>,
James Smart <james.smart@...adcom.com>,
Chaitanya Kulkarni <kch@...dia.com>,
Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
Naohiro Aota <naohiro.aota@....com>,
Johannes Thumshirn <jth@...nel.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 02/10] block: Add copy offload support infrastructure
On 4/26/22 12:12, Nitesh Shetty wrote:
> Introduce blkdev_issue_copy which supports source and destination bdevs,
> and an array of (source, destination and copy length) tuples.
> Introduce REQ_COPY copy offload operation flag. Create a read-write
> bio pair with a token as payload and submitted to the device in order.
> Read request populates token with source specific information which
> is then passed with write request.
> This design is courtesy Mikulas Patocka's token based copy
>
> Larger copy will be divided, based on max_copy_sectors,
> max_copy_range_sector limits.
>
> Signed-off-by: Nitesh Shetty <nj.shetty@...sung.com>
> Signed-off-by: Arnav Dawn <arnav.dawn@...sung.com>
> ---
> block/blk-lib.c | 232 ++++++++++++++++++++++++++++++++++++++
> block/blk.h | 2 +
> include/linux/blk_types.h | 21 ++++
> include/linux/blkdev.h | 2 +
> include/uapi/linux/fs.h | 14 +++
> 5 files changed, 271 insertions(+)
>
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 09b7e1200c0f..ba9da2d2f429 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -117,6 +117,238 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
> }
> EXPORT_SYMBOL(blkdev_issue_discard);
>
> +/*
> + * Wait on 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 bio_copy_end_io. IO
> + * errors are propagated through cio->io_error.
> + */
> +static int cio_await_completion(struct cio *cio)
> +{
> + int ret = 0;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&cio->lock, flags);
> + if (cio->refcount) {
> + cio->waiter = current;
> + __set_current_state(TASK_UNINTERRUPTIBLE);
> + spin_unlock_irqrestore(&cio->lock, flags);
> + blk_io_schedule();
> + /* wake up sets us TASK_RUNNING */
> + spin_lock_irqsave(&cio->lock, flags);
> + cio->waiter = NULL;
> + ret = cio->io_err;
> + }
> + spin_unlock_irqrestore(&cio->lock, flags);
> + kvfree(cio);
> +
> + return ret;
> +}
> +
> +static void bio_copy_end_io(struct bio *bio)
> +{
> + struct copy_ctx *ctx = bio->bi_private;
> + struct cio *cio = ctx->cio;
> + sector_t clen;
> + int ri = ctx->range_idx;
> + unsigned long flags;
> + bool wake = false;
> +
> + if (bio->bi_status) {
> + cio->io_err = bio->bi_status;
> + clen = (bio->bi_iter.bi_sector << SECTOR_SHIFT) - ctx->start_sec;
> + cio->rlist[ri].comp_len = min_t(sector_t, clen, cio->rlist[ri].comp_len);
> + }
> + __free_page(bio->bi_io_vec[0].bv_page);
> + kfree(ctx);
> + bio_put(bio);
> +
> + spin_lock_irqsave(&cio->lock, flags);
> + if (((--cio->refcount) <= 0) && cio->waiter)
> + wake = true;
> + spin_unlock_irqrestore(&cio->lock, flags);
> + if (wake)
> + wake_up_process(cio->waiter);
> +}
> +
> +/*
> + * blk_copy_offload - Use device's native copy offload feature
> + * Go through user provide payload, prepare new payload based on device's copy offload limits.
> + */
> +int blk_copy_offload(struct block_device *src_bdev, int nr_srcs,
> + struct range_entry *rlist, struct block_device *dst_bdev, gfp_t gfp_mask)
> +{
> + struct request_queue *sq = bdev_get_queue(src_bdev);
> + struct request_queue *dq = bdev_get_queue(dst_bdev);
> + struct bio *read_bio, *write_bio;
> + struct copy_ctx *ctx;
> + struct cio *cio;
> + struct page *token;
> + sector_t src_blk, copy_len, dst_blk;
> + sector_t remaining, max_copy_len = LONG_MAX;
> + unsigned long flags;
> + int ri = 0, ret = 0;
> +
> + cio = kzalloc(sizeof(struct cio), GFP_KERNEL);
> + if (!cio)
> + return -ENOMEM;
> + cio->rlist = rlist;
> + spin_lock_init(&cio->lock);
> +
> + max_copy_len = min_t(sector_t, sq->limits.max_copy_sectors, dq->limits.max_copy_sectors);
> + max_copy_len = min3(max_copy_len, (sector_t)sq->limits.max_copy_range_sectors,
> + (sector_t)dq->limits.max_copy_range_sectors) << SECTOR_SHIFT;
> +
> + for (ri = 0; ri < nr_srcs; ri++) {
> + cio->rlist[ri].comp_len = rlist[ri].len;
> + src_blk = rlist[ri].src;
> + dst_blk = rlist[ri].dst;
> + for (remaining = rlist[ri].len; remaining > 0; remaining -= copy_len) {
> + copy_len = min(remaining, max_copy_len);
> +
> + token = alloc_page(gfp_mask);
> + if (unlikely(!token)) {
> + ret = -ENOMEM;
> + goto err_token;
> + }
> +
> + ctx = kzalloc(sizeof(struct copy_ctx), gfp_mask);
> + if (!ctx) {
> + ret = -ENOMEM;
> + goto err_ctx;
> + }
> + ctx->cio = cio;
> + ctx->range_idx = ri;
> + ctx->start_sec = dst_blk;
> +
> + read_bio = bio_alloc(src_bdev, 1, REQ_OP_READ | REQ_COPY | REQ_NOMERGE,
> + gfp_mask);
> + if (!read_bio) {
> + ret = -ENOMEM;
> + goto err_read_bio;
> + }
> + read_bio->bi_iter.bi_sector = src_blk >> SECTOR_SHIFT;
> + __bio_add_page(read_bio, token, PAGE_SIZE, 0);
> + /*__bio_add_page increases bi_size by len, so overwrite it with copy len*/
> + read_bio->bi_iter.bi_size = copy_len;
> + ret = submit_bio_wait(read_bio);
> + bio_put(read_bio);
> + if (ret)
> + goto err_read_bio;
> +
> + write_bio = bio_alloc(dst_bdev, 1, REQ_OP_WRITE | REQ_COPY | REQ_NOMERGE,
> + gfp_mask);
> + if (!write_bio) {
> + ret = -ENOMEM;
> + goto err_read_bio;
> + }
> + write_bio->bi_iter.bi_sector = dst_blk >> SECTOR_SHIFT;
> + __bio_add_page(write_bio, token, PAGE_SIZE, 0);
> + /*__bio_add_page increases bi_size by len, so overwrite it with copy len*/
> + write_bio->bi_iter.bi_size = copy_len;
> + write_bio->bi_end_io = bio_copy_end_io;
> + write_bio->bi_private = ctx;
> +
> + spin_lock_irqsave(&cio->lock, flags);
> + ++cio->refcount;
> + spin_unlock_irqrestore(&cio->lock, flags);
> +
> + submit_bio(write_bio);
> + src_blk += copy_len;
> + dst_blk += copy_len;
> + }
> + }
> +
Hmm. I'm not sure if I like the copy loop.
What I definitely would do is to allocate the write bio before reading
data; after all, if we can't allocate the write bio reading is pretty
much pointless.
But the real issue I have with this is that it's doing synchronous
reads, thereby limiting the performance.
Can't you submit the write bio from the end_io function of the read bio?
That would disentangle things, and we should be getting a better
performance.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@...e.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Powered by blists - more mailing lists