diff mbox series

[V5,2/6] nvmet: add lba to sect coversion helpers

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

Commit Message

Chaitanya Kulkarni Dec. 10, 2020, 6:26 a.m. UTC 
In this preparation patch we add helpers to convert lbas to sectors and
sectors to lba. This is needed to eliminate code duplication in the ZBD
backend.

Use these helpers in the block device backennd.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/io-cmd-bdev.c |  8 +++-----
 drivers/nvme/target/nvmet.h       | 10 ++++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

Comments

Damien Le Moal Dec. 11, 2020, 12:50 a.m. UTC | #1 
On 2020/12/10 15:27, Chaitanya Kulkarni wrote:
> In this preparation patch we add helpers to convert lbas to sectors and
> sectors to lba. This is needed to eliminate code duplication in the ZBD
> backend.
> 
> Use these helpers in the block device backennd.

s/backennd/backend

And in the title:

s/coversion/conversion

scripts/checkpatch.pl does spell checking...

> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
>  drivers/nvme/target/io-cmd-bdev.c |  8 +++-----
>  drivers/nvme/target/nvmet.h       | 10 ++++++++++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
> index 125dde3f410e..23095bdfce06 100644
> --- a/drivers/nvme/target/io-cmd-bdev.c
> +++ b/drivers/nvme/target/io-cmd-bdev.c
> @@ -256,8 +256,7 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>  	if (is_pci_p2pdma_page(sg_page(req->sg)))
>  		op |= REQ_NOMERGE;
>  
> -	sector = le64_to_cpu(req->cmd->rw.slba);
> -	sector <<= (req->ns->blksize_shift - 9);
> +	sector = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
>  
>  	if (req->transfer_len <= NVMET_MAX_INLINE_DATA_LEN) {
>  		bio = &req->b.inline_bio;
> @@ -345,7 +344,7 @@ static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
>  	int ret;
>  
>  	ret = __blkdev_issue_discard(ns->bdev,
> -			le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
> +			nvmet_lba_to_sect(ns, range->slba),
>  			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
>  			GFP_KERNEL, 0, bio);
>  	if (ret && ret != -EOPNOTSUPP) {
> @@ -414,8 +413,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
>  	if (!nvmet_check_transfer_len(req, 0))
>  		return;
>  
> -	sector = le64_to_cpu(write_zeroes->slba) <<
> -		(req->ns->blksize_shift - 9);
> +	sector = nvmet_lba_to_sect(req->ns, write_zeroes->slba);
>  	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
>  		(req->ns->blksize_shift - 9));
>  
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 592763732065..4cb4cdae858c 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -603,4 +603,14 @@ static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
>  	return ns->pi_type && ns->metadata_size == sizeof(struct t10_pi_tuple);
>  }
>  
> +static inline u64 nvmet_sect_to_lba(struct nvmet_ns *ns, sector_t sect)
> +{
> +	return sect >> (ns->blksize_shift - SECTOR_SHIFT);
> +}
> +
> +static inline sector_t nvmet_lba_to_sect(struct nvmet_ns *ns, __le64 lba)
> +{
> +	return le64_to_cpu(lba) << (ns->blksize_shift - SECTOR_SHIFT);
> +}
> +

One thing here that I am not a fan of is the inconsistency between the 2 helpers
regarding the LBA endianess: nvmet_lba_to_sect() takes LE lba value, but
nvmet_sect_to_lba() returns a CPU endian lba value. Can't we unify them to work
on CPU endian values, and then if needed add another helper like:

static inline sector_t nvmet_lelba_to_sect(struct nvmet_ns *ns, __le64 lba)
{
	return nvmet_lba_to_sect(ns, le64_to_cpu(lba));
}


>  #endif /* _NVMET_H */
>
Chaitanya Kulkarni Dec. 12, 2020, 4:36 a.m. UTC | #2 
On 12/10/20 16:50, Damien Le Moal wrote:
> On 2020/12/10 15:27, Chaitanya Kulkarni wrote:
>> In this preparation patch we add helpers to convert lbas to sectors and
>> sectors to lba. This is needed to eliminate code duplication in the ZBD
>> backend.
>>
>> Use these helpers in the block device backennd.
> s/backennd/backend
>
> And in the title:
>
> s/coversion/conversion
>
> scripts/checkpatch.pl does spell checking...
I'll double check, it did not spit any warningand I rely on checkpatch.

zbd-nvmeof/v5/0002-nvmet-add-lba-to-sect-coversion-helpers.patch
----------------------------------------------------------------
total: 0 errors, 0 warnings, 40 lines checked

zbd-nvmeof/v5/0002-nvmet-add-lba-to-sect-coversion-helpers.patch has no
obvious style problems and is ready for submission.

>
>> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
>> ---
>>  drivers/nvme/target/io-cmd-bdev.c |  8 +++-----
>>  drivers/nvme/target/nvmet.h       | 10 ++++++++++
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
>> index 125dde3f410e..23095bdfce06 100644
>> --- a/drivers/nvme/target/io-cmd-bdev.c
>> +++ b/drivers/nvme/target/io-cmd-bdev.c
>> @@ -256,8 +256,7 @@ static void nvmet_bdev_execute_rw(struct nvmet_req *req)
>>  	if (is_pci_p2pdma_page(sg_page(req->sg)))
>>  		op |= REQ_NOMERGE;
>>  
>> -	sector = le64_to_cpu(req->cmd->rw.slba);
>> -	sector <<= (req->ns->blksize_shift - 9);
>> +	sector = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
>>  
>>  	if (req->transfer_len <= NVMET_MAX_INLINE_DATA_LEN) {
>>  		bio = &req->b.inline_bio;
>> @@ -345,7 +344,7 @@ static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
>>  	int ret;
>>  
>>  	ret = __blkdev_issue_discard(ns->bdev,
>> -			le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
>> +			nvmet_lba_to_sect(ns, range->slba),
>>  			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
>>  			GFP_KERNEL, 0, bio);
>>  	if (ret && ret != -EOPNOTSUPP) {
>> @@ -414,8 +413,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
>>  	if (!nvmet_check_transfer_len(req, 0))
>>  		return;
>>  
>> -	sector = le64_to_cpu(write_zeroes->slba) <<
>> -		(req->ns->blksize_shift - 9);
>> +	sector = nvmet_lba_to_sect(req->ns, write_zeroes->slba);
>>  	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
>>  		(req->ns->blksize_shift - 9));
>>  
>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>> index 592763732065..4cb4cdae858c 100644
>> --- a/drivers/nvme/target/nvmet.h
>> +++ b/drivers/nvme/target/nvmet.h
>> @@ -603,4 +603,14 @@ static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
>>  	return ns->pi_type && ns->metadata_size == sizeof(struct t10_pi_tuple);
>>  }
>>  
>> +static inline u64 nvmet_sect_to_lba(struct nvmet_ns *ns, sector_t sect)
>> +{
>> +	return sect >> (ns->blksize_shift - SECTOR_SHIFT);
>> +}
>> +
>> +static inline sector_t nvmet_lba_to_sect(struct nvmet_ns *ns, __le64 lba)
>> +{
>> +	return le64_to_cpu(lba) << (ns->blksize_shift - SECTOR_SHIFT);
>> +}
>> +
> One thing here that I am not a fan of is the inconsistency between the 2 helpers
> regarding the LBA endianess: nvmet_lba_to_sect() takes LE lba value, but
> nvmet_sect_to_lba() returns a CPU endian lba value. Can't we unify them to work
> on CPU endian values, and then if needed add another helper like:
>
> static inline sector_t nvmet_lelba_to_sect(struct nvmet_ns *ns, __le64 lba)
> {
> 	return nvmet_lba_to_sect(ns, le64_to_cpu(lba));
> }
>
I'll add the endian version of the nvmet_sect_to_lba() which matches the
nvmet_lba_to_sect() that is anyways needed to get rid of the
cpu_to_le64() calls
in the zns patch.
>>  #endif /* _NVMET_H */
>>
>
diff mbox series

Patch

diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index 125dde3f410e..23095bdfce06 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -256,8 +256,7 @@  static void nvmet_bdev_execute_rw(struct nvmet_req *req)
 	if (is_pci_p2pdma_page(sg_page(req->sg)))
 		op |= REQ_NOMERGE;
 
-	sector = le64_to_cpu(req->cmd->rw.slba);
-	sector <<= (req->ns->blksize_shift - 9);
+	sector = nvmet_lba_to_sect(req->ns, req->cmd->rw.slba);
 
 	if (req->transfer_len <= NVMET_MAX_INLINE_DATA_LEN) {
 		bio = &req->b.inline_bio;
@@ -345,7 +344,7 @@  static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
 	int ret;
 
 	ret = __blkdev_issue_discard(ns->bdev,
-			le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
+			nvmet_lba_to_sect(ns, range->slba),
 			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
 			GFP_KERNEL, 0, bio);
 	if (ret && ret != -EOPNOTSUPP) {
@@ -414,8 +413,7 @@  static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
 	if (!nvmet_check_transfer_len(req, 0))
 		return;
 
-	sector = le64_to_cpu(write_zeroes->slba) <<
-		(req->ns->blksize_shift - 9);
+	sector = nvmet_lba_to_sect(req->ns, write_zeroes->slba);
 	nr_sector = (((sector_t)le16_to_cpu(write_zeroes->length) + 1) <<
 		(req->ns->blksize_shift - 9));
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 592763732065..4cb4cdae858c 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -603,4 +603,14 @@  static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
 	return ns->pi_type && ns->metadata_size == sizeof(struct t10_pi_tuple);
 }
 
+static inline u64 nvmet_sect_to_lba(struct nvmet_ns *ns, sector_t sect)
+{
+	return sect >> (ns->blksize_shift - SECTOR_SHIFT);
+}
+
+static inline sector_t nvmet_lba_to_sect(struct nvmet_ns *ns, __le64 lba)
+{
+	return le64_to_cpu(lba) << (ns->blksize_shift - SECTOR_SHIFT);
+}
+
 #endif /* _NVMET_H */