Message-ID: <20230922095650epcms2p8e25340eff5de01f8b3ce63ae81266881@epcms2p8>
Date:   Fri, 22 Sep 2023 18:56:50 +0900
From:   Jinyoung Choi <j-young.choi@...sung.com>
To:     Nitesh Jagadeesh Shetty <nj.shetty@...sung.com>,
        Jens Axboe <axboe@...nel.dk>, Jonathan Corbet <corbet@....net>,
        Alasdair Kergon <agk@...hat.com>,
        Mike Snitzer <snitzer@...nel.org>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        Keith Busch <kbusch@...nel.org>,
        Christoph Hellwig <hch@....de>,
        Sagi Grimberg <sagi@...mberg.me>,
        Chaitanya Kulkarni <kch@...dia.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>
CC:     "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "nitheshshetty@...il.com" <nitheshshetty@...il.com>,
        "anuj1072538@...il.com" <anuj1072538@...il.com>,
        SSDR Gost Dev <gost.dev@...sung.com>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        Hannes Reinecke <hare@...e.de>,
        Anuj Gupta <anuj20.g@...sung.com>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: RE: [PATCH v16 03/12] block: add copy offload support

> +/*
> + * 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_io_completion(struct blkdev_copy_io *cio)

Hi, Nitesh,

don't functions waiting for completion usually set their names to 'wait_for_completion_'?
(e.g. blkdev_copy_wait_for_completion_io)


> +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;
> +        ssize_t rem, chunk, ret;
> +        ssize_t max_copy_bytes = bdev_max_copy_sectors(bdev) << SECTOR_SHIFT;

wouldn't it be better to use size_t for variables that don't return?
values such as chunk and max_copy_bytes may be defined as 'unsigned'.

> +        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_KERNEL);
> +        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_KERNEL);
> +                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;
> +
> +                src_bio = bio_alloc(bdev, 0, REQ_OP_COPY_SRC, gfp);
> +                if (!src_bio)
> +                        goto err_free_offload_io;
> +                src_bio->bi_iter.bi_size = chunk;
> +                src_bio->bi_iter.bi_sector = pos_in >> SECTOR_SHIFT;
> +
> +                blk_start_plug(&plug);
> +                dst_bio = blk_next_bio(src_bio, bdev, 0, REQ_OP_COPY_DST, gfp);
> +                if (!dst_bio)
> +                        goto err_free_src_bio;
> +                dst_bio->bi_iter.bi_size = chunk;
> +                dst_bio->bi_iter.bi_sector = pos_out >> SECTOR_SHIFT;
> +                dst_bio->bi_end_io = blkdev_copy_offload_dst_endio;
> +                dst_bio->bi_private = offload_io;
> +
> +                atomic_inc(&cio->refcount);
> +                submit_bio(dst_bio);
> +                blk_finish_plug(&plug);
> +                pos_in += chunk;
> +                pos_out += chunk;
> +        }
> +
> +        if (atomic_dec_and_test(&cio->refcount))
> +                blkdev_copy_endio(cio);
> +        if (cio->endio)

Isn't it a problem if the memory of cio is released in blkdev_copy_endio()?
It is unlikely to occur if there is an inflight i/o earlier,
it would be nice to modify considering code flow.


> +                return -EIOCBQUEUED;
> +
> +        return blkdev_copy_wait_io_completion(cio);
> +
> +err_free_src_bio:
> +        bio_put(src_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) {
> +                kfree(cio);
> +                return cio->status;

isn't it a problem if the memory of cio is released?

Best Regards,
Jinyoung.

Powered by blists - more mailing lists