Re: [PATCH v9 9/9] null_blk: add support for copy offload

[Nitesh Shetty <nj.shetty@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 7120 bytes --]

On 23/04/13 06:28AM, Chaitanya Kulkarni wrote:
>On 4/11/23 01:10, Anuj Gupta wrote:
>> From: Nitesh Shetty <nj.shetty@samsung.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@opensource.wdc.com>
>> Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
>> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
>> Signed-off-by: Vincent Fu <vincent.fu@samsung.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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]