From mboxrd@z Thu Jan 1 00:00:00 1970 From: hch@lst.de (Christoph Hellwig) Date: Thu, 1 Jun 2017 08:42:33 +0200 Subject: [PATCH v1 3/7] nvmet: implement namespace identify descriptor list In-Reply-To: References: Message-ID: <20170601064233.GC16778@lst.de> On Wed, May 31, 2017@03:32:13PM +0200, Johannes Thumshirn wrote: > 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 EUI-64, 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. > > Signed-off-by: Johannes Thumshirn > --- > drivers/nvme/target/admin-cmd.c | 63 +++++++++++++++++++++++++++++++++++++++++ > include/linux/nvme.h | 18 ++++++++++++ Can you split all the new structures in nvme.h into a separate patch at the beginning of the series? > + static const int buf_size = SZ_4K; Please add a NVME_IDENTIFY_DATA_SIZE define to nvme.h (and I'd slighty prefer to define it to 4096 instead of the odd SZ_4K). And also replace all the places where we use a magic 4096 for the identify payload with it. > + nid_list = kzalloc(buf_size, GFP_KERNEL); > + if (!nid_list) { > + status = NVME_SC_INTERNAL; > + goto out_put_ns; > + } > + > + p = nid_list; No need for the dynamic allocation. Just do a straight nvmet_copy_to_sgl from the stack for each element. > + > + if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) { > + ns_nid = (struct nvme_ns_nid *)p; > + ns_nid->nidt = NVME_NIDT_UUID; > + ns_nid->nidl = NVME_NIDT_UUID_LEN; > + memcpy(&ns_nid->nid, &ns->uuid, sizeof(ns->uuid)); > + pos += sizeof(struct nvme_ns_nid) + sizeof(ns->uuid); > + if (pos > buf_size) { > + status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR; > + goto out_free_nid_list; > + } > + p += pos; > + } E.g. something like struct nvme_ns_identifier_hdr nih; ... memset(&nih, 0, sizeof(nih)); nih.nidt = NVME_NIDT_UUID; nih.nidl = NVME_NIDT_UUID_LEN; status = nvmet_copy_to_sgl(req, off, &nih, sizeof(nih)); if (status) goto out_put_ns; off += sizeof(nih); status = nvmet_copy_to_sgl(req, off, &ns->uuid, sizeof(ns->uuid)); if (status) goto out_put_ns; off += sizeof(ns->uuid); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751284AbdFAGmf (ORCPT ); Thu, 1 Jun 2017 02:42:35 -0400 Received: from verein.lst.de ([213.95.11.211]:44465 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751047AbdFAGme (ORCPT ); Thu, 1 Jun 2017 02:42:34 -0400 Date: Thu, 1 Jun 2017 08:42:33 +0200 From: Christoph Hellwig To: Johannes Thumshirn Cc: Christoph Hellwig , Sagi Grimberg , Keith Busch , Hannes Reinecke , Max Gurtovoy , Linux NVMe Mailinglist , Linux Kernel Mailinglist Subject: Re: [PATCH v1 3/7] nvmet: implement namespace identify descriptor list Message-ID: <20170601064233.GC16778@lst.de> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 31, 2017 at 03:32:13PM +0200, Johannes Thumshirn wrote: > 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 EUI-64, 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. > > Signed-off-by: Johannes Thumshirn > --- > drivers/nvme/target/admin-cmd.c | 63 +++++++++++++++++++++++++++++++++++++++++ > include/linux/nvme.h | 18 ++++++++++++ Can you split all the new structures in nvme.h into a separate patch at the beginning of the series? > + static const int buf_size = SZ_4K; Please add a NVME_IDENTIFY_DATA_SIZE define to nvme.h (and I'd slighty prefer to define it to 4096 instead of the odd SZ_4K). And also replace all the places where we use a magic 4096 for the identify payload with it. > + nid_list = kzalloc(buf_size, GFP_KERNEL); > + if (!nid_list) { > + status = NVME_SC_INTERNAL; > + goto out_put_ns; > + } > + > + p = nid_list; No need for the dynamic allocation. Just do a straight nvmet_copy_to_sgl from the stack for each element. > + > + if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) { > + ns_nid = (struct nvme_ns_nid *)p; > + ns_nid->nidt = NVME_NIDT_UUID; > + ns_nid->nidl = NVME_NIDT_UUID_LEN; > + memcpy(&ns_nid->nid, &ns->uuid, sizeof(ns->uuid)); > + pos += sizeof(struct nvme_ns_nid) + sizeof(ns->uuid); > + if (pos > buf_size) { > + status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR; > + goto out_free_nid_list; > + } > + p += pos; > + } E.g. something like struct nvme_ns_identifier_hdr nih; ... memset(&nih, 0, sizeof(nih)); nih.nidt = NVME_NIDT_UUID; nih.nidl = NVME_NIDT_UUID_LEN; status = nvmet_copy_to_sgl(req, off, &nih, sizeof(nih)); if (status) goto out_put_ns; off += sizeof(nih); status = nvmet_copy_to_sgl(req, off, &ns->uuid, sizeof(ns->uuid)); if (status) goto out_put_ns; off += sizeof(ns->uuid);