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

[Damien Le Moal <Damien.LeMoal@wdc.com>]

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>

-- 
Damien Le Moal
Western Digital Research