Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1656075 > unrolled thread

[PATCH v3 3/8] nvmet: implement namespace identify descriptor list

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/8] nvmet: implement namespace identify descriptor list Johannes Thumshirn <jthumshirn@suse.de> - 2017-06-02 11:50 +0200
    Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor  list Sagi Grimberg <sagi@grimberg.me> - 2017-06-02 15:50 +0200
      Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor  list Johannes Thumshirn <jthumshirn@suse.de> - 2017-06-02 16:00 +0200
      Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor         list Christoph Hellwig <hch@lst.de> - 2017-06-03 07:20 +0200
        Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor  list Sagi Grimberg <sagi@grimberg.me> - 2017-06-03 10:10 +0200
    Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor         list Christoph Hellwig <hch@lst.de> - 2017-06-05 07:40 +0200
      Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor  list Johannes Thumshirn <jthumshirn@suse.de> - 2017-06-06 09:20 +0200
        Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor         list Christoph Hellwig <hch@lst.de> - 2017-06-06 09:20 +0200
          Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor  list Johannes Thumshirn <jthumshirn@suse.de> - 2017-06-06 09:30 +0200

#1656075 — [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

A NVMe Identify NS command with a CNS value of '3' is expecting a list
of Namespace Identification Descriptor structures to be returned to
the host for the namespace requested in the namespace identify
command.

This Namespace Identification Descriptor structure consists of the
type of the namespace identifier, the length of the identifier and the
actual identifier.

Valid types are NGUID and UUID which we have saved in our nvme_ns
structure if they have been configured via configfs. If no value has
been assigened to one of these we return an "invalid opcode" back to
the host to maintain backward compatibiliy with older implementations
without Namespace Identify Descriptor list support.

Also as the Namespace Identify Descriptor list is the only mandatory
feature change between 1.2.1 and 1.3.0 we can bump the advertised
version as well.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/nvme/target/admin-cmd.c | 53 +++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  2 +-
 drivers/nvme/target/nvmet.h     |  1 +
 3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index ff1f97006322..833b5ef935c3 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -367,6 +367,56 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_identify_desclist(struct nvmet_req *req)
+{
+	struct nvmet_ns *ns;
+	struct nvme_ns_identifier_hdr hdr;
+	u16 status = 0;
+	off_t off = 0;
+
+	ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
+	if (!ns) {
+		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
+		goto out;
+	}
+
+	if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
+		memset(&hdr, 0, sizeof(hdr));
+		hdr.nidt = NVME_NIDT_UUID;
+		hdr.nidl = NVME_NIDT_UUID_LEN;
+		status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
+		if (status)
+			goto out_put_ns;
+		off += sizeof(hdr);
+
+		status = nvmet_copy_to_sgl(req, off, &ns->uuid,
+					   sizeof(ns->uuid));
+		if (status)
+			goto out_put_ns;
+		off += sizeof(ns->uuid);
+	}
+	if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
+		memset(&hdr, 0, sizeof(hdr));
+		hdr.nidt = NVME_NIDT_NGUID;
+		hdr.nidl = NVME_NIDT_NGUID_LEN;
+		status = nvmet_copy_to_sgl(req, off, &hdr, sizeof(hdr));
+		if (status)
+			goto out_put_ns;
+		off += sizeof(hdr);
+
+		status = nvmet_copy_to_sgl(req, off, &ns->nguid,
+					   sizeof(ns->nguid));
+		if (status)
+			goto out_put_ns;
+		off += sizeof(ns->nguid);
+	}
+
+out_put_ns:
+	nvmet_put_namespace(ns);
+out:
+	nvmet_req_complete(req, status);
+}
+
 /*
  * A "mimimum viable" abort implementation: the command is mandatory in the
  * spec, but we are not required to do any useful work.  We couldn't really
@@ -515,6 +565,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 		case NVME_ID_CNS_NS_ACTIVE_LIST:
 			req->execute = nvmet_execute_identify_nslist;
 			return 0;
+		case NVME_ID_CNS_NS_DESC_LIST:
+			req->execute = nvmet_execute_identify_desclist;
+			return 0;
 		}
 		break;
 	case nvme_admin_abort_cmd:
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index eb9399ac97cf..d4eeee4f2fab 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -926,7 +926,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 	if (!subsys)
 		return NULL;
 
-	subsys->ver = NVME_VS(1, 2, 1); /* NVMe 1.2.1 */
+	subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */
 
 	switch (type) {
 	case NVME_NQN_NVME:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index cfc5c7fb0ab7..4c6cb5ea1186 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -46,6 +46,7 @@ struct nvmet_ns {
 	u32			blksize_shift;
 	loff_t			size;
 	u8			nguid[16];
+	uuid_be			uuid;
 
 	bool			enabled;
 	struct nvmet_subsys	*subsys;
-- 
2.12.0

[toc] | [next] | [standalone]

#1656215 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

>  	switch (type) {
>  	case NVME_NQN_NVME:
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index cfc5c7fb0ab7..4c6cb5ea1186 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -46,6 +46,7 @@ struct nvmet_ns {
>  	u32			blksize_shift;
>  	loff_t			size;
>  	u8			nguid[16];
> +	uuid_be			uuid;

This should be uuid_le to match the rest of NVMe.

Also, it would be useful to generate a random default with uuid_le_gen()
for a better out-of-the-box experience.

[toc] | [prev] | [next] | [standalone]

#1656224 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

On 06/02/2017 03:46 PM, Sagi Grimberg wrote:
>> +    uuid_be            uuid;
> 
> This should be uuid_le to match the rest of NVMe.

Ahm, are you sure?
$ PAGER= git grep -E uuid_\(l\|b\)e drivers/nvme
drivers/nvme/host/fabrics.c:61:	uuid_be_gen(&host->id);
drivers/nvme/host/fabrics.c:78:	uuid_be_gen(&host->id);
drivers/nvme/host/fabrics.c:398:	memcpy(&data->hostid,
&ctrl->opts->host->id, sizeof(uuid_be));
drivers/nvme/host/fabrics.c:457:	memcpy(&data->hostid,
&ctrl->opts->host->id, sizeof(uuid_be));
drivers/nvme/host/fabrics.h:39:	uuid_be			id;
drivers/nvme/host/fc.c:882:		min_t(size_t, FCNVME_ASSOC_HOSTID_LEN,
sizeof(uuid_be)));
drivers/nvme/target/admin-cmd.c:324:	memcpy(&id->nguid, &ns->nguid,
sizeof(uuid_le));
drivers/nvme/target/configfs.c:328:	if (uuid_be_to_bin(page, &ns->uuid))
drivers/nvme/target/nvmet.h:49:	uuid_be			uuid;

And RFC4122 4.1.2 says MSB first as well.

> 
> Also, it would be useful to generate a random default with uuid_le_gen()
> for a better out-of-the-box experience.

Yep, sounds reasonable.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1656716 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

On Fri, Jun 02, 2017 at 04:46:40PM +0300, Sagi Grimberg wrote:
>
>>  	switch (type) {
>>  	case NVME_NQN_NVME:
>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>> index cfc5c7fb0ab7..4c6cb5ea1186 100644
>> --- a/drivers/nvme/target/nvmet.h
>> +++ b/drivers/nvme/target/nvmet.h
>> @@ -46,6 +46,7 @@ struct nvmet_ns {
>>  	u32			blksize_shift;
>>  	loff_t			size;
>>  	u8			nguid[16];
>> +	uuid_be			uuid;
>
> This should be uuid_le to match the rest of NVMe.

It shouldn't.  A RFC4122 uuid is always in "big endian" notation.
A little endiane uuid doesn't exist, it's called a GUID in Wintel
speak and isn't used in NVMe.  My new uuid tree tries to fix up
the confusion by renaming our uuid_be type to uuid_t and uuid_le
to guid_t.

> Also, it would be useful to generate a random default with uuid_le_gen()
> for a better out-of-the-box experience.

That's actually a good point.  Just needs to be uuid_be_gen/uuid_gen :)

[toc] | [prev] | [next] | [standalone]

#1656751 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

>>>   	switch (type) {
>>>   	case NVME_NQN_NVME:
>>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>>> index cfc5c7fb0ab7..4c6cb5ea1186 100644
>>> --- a/drivers/nvme/target/nvmet.h
>>> +++ b/drivers/nvme/target/nvmet.h
>>> @@ -46,6 +46,7 @@ struct nvmet_ns {
>>>   	u32			blksize_shift;
>>>   	loff_t			size;
>>>   	u8			nguid[16];
>>> +	uuid_be			uuid;
>>
>> This should be uuid_le to match the rest of NVMe.
> 
> It shouldn't.  A RFC4122 uuid is always in "big endian" notation.
> A little endiane uuid doesn't exist, it's called a GUID in Wintel
> speak and isn't used in NVMe.  My new uuid tree tries to fix up
> the confusion by renaming our uuid_be type to uuid_t and uuid_le
> to guid_t.

Thanks for clarifying.

At one point we had a uuid before we allowed user-space to pass a nguid
and it was little endian, but it was a hack anyway.

[toc] | [prev] | [next] | [standalone]

#1657276 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

What about a little helper like this: ?

static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
		void *id, off_t *off)
{
	struct nvme_ns_identifier_hdr hdr = {
		.nidt	= type,
		.nidl	= len,
	};
	u16 status;

	status = nvmet_copy_to_sgl(req, *off, &hdr, sizeof(hdr));
	if (status)
		return status;
	*off += sizeof(hdr);

	status = nvmet_copy_to_sgl(req, off, id, len);
	if (status)
		return status;
	*off += len;

	return 0;
}

[toc] | [prev] | [next] | [standalone]

#1658418 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

On 06/05/2017 07:31 AM, Christoph Hellwig wrote:
> What about a little helper like this: ?
> 
> static u16 nvmet_copy_ns_identifier(struct nvmet_req *req, u8 type, u8 len,
> 		void *id, off_t *off)
> {
> 	struct nvme_ns_identifier_hdr hdr = {
> 		.nidt	= type,
> 		.nidl	= len,
> 	};
> 	u16 status;
> 
> 	status = nvmet_copy_to_sgl(req, *off, &hdr, sizeof(hdr));
> 	if (status)
> 		return status;
> 	*off += sizeof(hdr);
> 
> 	status = nvmet_copy_to_sgl(req, off, id, len);
> 	if (status)
> 		return status;
> 	*off += len;
> 
> 	return 0;
> }
> 

Yep sounds good. Sorry I didn't see this for the v4 submission, I'll
include in (the hopefully final) v5.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [next] | [standalone]

#1658425 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

Can you rebase on top of the new uuid_t type and the plain uuid_*
prefixed helpers here:

http://git.infradead.org/users/hch/uuid.git/shortlog/refs/heads/for-next

I'll declare that branch stable soon and don't want to introduce
more users of the old types.  Thanks!

[toc] | [prev] | [next] | [standalone]

#1658438 — Re: [PATCH v3 3/8] nvmet: implement namespace identify descriptor list

On 06/06/2017 09:19 AM, Christoph Hellwig wrote:
> Can you rebase on top of the new uuid_t type and the plain uuid_*
> prefixed helpers here:
> 
> http://git.infradead.org/users/hch/uuid.git/shortlog/refs/heads/for-next
> 
> I'll declare that branch stable soon and don't want to introduce
> more users of the old types.  Thanks!

Sure


-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web