Messages in this thread | | | Date | Thu, 13 Apr 2023 16:06:10 +0530 | From | Nitesh Shetty <> | 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@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
| |