Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 13 Apr 2023 16:06:10 +0530
From:   Nitesh Shetty <nj.shetty@samsung.com>
To:     Chaitanya Kulkarni <chaitanyak@nvidia.com>
Cc:     Anuj Gupta <anuj20.g@samsung.com>, Jens Axboe <axboe@kernel.dk>,
        Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@kernel.org>,
        "dm-devel@redhat.com" <dm-devel@redhat.com>,
        Keith Busch <kbusch@kernel.org>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        James Smart <james.smart@broadcom.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        "hare@suse.de" <hare@suse.de>,
        "ming.lei@redhat.com" <ming.lei@redhat.com>,
        "dlemoal@kernel.org" <dlemoal@kernel.org>,
        "joshi.k@samsung.com" <joshi.k@samsung.com>,
        "nitheshshetty@gmail.com" <nitheshshetty@gmail.com>,
        "gost.dev@samsung.com" <gost.dev@samsung.com>,
        Damien Le Moal <damien.lemoal@opensource.wdc.com>,
        Vincent Fu <vincent.fu@samsung.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v9 9/9] null_blk: add support for copy offload
Message-ID: <20230413103610.3calcy37ogf72y6q@green5>
MIME-Version: 1.0
In-Reply-To: <b8eb491b-ecb6-c559-1340-9984897f2aa4@nvidia.com>
User-Agent: NeoMutt/20171215
Content-Type: multipart/mixed;
        boundary="----PdhGkLDwNmEstpipWwkisvJIpjwP47YWkcBE-d9KzOb1cur7=_14869_"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <20230411081041.5328-1-anuj20.g@samsung.com>
        <CGME20230411081400epcas5p151186138b36daf361520b08618300502@epcas5p1.samsung.com>
        <20230411081041.5328-10-anuj20.g@samsung.com>
        <b8eb491b-ecb6-c559-1340-9984897f2aa4@nvidia.com>
Precedence: bulk

------PdhGkLDwNmEstpipWwkisvJIpjwP47YWkcBE-d9KzOb1cur7=_14869_
Content-Type: text/plain; charset="utf-8"; format="flowed"
Content-Disposition: inline

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

------PdhGkLDwNmEstpipWwkisvJIpjwP47YWkcBE-d9KzOb1cur7=_14869_
Content-Type: text/plain; charset="utf-8"


------PdhGkLDwNmEstpipWwkisvJIpjwP47YWkcBE-d9KzOb1cur7=_14869_--