linux-kernel - RE: [PATCH v16 04/12] block: add emulation for copy

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Sep 2023 22:08:15 +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>,
        Vincent Kang Fu <vincent.fu@...sung.com>,
        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 04/12] block: add emulation for copy

> +static void blkdev_copy_emulation_work(struct work_struct *work)
> +{
> +        struct blkdev_copy_emulation_io *emulation_io = container_of(work,
> +                        struct blkdev_copy_emulation_io, emulation_work);
> +        struct blkdev_copy_io *cio = emulation_io->cio;
> +        struct bio *read_bio, *write_bio;
> +        loff_t pos_in = emulation_io->pos_in, pos_out = emulation_io->pos_out;
> +        ssize_t rem, chunk;
> +        int ret = 0;
> +
> +        for (rem = emulation_io->len; rem > 0; rem -= chunk) {
> +                chunk = min_t(int, emulation_io->buf_len, rem);
> +
> +                read_bio = bio_map_buf(emulation_io->buf,
> +                                       emulation_io->buf_len,
> +                                       emulation_io->gfp);
> +                if (IS_ERR(read_bio)) {
> +                        ret = PTR_ERR(read_bio);
> +                        break;
> +                }
> +                read_bio->bi_opf = REQ_OP_READ | REQ_SYNC;
> +                bio_set_dev(read_bio, emulation_io->bdev_in);
> +                read_bio->bi_iter.bi_sector = pos_in >> SECTOR_SHIFT;
> +                read_bio->bi_iter.bi_size = chunk;
> +                ret = submit_bio_wait(read_bio);
> +                kfree(read_bio);

Hi, Nitesh,

blk_mq_map_bio_put(read_bio)?
or bio_uninit(read_bio); kfree(read_bio)?

> +                if (ret)
> +                        break;
> +
> +                write_bio = bio_map_buf(emulation_io->buf,
> +                                        emulation_io->buf_len,
> +                                        emulation_io->gfp);
> +                if (IS_ERR(write_bio)) {
> +                        ret = PTR_ERR(write_bio);
> +                        break;
> +                }
> +                write_bio->bi_opf = REQ_OP_WRITE | REQ_SYNC;
> +                bio_set_dev(write_bio, emulation_io->bdev_out);
> +                write_bio->bi_iter.bi_sector = pos_out >> SECTOR_SHIFT;
> +                write_bio->bi_iter.bi_size = chunk;
> +                ret = submit_bio_wait(write_bio);
> +                kfree(write_bio);

blk_mq_map_bio_put(write_bio) ?
or bio_uninit(write_bio); kfree(write_bio)?

hmm... 
It continuously allocates and releases memory for bio,
Why don't you just allocate and reuse bio outside the loop?

> +                if (ret)
> +                        break;
> +
> +                pos_in += chunk;
> +                pos_out += chunk;
> +        }
> +        cio->status = ret;
> +        kvfree(emulation_io->buf);
> +        kfree(emulation_io);

I have not usually seen an implementation that releases memory for
itself while performing a worker. ( I don't know what's right. :) )

Since blkdev_copy_emulation() allocates memory for the emulation 
and waits for it to be completed, wouldn't it be better to proceed
with the memory release for it in the same context?

That is, IMO, wouldn't it be better to free the memory related to
emulation in blkdev_copy_wait_io_completion()?

Best Regards,
Jinyoung.