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

[Michael Kropaczek (CW)]

-----Original Message-----
From: Keith Busch <kbusch@kernel.org> 
Sent: Thursday, October 27, 2022 1:21 PM
To: Jonathan Derrick <jonathan.derrick@linux.dev>
Cc: qemu-devel@nongnu.org; Michael Kropaczek (CW) 
<Michael.Kropaczek@solidigm.com>; qemu-block@nongnu.org; Klaus Jensen 
<its@irrelevant.dk>; Kevin Wolf <kwolf@redhat.com>; Hanna Reitz 
<hreitz@redhat.com>
Subject: Re: [PATCH v3 1/2] hw/nvme: Support for Namespaces Management from 
guest OS - create-ns

[You don't often get email from kbusch@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=<path to nvme storage location>``
> +  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_<ctrl SN>_ns_<NS-ID>.cfg' and 'nvme_<ctrl SN>_ns_<NS-ID>.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 SN>_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 <serial> -C <total NVMe capacity>}
> +                     [-N <NsId max>] {<path>}
> +
> +  Parameters:
> +  -S and -C and <path> are mandatory, `-S` must match `serial` 
> + parameter  and <path> 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(char *ns_directory, char *serial, 
> +QDict *ns_cfg, uint32_t nsid) {
> +    GString *json = NULL;
> +    char *filename;
> +    FILE *fp;
> +    int ret = 0;
> +    Error *local_err = NULL;
> +
> +    json = qobject_to_json_pretty(QOBJECT(ns_cfg), false);
> +
> +    if (strlen(json->str) + 2 /* '\n'+'\0' */ > NS_CFG_MAXSIZE) {
> +        error_setg(&local_err, "ns-cfg allowed max size %d exceeded", 
> + NS_CFG_MAXSIZE);

I find this whole "local_err" control flow unpleasant to follow. The 
local_error gets set in this above condition only to be overwritten in the very 
next step without ever being used? Surely that can't be right.

I'm just picking on this one example here, but the pattern appears to repeat. I 
think this would be easier to read if the error conditions took 'goto' paths to 
unwind so that the good path doesn't require such deep 'if/else' nesting.
[MK]: this will be corrected.
> +    }
> +
> +    filename = create_cfg_name(ns_directory, serial, nsid, &local_err);
> +    if (!local_err) {
> +        fp = fopen(filename, "w");
> +        if (fp == NULL) {
> +            error_setg(&local_err, "open %s: %s", filename,
> +                         strerror(errno));
> +        } else {
> +            chmod(filename, 0644);
> +            if (!fprintf(fp, "%s\n", json->str)) {
> +                error_setg(&local_err, "could not write ns-cfg %s: %s", 
> filename,
> +                             strerror(errno));
> +            }
> +            fclose(fp);
> +        }
> +    }
> +
> +    if (local_err) {
> +        error_report_err(local_err);
> +        ret = -1;
> +    }
> +
> +    g_string_free(json, true);
> +    g_free(filename);
> +    qobject_unref(ns_cfg);
> +
> +    return ret;
> +}