RE: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns

[Michael Kropaczek (CW)]

Hi Keith,

Thank you for the review and I agree with you, the issues will be addressed.

Regards,
  Michael

-Original Message-
From: Keith Busch  
Sent: Wednesday, December 28, 2022 12:10 PM
To: Jonathan Derrick 
Cc: qemu-de...@nongnu.org; Michael Kropaczek (CW) 
; qemu-block@nongnu.org; Klaus Jensen 
; Kevin Wolf ; Hanna Reitz 

Subject: Re: [PATCH v4 1/2] hw/nvme: Support for Namespaces Management from 
guest OS - create-ns

[You don't often get email from kbu...@kernel.org. Learn why this is important 
at https://aka.ms/LearnAboutSenderIdentification ]

Caution: External Email


On Wed, Dec 28, 2022 at 01:41:40PM -0600, Jonathan Derrick wrote:
> +static uint16_t nvme_ns_mgmt(NvmeCtrl *n, NvmeRequest *req) {
> +NvmeCtrl *n_p = NULL; /* primary controller */
> +NvmeIdCtrl *id = >id_ctrl;
> +NvmeNamespace *ns;
> +NvmeIdNsMgmt id_ns = {};
> +uint8_t flags = req->cmd.flags;
> +uint32_t nsid = le32_to_cpu(req->cmd.nsid);
> +uint32_t dw10 = le32_to_cpu(req->cmd.cdw10);
> +uint32_t dw11 = le32_to_cpu(req->cmd.cdw11);
> +uint8_t sel = dw10 & 0xf;
> +uint8_t csi = (dw11 >> 24) & 0xf;
> +uint16_t i;
> +uint16_t ret;
> +Error *local_err = NULL;
> +
> +trace_pci_nvme_ns_mgmt(nvme_cid(req), nsid, sel, csi, 
> + NVME_CMD_FLAGS_PSDT(flags));
> +
> +if (!(le16_to_cpu(id->oacs) & NVME_OACS_NS_MGMT)) {
> +return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR;
> +}
> +
> +if (n->cntlid && !n->subsys) {
> +error_setg(_err, "Secondary controller without subsystem");
> +return NVME_NS_ATTACH_MGMT_NOTSPRD | NVME_DNR;

Leaking local_err. Any time you call error_setg(), the error needs to be 
reported or freed at some point.

> +}
> +
> +n_p = n->subsys->ctrls[0];
> +
> +switch (sel) {
> +case NVME_NS_MANAGEMENT_CREATE:
> +switch (csi) {
> +case NVME_CSI_NVM:

The following case is sufficiently large enough that the implementation should 
be its own function.

> +if (nsid) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +ret = nvme_h2c(n, (uint8_t *)_ns, sizeof(id_ns), req);
> +if (ret) {
> +return ret;
> +}
> +
> +uint64_t nsze = le64_to_cpu(id_ns.nsze);
> +uint64_t ncap = le64_to_cpu(id_ns.ncap);

Please don't mix declarations with code; declare these local variables at the 
top of the scope.

> +
> +if (ncap > nsze) {
> +return NVME_INVALID_FIELD | NVME_DNR;
> +} else if (ncap != nsze) {
> +return NVME_THIN_PROVISION_NOTSPRD | NVME_DNR;
> +}
> +
> +nvme_validate_flbas(id_ns.flbas, _err);
> +if (local_err) {
> +error_report_err(local_err);
> +return NVME_INVALID_FORMAT | NVME_DNR;
> +}
> +
> +for (i = 1; i <= NVME_MAX_NAMESPACES; i++) {
> +if (nvme_ns(n_p, (uint32_t)i) || nvme_subsys_ns(n_p->subsys, 
> (uint32_t)i)) {
> +continue;
> +}
> +break;
> +}
> +
> +
> +if (i >  le32_to_cpu(n_p->id_ctrl.nn) || i >  
> NVME_MAX_NAMESPACES) {
> +   return NVME_NS_IDNTIFIER_UNAVAIL | NVME_DNR;
> +}
> +nsid = i;
> +
> +/* create ns here */
> +ns = nvme_ns_mgmt_create(n, nsid, _ns, _err);
> +if (!ns || local_err) {
> +if (local_err) {
> +error_report_err(local_err);
> +}
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +if (nvme_cfg_update(n, ns->size, NVME_NS_ALLOC_CHK)) {
> +/* place for delete-ns */
> +error_setg(_err, "Insufficient capacity, an orphaned 
> ns[%"PRIu32"] will be left behind", nsid);
> +return NVME_NS_INSUFFICIENT_CAPAC | NVME_DNR;

Leaked local_err.

> +}
> +(void)nvme_cfg_update(n, ns->size, NVME_NS_ALLOC);
> +if (nvme_cfg_save(n)) {
> +(void)nvme_cfg_update(n, ns->size, NVME_NS_DEALLOC);
> +/* place for delete-ns */
> +error_setg(_err, "Cannot save conf file, an orphaned 
> ns[%"PRIu32"] will be left behind", nsid);
> +return NVME_INVALID_FIELD | NVME_DNR;

Another leaked local_err.

> +}
> +req->cqe.result = cpu_to_le32(nsid);
> +break;
> +case NVME_CSI_ZONED:
> +/* fall through for now */
> +default:
> +return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +break;
> +default:
> +return NVME_INVALID_FIELD | NVME_DNR;
> +}
> +
> +return NVME_SUCCESS;
> +}

RE: [PATCH v3 1/2] hw/nvme: Support for Namespaces Management from guest OS - create-ns

[Michael Kropaczek (CW)]

-Original Message-
From: Keith Busch  
Sent: Thursday, October 27, 2022 1:21 PM
To: Jonathan Derrick 
Cc: qemu-de...@nongnu.org; Michael Kropaczek (CW) 
; qemu-block@nongnu.org; Klaus Jensen 
; Kevin Wolf ; Hanna Reitz 

Subject: Re: [PATCH v3 1/2] hw/nvme: Support for Namespaces Management from 
guest OS - create-ns

[You don't often get email from kbu...@kernel.org. Learn why this is important 
at https://aka.ms/LearnAboutSenderIdentification ]

Caution: External Email


On Thu, Oct 27, 2022 at 01:00:45PM -0500, Jonathan Derrick wrote:
> +Parameters:
> +
> +``auto-ns-path=``
> +  If specified indicates a support for dynamic management of nvme 
> +namespaces
> +  by means of nvme create-ns command. This path points
> +  to the storage area for backend images must exist. Additionally it 
> +requires
> +  that parameter `ns-subsys` must be specified whereas parameter 
> +`drive`
> +  must not. The legacy namespace backend is disabled, instead, a pair 
> +of
> +  files 'nvme__ns_.cfg' and 'nvme__ns_.img'
> +  will refer to respective namespaces. The create-ns, attach-ns
> +  and detach-ns commands, issued at the guest side, will make changes 
> +to
> +  those files accordingly.
> +  For each namespace exists an image file in raw format and a config 
> +file
> +  containing namespace parameters and state of the attachment 
> +allowing QEMU
> +  to configure namespaces accordingly during start up. If for 
> +instance an
> +  image file has a size of 0 bytes this will be interpreted as non 
> +existent
> +  namespace. Issuing create-ns command will change the status in the 
> +config
> +  files and and will re-size the image file accordingly so the image 
> +file
> +  will be associated with the respective namespace. The main config 
> +file
> +  nvme__ctrl.cfg keeps the track of allocated capacity to 
> +the
> +  namespaces within the nvme controller.
> +  As it is the case of a typical hard drive, backend images together 
> +with
> +  config files need to be created. For this reason the qemu-img tool 
> +has
> +  been extended by adding createns command.
> +
> +   qemu-img createns {-S  -C }
> + [-N ] {}
> +
> +  Parameters:
> +  -S and -C and  are mandatory, `-S` must match `serial` 
> + parameter  and  must match `auto-ns-path` parameter of "-device 
> nvme,..."
> +  specification.
> +  -N is optional, if specified it will set a limit to the number of 
> + potential  namespaces and will reduce the number of backend images 
> + and config files  accordingly. As a default, a set of images of 0 
> + bytes size and default  config files for 256 namespaces will be created, a 
> total of 513 files.
> +
> +Please note that ``nvme-ns`` device is not required to support of 
> +dynamic namespaces management feature. It is not prohibited to assign 
> +a such device to ``nvme`` device specified to support dynamic 
> +namespace management if one has an use case to do so, however, it 
> +will only coexist and be out of the scope of Namespaces Management. 
> +NsIds will be consistently managed, creation (create-ns) of a 
> +namespace will not allocate the NsId already being taken. If 
> +``nvme-ns`` device conflicts with previously created one by create-ns (the 
> same NsId), it will break QEMU's start up.

By requiring the controller's serial number up-front, does this mean we can't 
share dynamic namespaces with other controllers in the subsystem?
[MK]: At this point namespaces of one controller can be attached to the 
subsystem. 
[MK]: More controllers would require additional parameter specifying the number 
of controllers and expanding the naming pattern of backend files.
> +static inline char *create_fmt_name(const char *fmt, char 
> +*ns_directory, char *serial, uint32_t nsid, Error **errp) {
> +char *file_name = NULL;
> +Error *local_err = NULL;
> +
> +storage_path_check(ns_directory, serial, errp);
> +if (local_err) {

How is 'local_err' ever *not* NULL here? Are you meaning to check "*errp" 
instead?
[MK]: This will be corrected, indeed a dead code.
> +error_propagate(errp, local_err);
> +} else {
> +file_name = g_strdup_printf(fmt, ns_directory, serial, nsid);
> +}
> +
> +return file_name;
> +}
> +
> +static inline char *create_cfg_name(char *ns_directory, char *serial, 
> +uint32_t nsid, Error **errp) {
> +return create_fmt_name(NS_FILE_FMT NS_CFG_EXT, ns_directory, 
> +serial, nsid, errp); }
> +
> +
> +static inline char *create_image_name(char *ns_directory, char 
> +*serial, uint32_t nsid, Error **errp) {
> +return create_fmt_name(NS_FILE_FMT NS_IMG_EXT, ns_directory, 
> +serial, nsid, errp); }
> +
> +static inline int nsid_cfg_save(cha