diff mbox series

[V9,6/9] nvmet: add bio init helper for different backends

Message ID 20210112042623.6316-7-chaitanya.kulkarni@wdc.com (mailing list archive)
State New, archived
Headers show
Series nvmet: add ZBD backend support | expand

Commit Message

Chaitanya Kulkarni Jan. 12, 2021, 4:26 a.m. UTC 
With the addition of the zns backend now we have two different backends
with the same bio initialization code. That leads to having duplicate
code in two backends: generic bdev and generic zns.

Add a helper function to reduce the duplicate code such that helper
function initializes the various bio initialization parameters such as
bio block device, op flags, sector, end io callback, and private member,

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/io-cmd-bdev.c |  6 +-----
 drivers/nvme/target/nvmet.h       | 11 +++++++++++
 drivers/nvme/target/zns.c         |  6 +++---
 3 files changed, 15 insertions(+), 8 deletions(-)

Comments

Damien Le Moal Jan. 12, 2021, 5:40 a.m. UTC | #1 
On 2021/01/12 13:27, Chaitanya Kulkarni wrote:
> With the addition of the zns backend now we have two different backends
> with the same bio initialization code. That leads to having duplicate
> code in two backends: generic bdev and generic zns.
> 
> Add a helper function to reduce the duplicate code such that helper
> function initializes the various bio initialization parameters such as
> bio block device, op flags, sector, end io callback, and private member,
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/target/io-cmd-bdev.c |  6 +-----
>  drivers/nvme/target/nvmet.h       | 11 +++++++++++
>  drivers/nvme/target/zns.c         |  6 +++---
>  3 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index 72746e29cb0d..b1fb0bb1f39f 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -267,11 +267,7 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>  	sector = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
>  
>  	bio = nvmet_req_bio_get(req, NULL);
> -	bio_set_dev(bio, req->ns->bdev);
> -	bio->bi_iter.bi_sector = sector;
> -	bio->bi_private = req;
> -	bio->bi_end_io = nvmet_bio_done;
> -	bio->bi_opf = op;
> +	nvmet_bio_init(bio, req->ns->bdev, op, sector, req, nvmet_bio_done);
>  
>  	blk_start_plug(&plug);
>  	if (req->metadata_len)
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 3fc84f79cce1..1ec9e1b35c67 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -668,4 +668,15 @@ static inline struct bio *nvmet_req_bio_get(struct nvmet_req *req,
>  	return bio;
>  }
>  
> +static inline void nvmet_bio_init(struct bio *bio, struct block_device *bdev,
> +				  unsigned int op, sector_t sect, void *private,
> +				  bio_end_io_t *bi_end_io)
> +{
> +	bio_set_dev(bio, bdev);
> +	bio->bi_opf = op;
> +	bio->bi_iter.bi_sector = sect;
> +	bio->bi_private = private;
> +	bio->bi_end_io = bi_end_io;
> +}
> +
>  #endif /* _NVMET_H */
> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
> index c32e93a3c7e1..92213bed0006 100644
> --- a/drivers/nvme/target/zns.c
> +++ b/drivers/nvme/target/zns.c
> @@ -281,6 +281,7 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
>  {
>  	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
>  	struct request_queue *q = req->ns->bdev->bd_disk->queue;
> +	unsigned int op = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
>  	unsigned int max_sects = queue_max_zone_append_sectors(q);
>  	u16 status = NVME_SC_SUCCESS;
>  	unsigned int total_len = 0;
> @@ -297,9 +298,8 @@ void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
>  	}
>  
>  	bio = nvmet_req_bio_get(req, NULL);
> -	bio_set_dev(bio, req->ns->bdev);
> -	bio->bi_iter.bi_sector = sect;
> -	bio->bi_opf = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
> +	nvmet_bio_init(bio, req->ns->bdev, op, sect, NULL, NULL);

op is used only here I think. So is that variable really necessary ?

> +
>  	if (req->cmd->rw.control & cpu_to_le16(NVME_RW_FUA))
>  		bio->bi_opf |= REQ_FUA;
>  
> 

Apart from the nit above, looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
Chaitanya Kulkarni Jan. 12, 2021, 5:57 a.m. UTC | #2 
On 1/11/21 21:40, Damien Le Moal wrote:
>>  	bio = nvmet_req_bio_get(req, NULL);
>> -	bio_set_dev(bio, req->ns->bdev);
>> -	bio->bi_iter.bi_sector = sect;
>> -	bio->bi_opf = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
>> +	nvmet_bio_init(bio, req->ns->bdev, op, sect, NULL, NULL);
> op is used only here I think. So is that variable really necessary ?
>
This is just my personal preference as without using op we will have to
add a new line to a function call, I like to keep the function call in
one line as much as I can.
Damien Le Moal Jan. 12, 2021, 6:27 a.m. UTC | #3 
On 2021/01/12 14:57, Chaitanya Kulkarni wrote:
> On 1/11/21 21:40, Damien Le Moal wrote:
>>>  	bio = nvmet_req_bio_get(req, NULL);
>>> -	bio_set_dev(bio, req->ns->bdev);
>>> -	bio->bi_iter.bi_sector = sect;
>>> -	bio->bi_opf = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
>>> +	nvmet_bio_init(bio, req->ns->bdev, op, sect, NULL, NULL);
>> op is used only here I think. So is that variable really necessary ?
>>
> This is just my personal preference as without using op we will have to
> add a new line to a function call, I like to keep the function call in
> one line as much as I can.

A new line in the code costs nothing. An unnecessary variable costs stack space...
Christoph Hellwig Jan. 12, 2021, 7:33 a.m. UTC | #4 
> +static inline void nvmet_bio_init(struct bio *bio, struct block_device *bdev,
> +				  unsigned int op, sector_t sect, void *private,
> +				  bio_end_io_t *bi_end_io)
> +{
> +	bio_set_dev(bio, bdev);
> +	bio->bi_opf = op;
> +	bio->bi_iter.bi_sector = sect;
> +	bio->bi_private = private;
> +	bio->bi_end_io = bi_end_io;
> +}

Nothing NVMe specific about this.  The helper also doesn't relaly contain
any logic either.
Chaitanya Kulkarni Jan. 13, 2021, 5:04 a.m. UTC | #5 
On 1/11/21 23:34, Christoph Hellwig wrote:
> Nothing NVMe specific about this.  The helper also doesn't relaly contain
> any logic either.
>
What is the preferable name ? just bio_init_fields() ?
Christoph Hellwig Jan. 18, 2021, 6:33 p.m. UTC | #6 
On Wed, Jan 13, 2021 at 05:04:49AM +0000, Chaitanya Kulkarni wrote:
> On 1/11/21 23:34, Christoph Hellwig wrote:
> > Nothing NVMe specific about this.  The helper also doesn't relaly contain
> > any logic either.
> >
> What is the preferable name ? just bio_init_fields() ?

We'll eventually need to replace bio_alloc with something initializing
the additional fields.  For now I'd just do nothing and skip the helper
for now.
diff mbox series

Patch

diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 72746e29cb0d..b1fb0bb1f39f 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -267,11 +267,7 @@  static void nvmet_bdev_execute_rw(struct nvmet_req *req)
 	sector = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
 
 	bio = nvmet_req_bio_get(req, NULL);
-	bio_set_dev(bio, req->ns->bdev);
-	bio->bi_iter.bi_sector = sector;
-	bio->bi_private = req;
-	bio->bi_end_io = nvmet_bio_done;
-	bio->bi_opf = op;
+	nvmet_bio_init(bio, req->ns->bdev, op, sector, req, nvmet_bio_done);
 
 	blk_start_plug(&plug);
 	if (req->metadata_len)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 3fc84f79cce1..1ec9e1b35c67 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -668,4 +668,15 @@  static inline struct bio *nvmet_req_bio_get(struct nvmet_req *req,
 	return bio;
 }
 
+static inline void nvmet_bio_init(struct bio *bio, struct block_device *bdev,
+				  unsigned int op, sector_t sect, void *private,
+				  bio_end_io_t *bi_end_io)
+{
+	bio_set_dev(bio, bdev);
+	bio->bi_opf = op;
+	bio->bi_iter.bi_sector = sect;
+	bio->bi_private = private;
+	bio->bi_end_io = bi_end_io;
+}
+
 #endif /* _NVMET_H */
diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
index c32e93a3c7e1..92213bed0006 100644
--- a/drivers/nvme/target/zns.c
+++ b/drivers/nvme/target/zns.c
@@ -281,6 +281,7 @@  void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
 {
 	sector_t sect = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
 	struct request_queue *q = req->ns->bdev->bd_disk->queue;
+	unsigned int op = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
 	unsigned int max_sects = queue_max_zone_append_sectors(q);
 	u16 status = NVME_SC_SUCCESS;
 	unsigned int total_len = 0;
@@ -297,9 +298,8 @@  void nvmet_bdev_execute_zone_append(struct nvmet_req *req)
 	}
 
 	bio = nvmet_req_bio_get(req, NULL);
-	bio_set_dev(bio, req->ns->bdev);
-	bio->bi_iter.bi_sector = sect;
-	bio->bi_opf = REQ_OP_ZONE_APPEND | REQ_SYNC | REQ_IDLE;
+	nvmet_bio_init(bio, req->ns->bdev, op, sect, NULL, NULL);
+
 	if (req->cmd->rw.control & cpu_to_le16(NVME_RW_FUA))
 		bio->bi_opf |= REQ_FUA;