[PATCH] nvmet: fix data units read and written counters in SMART log

[PATCH] nvmet: fix data units read and written counters in SMART log

[Tom Wu]

In nvme spec 1.3 there is a definition for data write/read counters
from SMART log. (See section 5.14.1.2):
	This value is reported in thousands (i.e., a value of 1
	corresponds to 1000 units of 512 bytes read) and is rounded up.

However, in nvme target where value is reported with actual units,
but not thousands of units as the spec requires.

Signed-off-by: Tom Wu <tomwu at mellanox.com>
Reviewed-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/admin-cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 9f72d51..acbadbe 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -85,6 +85,9 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 	host_writes = part_stat_read(ns->bdev->bd_part, ios[WRITE]);
 	data_units_written = part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
 
+	data_units_read = DIV_ROUND_UP(data_units_read, 1000);
+	data_units_written = DIV_ROUND_UP(data_units_written, 1000);
+
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
 	put_unaligned_le64(host_writes, &slog->host_writes[0]);
@@ -120,6 +123,9 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 	}
 	rcu_read_unlock();
 
+	data_units_read = DIV_ROUND_UP(data_units_read, 1000);
+	data_units_written = DIV_ROUND_UP(data_units_written, 1000);
+
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
 	put_unaligned_le64(host_writes, &slog->host_writes[0]);
-- 
1.8.4.3

[PATCH] nvmet: fix data units read and written counters in SMART log

[Minwoo Im]

> -----Original Message-----
> From: Linux-nvme <linux-nvme-bounces at lists.infradead.org> On Behalf Of Tom Wu
> Sent: Monday, August 5, 2019 10:58 AM
> To: Linux-nvme <linux-nvme at lists.infradead.org>; keith.busch at intel.com;
> hch at lst.de; sagi at grimberg.me
> Cc: Shlomi Nimrodi <shlomin at mellanox.com>; Max Gurtovoy
> <maxg at mellanox.com>; Tom Wu <tomwu at mellanox.com>; Israel Rukshin
> <israelr at mellanox.com>
> Subject: [PATCH] nvmet: fix data units read and written counters in SMART log
> 
> In nvme spec 1.3 there is a definition for data write/read counters
> from SMART log. (See section 5.14.1.2):
> 	This value is reported in thousands (i.e., a value of 1
> 	corresponds to 1000 units of 512 bytes read) and is rounded up.
> 
> However, in nvme target where value is reported with actual units,
> but not thousands of units as the spec requires.
> 
> Signed-off-by: Tom Wu <tomwu at mellanox.com>
> Reviewed-by: Israel Rukshin <israelr at mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

Reviewed-by: Minwoo Im <minwoo.im at samsung.com>

[PATCH] nvmet: fix data units read and written counters in SMART log

[Sagi Grimberg]

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 9f72d51..acbadbe 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -85,6 +85,9 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
>   	host_writes = part_stat_read(ns->bdev->bd_part, ios[WRITE]);
>   	data_units_written = part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
>   
> +	data_units_read = DIV_ROUND_UP(data_units_read, 1000);
> +	data_units_written = DIV_ROUND_UP(data_units_written, 1000);
> +

How about either modifying the actual retrieval or move the fixups right 
next to the initial setting...

[PATCH] nvmet: fix data units read and written counters in SMART log

[Tom Wu]

Hi, Sagi

when you say I can 'modifying the actual retrieval', you are try to 
suggest that I can do the changes as below, right?

--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -81,9 +81,11 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req 
*req,
 ??????????????? goto out;

 ??????? host_reads = part_stat_read(ns->bdev->bd_part, ios[READ]);
-?????? data_units_read = part_stat_read(ns->bdev->bd_part, sectors[READ]);
+?????? data_units_read = DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part,
+?????????????? sectors[READ]), 1000);
 ??????? host_writes = part_stat_read(ns->bdev->bd_part, ios[WRITE]);
-?????? data_units_written = part_stat_read(ns->bdev->bd_part, 
sectors[WRITE]);
+?????? data_units_written = DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part,
+?????????????? sectors[WRITE]), 1000);

 ??????? put_unaligned_le64(host_reads, &slog->host_reads[0]);
 ??????? put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
@@ -112,10 +114,10 @@ static u16 nvmet_get_smart_log_all(struct 
nvmet_req *req,
 ??????????????????????? continue;
 ??????????????? host_reads += part_stat_read(ns->bdev->bd_part, ios[READ]);
 ??????????????? data_units_read +=
-?????????????????????? part_stat_read(ns->bdev->bd_part, sectors[READ]);
+ DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part, sectors[READ]), 1000);
 ??????????????? host_writes += part_stat_read(ns->bdev->bd_part, 
ios[WRITE]);
 ??????????????? data_units_written +=
-?????????????????????? part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
+ DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part, sectors[WRITE]), 1000);

 ??????? }

On 8/6/2019 2:24 AM, Sagi Grimberg wrote:
>
>> diff --git a/drivers/nvme/target/admin-cmd.c 
>> b/drivers/nvme/target/admin-cmd.c
>> index 9f72d51..acbadbe 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -85,6 +85,9 @@ static u16 nvmet_get_smart_log_nsid(struct 
>> nvmet_req *req,
>> ????? host_writes = part_stat_read(ns->bdev->bd_part, ios[WRITE]);
>> ????? data_units_written = part_stat_read(ns->bdev->bd_part, 
>> sectors[WRITE]);
>> ? +??? data_units_read = DIV_ROUND_UP(data_units_read, 1000);
>> +??? data_units_written = DIV_ROUND_UP(data_units_written, 1000);
>> +
>
> How about either modifying the actual retrieval or move the fixups 
> right next to the initial setting...

[PATCH] nvmet: fix data units read and written counters in SMART log

[Sagi Grimberg]

> Hi, Sagi
> 
> when you say I can 'modifying the actual retrieval', you are try to
> suggest that I can do the changes as below, right?

Yes.

[PATCH] nvmet: fix data units read and written counters in SMART log

[Sagi Grimberg]

Thanks, pulled to nvme-5.4

[PATCH] nvmet: fix data units read and written counters in SMART log

[Chaitanya Kulkarni]

Looks good to me.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
On 8/7/19 7:23 PM, Tom Wu wrote:
> In nvme spec 1.3 there is a definition for data write/read counters
> from SMART log, (See section 5.14.1.2):
> 	This value is reported in thousands (i.e., a value of 1
> 	corresponds to 1000 units of 512 bytes read) and is rounded up.
>
> However, in nvme target where value is reported with actual units,
> but not thousands of units as the spec requires.
>
> Signed-off-by: Tom Wu <tomwu at mellanox.com>
> Reviewed-by: Israel Rukshin <israelr at mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> ---
>  drivers/nvme/target/admin-cmd.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 9f72d51..4099093 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -81,9 +81,11 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
>  		goto out;
>  
>  	host_reads = part_stat_read(ns->bdev->bd_part, ios[READ]);
> -	data_units_read = part_stat_read(ns->bdev->bd_part, sectors[READ]);
> +	data_units_read = DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part,
> +		sectors[READ]), 1000);
>  	host_writes = part_stat_read(ns->bdev->bd_part, ios[WRITE]);
> -	data_units_written = part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
> +	data_units_written = DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part,
> +		sectors[WRITE]), 1000);
>  
>  	put_unaligned_le64(host_reads, &slog->host_reads[0]);
>  	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
> @@ -111,11 +113,11 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
>  		if (!ns->bdev)
>  			continue;
>  		host_reads += part_stat_read(ns->bdev->bd_part, ios[READ]);
> -		data_units_read +=
> -			part_stat_read(ns->bdev->bd_part, sectors[READ]);
> +		data_units_read += DIV_ROUND_UP(
> +			part_stat_read(ns->bdev->bd_part, sectors[READ]), 1000);
>  		host_writes += part_stat_read(ns->bdev->bd_part, ios[WRITE]);
> -		data_units_written +=
> -			part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
> +		data_units_written += DIV_ROUND_UP(
> +			part_stat_read(ns->bdev->bd_part, sectors[WRITE]), 1000);
>  
>  	}
>  	rcu_read_unlock();

[PATCH] nvmet: fix data units read and written counters in SMART log

[Tom Wu]

In nvme spec 1.3 there is a definition for data write/read counters
from SMART log, (See section 5.14.1.2):
	This value is reported in thousands (i.e., a value of 1
	corresponds to 1000 units of 512 bytes read) and is rounded up.

However, in nvme target where value is reported with actual units,
but not thousands of units as the spec requires.

Signed-off-by: Tom Wu <tomwu at mellanox.com>
Reviewed-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/target/admin-cmd.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 9f72d51..4099093 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -81,9 +81,11 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		goto out;
 
 	host_reads = part_stat_read(ns->bdev->bd_part, ios[READ]);
-	data_units_read = part_stat_read(ns->bdev->bd_part, sectors[READ]);
+	data_units_read = DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part,
+		sectors[READ]), 1000);
 	host_writes = part_stat_read(ns->bdev->bd_part, ios[WRITE]);
-	data_units_written = part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
+	data_units_written = DIV_ROUND_UP(part_stat_read(ns->bdev->bd_part,
+		sectors[WRITE]), 1000);
 
 	put_unaligned_le64(host_reads, &slog->host_reads[0]);
 	put_unaligned_le64(data_units_read, &slog->data_units_read[0]);
@@ -111,11 +113,11 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,
 		if (!ns->bdev)
 			continue;
 		host_reads += part_stat_read(ns->bdev->bd_part, ios[READ]);
-		data_units_read +=
-			part_stat_read(ns->bdev->bd_part, sectors[READ]);
+		data_units_read += DIV_ROUND_UP(
+			part_stat_read(ns->bdev->bd_part, sectors[READ]), 1000);
 		host_writes += part_stat_read(ns->bdev->bd_part, ios[WRITE]);
-		data_units_written +=
-			part_stat_read(ns->bdev->bd_part, sectors[WRITE]);
+		data_units_written += DIV_ROUND_UP(
+			part_stat_read(ns->bdev->bd_part, sectors[WRITE]), 1000);
 
 	}
 	rcu_read_unlock();
-- 
1.8.4.3