| 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: <20230413103610.3calcy37ogf72y6q@green5>
Date: Thu, 13 Apr 2023 16:06:10 +0530
From: Nitesh Shetty <nj.shetty@...sung.com>
To: Chaitanya Kulkarni <chaitanyak@...dia.com>
Cc: Anuj Gupta <anuj20.g@...sung.com>, Jens Axboe <axboe@...nel.dk>,
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>,
James Smart <james.smart@...adcom.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>,
"bvanassche@....org" <bvanassche@....org>,
"hare@...e.de" <hare@...e.de>,
"ming.lei@...hat.com" <ming.lei@...hat.com>,
"dlemoal@...nel.org" <dlemoal@...nel.org>,
"joshi.k@...sung.com" <joshi.k@...sung.com>,
"nitheshshetty@...il.com" <nitheshshetty@...il.com>,
"gost.dev@...sung.com" <gost.dev@...sung.com>,
Damien Le Moal <damien.lemoal@...nsource.wdc.com>,
Vincent Fu <vincent.fu@...sung.com>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...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 v9 9/9] null_blk: add support for copy offload
On 23/04/13 06:28AM, Chaitanya Kulkarni wrote:
>On 4/11/23 01:10, Anuj Gupta wrote:
>> From: Nitesh Shetty <nj.shetty@...sung.com>
>>
>> Implementaion is based on existing read and write infrastructure.
>> copy_max_bytes: A new configfs and module parameter is introduced, which
>> can be used to set hardware/driver supported maximum copy limit.
>>
>> Suggested-by: Damien Le Moal <damien.lemoal@...nsource.wdc.com>
>> Signed-off-by: Anuj Gupta <anuj20.g@...sung.com>
>> Signed-off-by: Nitesh Shetty <nj.shetty@...sung.com>
>> Signed-off-by: Vincent Fu <vincent.fu@...sung.com>
>> ---
>> drivers/block/null_blk/main.c | 101 ++++++++++++++++++++++++++++++
>> drivers/block/null_blk/null_blk.h | 8 +++
>> 2 files changed, 109 insertions(+)
>>
>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>> index bc2c58724df3..e273e18ace74 100644
>> --- a/drivers/block/null_blk/main.c
>> +++ b/drivers/block/null_blk/main.c
>> @@ -157,6 +157,10 @@ static int g_max_sectors;
>> module_param_named(max_sectors, g_max_sectors, int, 0444);
>> MODULE_PARM_DESC(max_sectors, "Maximum size of a command (in 512B sectors)");
>>
>> +static int g_copy_max_bytes = COPY_MAX_BYTES;
>
>how about following ? matches nullb_device->copy_max_bytes type ..
>
>-static int g_copy_max_bytes = COPY_MAX_BYTES;
>-module_param_named(copy_max_bytes, g_copy_max_bytes, int, 0444);
>+static unsigned long g_copy_max_bytes = COPY_MAX_BYTES;
>+module_param_named(copy_max_bytes, g_copy_max_bytes, ulong, 0444);
>
>[...]
>
acked
>> @@ -631,6 +637,7 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page)
>> "badblocks,blocking,blocksize,cache_size,"
>> "completion_nsec,discard,home_node,hw_queue_depth,"
>> "irqmode,max_sectors,mbps,memory_backed,no_sched,"
>> + "copy_max_bytes,"
>> "poll_queues,power,queue_mode,shared_tag_bitmap,size,"
>> "submit_queues,use_per_node_hctx,virt_boundary,zoned,"
>> "zone_capacity,zone_max_active,zone_max_open,"
>
>why not ?
>
>@@ -637,11 +637,12 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page)
> "badblocks,blocking,blocksize,cache_size,"
> "completion_nsec,discard,home_node,hw_queue_depth,"
> "irqmode,max_sectors,mbps,memory_backed,no_sched,"
>- "copy_max_bytes,"
> "poll_queues,power,queue_mode,shared_tag_bitmap,size,"
> "submit_queues,use_per_node_hctx,virt_boundary,zoned,"
> "zone_capacity,zone_max_active,zone_max_open,"
>- "zone_nr_conv,zone_offline,zone_readonly,zone_size\n");
>+ "zone_nr_conv,zone_offline,zone_readonly,zone_size"
>+ "copy_max_bytes\n");
> }
>
>[...]
>
acked, one doubt I see checkpatch complains
"WARNING: quoted string split across lines".
Is there any graceful way to avoid this warning ?
>+static inline int nullb_setup_copy_read(struct nullb *nullb,
>+ struct bio *bio)
>+{
>+ struct nullb_copy_token *token = bvec_kmap_local(&bio->bi_io_vec[0]);
>+
>+ memcpy(token->subsys, "nullb", 5);
>
>do you really need to use memcpy here ? can token->subsys be a pointer
>and use with assignment token->subsys = nullb ?
>
We do have token->nullb, which stores this device.
Idea behind token->subsys is to differentiate between different types of
copies. Like copy between namespace, across namespace etc.
>+ token->sector_in = bio->bi_iter.bi_sector;
>+ token->nullb = nullb;
>+ token->sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
>+
>+ return 0;
>+}
>+
>
>no point in return 1 , use local bool for fua instead of repeating
>expression and no need to fold line for nullb_setup_copy_read()
>makes is easy to read and removes extra lines and indentation see below :-
>
acked.
>-static inline int nullb_setup_copy_read(struct nullb *nullb,
>- struct bio *bio)
>+static inline void nullb_setup_copy_read(struct nullb *nullb, struct bio *bio)
> {
> struct nullb_copy_token *token = bvec_kmap_local(&bio->bi_io_vec[0]);
>
>- memcpy(token->subsys, "nullb", 5);
>+ token->subsys = "nullb;
if you meant, token->subsys = "nullb", yeah we can add this in next
version.
> token->sector_in = bio->bi_iter.bi_sector;
> token->nullb = nullb;
> token->sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
>-
>- return 0;
> }
>
> static inline int nullb_setup_copy_write(struct nullb *nullb,
>@@ -1334,20 +1331,21 @@ static int null_handle_rq(struct nullb_cmd *cmd)
> sector_t sector = blk_rq_pos(rq);
> struct req_iterator iter;
> struct bio_vec bvec;
>+ bool fua = rq->cmd_flags & REQ_FUA;
>
> if (rq->cmd_flags & REQ_COPY) {
> if (op_is_write(req_op(rq)))
>- return nullb_setup_copy_write(nullb, rq->bio,
>- rq->cmd_flags & REQ_FUA);
>- return nullb_setup_copy_read(nullb, rq->bio);
>+ return nullb_setup_copy_write(nullb, rq->bio, fua);
>+
>+ nullb_setup_copy_read(nullb, rq->bio);
>+ return 0;
> }
>
> spin_lock_irq(&nullb->lock);
> rq_for_each_segment(bvec, rq, iter) {
> len = bvec.bv_len;
> err = null_transfer(nullb, bvec.bv_page, len, bvec.bv_offset,
>- op_is_write(req_op(rq)), sector,
>- rq->cmd_flags & REQ_FUA);
>+ op_is_write(req_op(rq)), sector, fua);
> if (err) {
> spin_unlock_irq(&nullb->lock);
> return err;
>@@ -1368,12 +1366,13 @@ static int null_handle_bio(struct nullb_cmd *cmd)
> sector_t sector = bio->bi_iter.bi_sector;
> struct bio_vec bvec;
> struct bvec_iter iter;
>+ bool fua = bio->bi_opf & REQ_FUA
>
> if (bio->bi_opf & REQ_COPY) {
> if (op_is_write(bio_op(bio)))
>- return nullb_setup_copy_write(nullb, bio,
>- bio->bi_opf & REQ_FUA);
>- return nullb_setup_copy_read(nullb, bio);
>+ return nullb_setup_copy_write(nullb, bio, fua);
>+ nullb_setup_copy_read(nullb, bio);
>+ return 0;
> }
>
>
>
>
>[...]
>
acked
>+struct nullb_copy_token {
>+ char subsys[5];
>+ struct nullb *nullb;
>+ u64 sector_in;
>+ u64 sectors;
>+};
>+
>
>why not use sector_t ?
>
>diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h
>index c67c098d92fa..ffa4b6a6d19b 100644
>--- a/drivers/block/null_blk/null_blk.h
>+++ b/drivers/block/null_blk/null_blk.h
>@@ -70,8 +70,8 @@ enum {
> struct nullb_copy_token {
> char subsys[5];
> struct nullb *nullb;
>- u64 sector_in;
>- u64 sectors;
>+ sector_t sector_in;
>+ sector_t sectors;
> };
>
>
acked
-- Thank you,
-- Nitesh Shetty
Powered by blists - more mailing lists