Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] nvmet: add set feature based timestamp support
@ 2020-02-18 21:43 Chaitanya Kulkarni
  2020-02-18 21:43 ` [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently Chaitanya Kulkarni
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-18 21:43 UTC (permalink / raw)
  To: linux-nvme; +Cc: kbusch, hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently
  2020-02-18 21:43 [PATCH 0/4] nvmet: add set feature based timestamp support Chaitanya Kulkarni
@ 2020-02-18 21:43 ` Chaitanya Kulkarni
  2020-02-19 14:59   ` Christoph Hellwig
  2020-02-18 21:43 ` [PATCH 2/4] nvmet: add support for set-feat-timestamp cmd Chaitanya Kulkarni
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-18 21:43 UTC (permalink / raw)
  To: linux-nvme; +Cc: kbusch, hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/4] nvmet: add support for set-feat-timestamp cmd
  2020-02-18 21:43 [PATCH 0/4] nvmet: add set feature based timestamp support Chaitanya Kulkarni
  2020-02-18 21:43 ` [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently Chaitanya Kulkarni
@ 2020-02-18 21:43 ` Chaitanya Kulkarni
  2020-02-18 21:43 ` [PATCH 3/4] nvmet: add support for get-feat-timestamp cmd Chaitanya Kulkarni
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-18 21:43 UTC (permalink / raw)
  To: linux-nvme; +Cc: kbusch, hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/4] nvmet: add support for get-feat-timestamp cmd
  2020-02-18 21:43 [PATCH 0/4] nvmet: add set feature based timestamp support Chaitanya Kulkarni
  2020-02-18 21:43 ` [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently Chaitanya Kulkarni
  2020-02-18 21:43 ` [PATCH 2/4] nvmet: add support for set-feat-timestamp cmd Chaitanya Kulkarni
@ 2020-02-18 21:43 ` Chaitanya Kulkarni
  2020-02-18 21:43 ` [PATCH 4/4] nvmet: update ctrl oncs values for timestamp Chaitanya Kulkarni
  2020-02-18 23:10 ` [PATCH 0/4] nvmet: add set feature based timestamp support Keith Busch
  4 siblings, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-18 21:43 UTC (permalink / raw)
  To: linux-nvme; +Cc: kbusch, hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 4/4] nvmet: update ctrl oncs values for timestamp
  2020-02-18 21:43 [PATCH 0/4] nvmet: add set feature based timestamp support Chaitanya Kulkarni
                   ` (2 preceding siblings ...)
  2020-02-18 21:43 ` [PATCH 3/4] nvmet: add support for get-feat-timestamp cmd Chaitanya Kulkarni
@ 2020-02-18 21:43 ` Chaitanya Kulkarni
  2020-02-18 23:10 ` [PATCH 0/4] nvmet: add set feature based timestamp support Keith Busch
  4 siblings, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-18 21:43 UTC (permalink / raw)
  To: linux-nvme; +Cc: kbusch, hch, Chaitanya Kulkarni, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] nvmet: add set feature based timestamp support
  2020-02-18 21:43 [PATCH 0/4] nvmet: add set feature based timestamp support Chaitanya Kulkarni
                   ` (3 preceding siblings ...)
  2020-02-18 21:43 ` [PATCH 4/4] nvmet: update ctrl oncs values for timestamp Chaitanya Kulkarni
@ 2020-02-18 23:10 ` Keith Busch
  2020-02-19 14:59   ` Christoph Hellwig
  4 siblings, 1 reply; 11+ messages in thread
From: Keith Busch @ 2020-02-18 23:10 UTC (permalink / raw)
  To: Chaitanya Kulkarni; +Cc: hch, linux-nvme, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] nvmet: add set feature based timestamp support
  2020-02-18 23:10 ` [PATCH 0/4] nvmet: add set feature based timestamp support Keith Busch
@ 2020-02-19 14:59   ` Christoph Hellwig
  2020-02-19 15:49     ` Chaitanya Kulkarni
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2020-02-19 14:59 UTC (permalink / raw)
  To: Keith Busch; +Cc: sagi, linux-nvme, Chaitanya Kulkarni, hch

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently
  2020-02-18 21:43 ` [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently Chaitanya Kulkarni
@ 2020-02-19 14:59   ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2020-02-19 14:59 UTC (permalink / raw)
  To: Chaitanya Kulkarni; +Cc: kbusch, hch, linux-nvme, sagi

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] nvmet: add set feature based timestamp support
  2020-02-19 14:59   ` Christoph Hellwig
@ 2020-02-19 15:49     ` Chaitanya Kulkarni
  2020-02-20  7:48       ` Sagi Grimberg
  0 siblings, 1 reply; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-19 15:49 UTC (permalink / raw)
  To: Christoph Hellwig, Keith Busch; +Cc: sagi, linux-nvme

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] nvmet: add set feature based timestamp support
  2020-02-19 15:49     ` Chaitanya Kulkarni
@ 2020-02-20  7:48       ` Sagi Grimberg
  2020-02-20 16:36         ` Chaitanya Kulkarni
  0 siblings, 1 reply; 11+ messages in thread
From: Sagi Grimberg @ 2020-02-20  7:48 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Christoph Hellwig, Keith Busch; +Cc: linux-nvme


>>> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] nvmet: add set feature based timestamp support
  2020-02-20  7:48       ` Sagi Grimberg
@ 2020-02-20 16:36         ` Chaitanya Kulkarni
  0 siblings, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2020-02-20 16:36 UTC (permalink / raw)
  To: Sagi Grimberg, Christoph Hellwig, Keith Busch; +Cc: linux-nvme

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 21:43 [PATCH 0/4] nvmet: add set feature based timestamp support Chaitanya Kulkarni
2020-02-18 21:43 ` [PATCH 1/4] nvmet: use nvmet_feat_data_len consistently Chaitanya Kulkarni
2020-02-19 14:59   ` Christoph Hellwig
2020-02-18 21:43 ` [PATCH 2/4] nvmet: add support for set-feat-timestamp cmd Chaitanya Kulkarni
2020-02-18 21:43 ` [PATCH 3/4] nvmet: add support for get-feat-timestamp cmd Chaitanya Kulkarni
2020-02-18 21:43 ` [PATCH 4/4] nvmet: update ctrl oncs values for timestamp Chaitanya Kulkarni
2020-02-18 23:10 ` [PATCH 0/4] nvmet: add set feature based timestamp support Keith Busch
2020-02-19 14:59   ` Christoph Hellwig
2020-02-19 15:49     ` Chaitanya Kulkarni
2020-02-20  7:48       ` Sagi Grimberg
2020-02-20 16:36         ` Chaitanya Kulkarni

Linux-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git