[PATCH 0/4] nvmet: add set feature based timestamp support

[PATCH 0/4] nvmet: add set feature based timestamp support

[Chaitanya Kulkarni]

This is a small patch-series which implements set-feature based
controller timestamp feature support for NVMeOF Target controller.
This allows host to set the timestamp using set feature and read it
with get feature commands when controllers are distributed across
 different systems. We return the value set with a Set Features
command for the current value plus the elapsed time since being set.
NVMe [1].

Regards,
Chaitanya

[1] NVMExpress Revision 1.4 (Page 220) Set Timestamp Origin -> 001b.

Chaitanya Kulkarni (4):
  nvmet: use nvmet_feat_data_len consistently
  nvmet: add support for set-feat-timestamp cmd
  nvmet: add support for get-feat-timestamp cmd
  nvmet: update ctrl oncs values for timestamp

 drivers/nvme/target/admin-cmd.c | 47 +++++++++++++++++++++++++++++++--
 drivers/nvme/target/nvmet.h     |  3 +++
 2 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.22.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/4] nvmet: use nvmet_feat_data_len consistently

[Chaitanya Kulkarni]

This is a preparation patch which makes admin-cmd.c to use newly added
nvmet_feat_data_len() helper consistently in nvmet_execute_set_features
and nvmet_execute_get_features when checking the data length.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 19f949570625..f6374cd5e938 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -735,7 +735,7 @@ static void nvmet_execute_set_features(struct nvmet_req *req)
 	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 status = 0;
 
-	if (!nvmet_check_data_len(req, 0))
+	if (!nvmet_check_data_len(req, nvmet_feat_data_len(req, cdw10)))
 		return;
 
 	switch (cdw10 & 0xff) {
-- 
2.22.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently

[Christoph Hellwig]

On Tue, Feb 18, 2020 at 01:43:35PM -0800, Chaitanya Kulkarni wrote:
> This is a preparation patch which makes admin-cmd.c to use newly added
> nvmet_feat_data_len() helper consistently in nvmet_execute_set_features
> and nvmet_execute_get_features when checking the data length.
> 
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 2/4] nvmet: add support for set-feat-timestamp cmd

[Chaitanya Kulkarni]

This patch allows host to use set-feature command to set the timestamp
value in the target controller.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 20 ++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f6374cd5e938..ebb595480422 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -29,6 +29,8 @@ static u32 nvmet_feat_data_len(struct nvmet_req *req, u32 cdw10)
 	switch (cdw10 & 0xff) {
 	case NVME_FEAT_HOST_ID:
 		return sizeof(req->sq->ctrl->hostid);
+	case NVME_FEAT_TIMESTAMP:
+		return sizeof(req->sq->ctrl->ts);
 	default:
 		return 0;
 	}
@@ -729,6 +731,21 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 	return 0;
 }
 
+static u16 nvmet_set_feat_ts(struct nvmet_req *req)
+{
+	u16 status;
+	__le64 ts;
+
+	status = nvmet_copy_from_sgl(req, 0, &ts, sizeof(ts));
+	if (status)
+		goto out;
+
+	req->sq->ctrl->ts = le64_to_cpu(ts);
+	req->sq->ctrl->local_ts = ktime_to_ms(ktime_get_real());
+out:
+	return status;
+}
+
 static void nvmet_execute_set_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
@@ -755,6 +772,9 @@ static void nvmet_execute_set_features(struct nvmet_req *req)
 	case NVME_FEAT_WRITE_PROTECT:
 		status = nvmet_set_feat_write_protect(req);
 		break;
+	case NVME_FEAT_TIMESTAMP:
+		status = nvmet_set_feat_ts(req);
+		break;
 	default:
 		req->error_loc = offsetof(struct nvme_common_command, cdw10);
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 42ba2ddd9e96..a3ecc269ebb9 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -173,6 +173,9 @@ struct nvmet_ctrl {
 	u16			cntlid;
 	u32			kato;
 
+	u64			ts;
+	u64			local_ts;
+
 	struct nvmet_port	*port;
 
 	u32			aen_enabled;
-- 
2.22.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 3/4] nvmet: add support for get-feat-timestamp cmd

[Chaitanya Kulkarni]

This patch allows host to use get-features command to read the timestamp
value from target controller which is previously set.

We return the value set with a Set Features command for the current
value plus the elapsed time since being set. With that, we also set the
timestamp origin value to 001b (NVMExpress Revision 1.4 (Page 220)).

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ebb595480422..17411d70d4d4 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -815,6 +815,25 @@ void nvmet_get_feat_async_event(struct nvmet_req *req)
 	nvmet_set_result(req, READ_ONCE(req->sq->ctrl->aen_enabled));
 }
 
+static u16 nvmet_get_feat_ts(struct nvmet_req *req)
+{
+	__le64 timestamp;
+	u16 status;
+	u64 res;
+
+	res = req->sq->ctrl->ts +
+		(ktime_to_ms(ktime_get_real()) - req->sq->ctrl->local_ts);
+
+	/* NVMExpress Revision 1.4 (Page 220) Set Timestamp Origin -> 001b. */
+	res = (res & 0x0000FFFFFFFFFFFF) | (1ULL << 49);
+	timestamp = cpu_to_le64(res);
+	status = nvmet_copy_to_sgl(req, 0, &timestamp, sizeof(timestamp));
+	if (status)
+		goto out;
+out:
+	return status;
+}
+
 static void nvmet_execute_get_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
@@ -874,6 +893,9 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 	case NVME_FEAT_WRITE_PROTECT:
 		status = nvmet_get_feat_write_protect(req);
 		break;
+	case NVME_FEAT_TIMESTAMP:
+		status = nvmet_get_feat_ts(req);
+		break;
 	default:
 		req->error_loc =
 			offsetof(struct nvme_common_command, cdw10);
-- 
2.22.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 4/4] nvmet: update ctrl oncs values for timestamp

[Chaitanya Kulkarni]

Now that we support the timestamp feature, update controller ONCS
field so that host driver can set timestamp from
nvme_init_identify() -> nvme_configure_timestamp().

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 17411d70d4d4..39ecfe8c99a0 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -410,7 +410,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->nn = cpu_to_le32(ctrl->subsys->max_nsid);
 	id->mnan = cpu_to_le32(NVMET_MAX_NAMESPACES);
 	id->oncs = cpu_to_le16(NVME_CTRL_ONCS_DSM |
-			NVME_CTRL_ONCS_WRITE_ZEROES);
+			       NVME_CTRL_ONCS_WRITE_ZEROES |
+			       NVME_CTRL_ONCS_TIMESTAMP);
 
 	/* XXX: don't report vwc if the underlying device is write through */
 	id->vwc = NVME_CTRL_VWC_PRESENT;
-- 
2.22.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/4] nvmet: add set feature based timestamp support

[Keith Busch]

On Tue, Feb 18, 2020 at 01:43:34PM -0800, Chaitanya Kulkarni wrote:
> This is a small patch-series which implements set-feature based
> controller timestamp feature support for NVMeOF Target controller.
> This allows host to set the timestamp using set feature and read it
> with get feature commands when controllers are distributed across
>  different systems. We return the value set with a Set Features
> command for the current value plus the elapsed time since being set.
> NVMe [1].
> 
> Regards,
> Chaitanya
> 
> [1] NVMExpress Revision 1.4 (Page 220) Set Timestamp Origin -> 001b.
> 
> Chaitanya Kulkarni (4):
>   nvmet: use nvmet_feat_data_len consistently
>   nvmet: add support for set-feat-timestamp cmd
>   nvmet: add support for get-feat-timestamp cmd
>   nvmet: update ctrl oncs values for timestamp

Patch 1 looks good.

The last three ought to be a single patch IMO since those should really
from an inseparable package deal for this feature.

My only comment on the implementation is you need to initialize nvmet_ctrl
'ts' and 'local_ts' on each reset so that we have correct values to
report in case the host never sets the timestamp.

As to why we'd support this feature, the spec says "The use of the
Timestamp is outside the scope of this specification". Is there anything
interesting we can do with this?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/4] nvmet: add set feature based timestamp support

[Christoph Hellwig]

On Wed, Feb 19, 2020 at 08:10:07AM +0900, Keith Busch wrote:
> The last three ought to be a single patch IMO since those should really
> from an inseparable package deal for this feature.

Agreed.

> As to why we'd support this feature, the spec says "The use of the
> Timestamp is outside the scope of this specification". Is there anything
> interesting we can do with this?

I can't really think of why this is useful either..

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/4] nvmet: add set feature based timestamp support

[Chaitanya Kulkarni]

On 02/19/2020 06:59 AM, Christoph Hellwig wrote:
> On Wed, Feb 19, 2020 at 08:10:07AM +0900, Keith Busch wrote:
>> The last three ought to be a single patch IMO since those should really
>> from an inseparable package deal for this feature.
>
> Agreed.
>
Agree, just made series into small chunks so that it will be easier
for a reviewer to go through changes in a botttom up manner.
>> As to why we'd support this feature, the spec says "The use of the
>> Timestamp is outside the scope of this specification". Is there anything
>> interesting we can do with this?
>
> I can't really think of why this is useful either..
>
If a host is dealing with different controllers which are on a different
machines it will be easier to sync timestamp and read it's value.

Although spec doesn't specify the right use, having this feature will
allow application(s) (if any) to use it if needed.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/4] nvmet: add set feature based timestamp support

[Sagi Grimberg]

>>> The last three ought to be a single patch IMO since those should really
>>> from an inseparable package deal for this feature.
>>
>> Agreed.
>>
> Agree, just made series into small chunks so that it will be easier
> for a reviewer to go through changes in a botttom up manner.
>>> As to why we'd support this feature, the spec says "The use of the
>>> Timestamp is outside the scope of this specification". Is there anything
>>> interesting we can do with this?
>>
>> I can't really think of why this is useful either..
>>
> If a host is dealing with different controllers which are on a different
> machines it will be easier to sync timestamp and read it's value.
> 
> Although spec doesn't specify the right use, having this feature will
> allow application(s) (if any) to use it if needed.

Do we know why is that needed?

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 0/4] nvmet: add set feature based timestamp support

[Chaitanya Kulkarni]

On 02/19/2020 11:48 PM, Sagi Grimberg wrote:
>
>>>> The last three ought to be a single patch IMO since those should really
>>>> from an inseparable package deal for this feature.
>>>
>>> Agreed.
>>>
>> Agree, just made series into small chunks so that it will be easier
>> for a reviewer to go through changes in a botttom up manner.
>>>> As to why we'd support this feature, the spec says "The use of the
>>>> Timestamp is outside the scope of this specification". Is there anything
>>>> interesting we can do with this?
>>>
>>> I can't really think of why this is useful either..
>>>
>> If a host is dealing with different controllers which are on a different
>> machines it will be easier to sync timestamp and read it's value.
>>
>> Although spec doesn't specify the right use, having this feature will
>> allow application(s) (if any) to use it if needed.
>
> Do we know why is that needed?
>

As of now I don't have an application which needs this ASAP.

When I was reading the host/core.c I found that we do set the
timestamp as a part of initializing the controller so it is nice to
have feature with a small code change which makes target controller
spec compliant.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme