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@xxxxxxx>
> ---
> 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@xxxxxxx>
--
Damien Le Moal
Western Digital Research
