Re: [PATCH v2] hw/nvme: Support for Namespaces Management from guest OS - create-ns

[Klaus Jensen]

Hi Michael,

Thanks for your interest in contributing this!

(CC: +Naveen, +Keith, -linux-nvme)

Some observations and commets on your patch submission below.

Firstly, your mail delivery is broken - this is not getting through to
qemu-devel. Also, I don't think kernel developers have that much
interest in this, so no need to CC linux-nvme.

Also, please CC all maintainers (see ./scripts/get_maintainer.pl).

On Aug 23 17:30, Michael Kropaczek wrote:
>      Added support for NVMEe NameSpaces Mangement allowing the guest OS to
>      create, delete namespaces by issuing create-ns and delet-ns commands.
>      It is an extension to currently implemented Qemu nvme virtual device.
>      Virtual devices representing namespaces will be created and/or deleted
>      during Qemu's running session, at anytime.
> 
>       The implementation of the Namespaces Management consists of two patches:
>        First:  create-ns (this patch)
>        Second: delete-ns (a patch will follow)
> 

  1. Please format the commit message without the indendations.
  2. You do not need to state the patches belonging to the series (that
     goes into the cover letter - see below).

> Signed-off-by: Michael Kropaczek <michael.kropaczek@solidigm.com>
> 
> Description:
> 
> Currently namespaces could be defined as follows:
> 1. Legacy Namespace - just one namespace within Nvme controller's
>    where the back-end was specified for nvme device by -drive parameter
>    pointing directly to the image file.
> 2. Additional Namespaces - specified by nvme-ns devices each having its
>    own back-end. To have multiple namespaces each needed to be specified
>    at Qemu's command line being associated with the most recently defined
>    nvme-bus from nvme device.
>    If a such additional namespace should be attached and/or detached by the
>    guest OS, nvme controller has to be linked with another device nvme-subsys.
> 
> All that have a static behavior, all need to be specified at the Qemu's 
> command
> line, all specified virtual nvme entities will be processed during Qemu's
> start-up created and provided to the guest OS.
> 
> To have a support for nvme create-ns and delete-ns commands with specified
> parameters, as the NVMe specification defines, a different approach is needed.
> Virtual devices representing namespaces need to be created and/or deleted 
> during Qemu's running session, at anytime. The back-end image sizes for a
> namespace must accommodate the payload size and size of metadata resulted
> from specified parameters. And the total capacity of the nvme controller
> together with un-allocated capacity needs to be taken into account and updated
> following the commands nvme create-ns and delete-ns respectively.
> 
> Here is the approach:
> The nvme device will get new parameters:
>  - auto-ns-path, specifies the path to to image and necessary
>    configuration files.
>  - auto-ns-purge, controls behavior when delete-ns command is issued.
>    If set to 'on' the associated back-end images will be deleted,
>    otherwise such will be preserved as backup files (not Qemu backup files)
>  - auto-ns-tnvmcap, specifies total controller's space pool in bytes that
>    can be allocated for namespaces, usually when nvme device is created
>    first time. When Qemu restarted this parameter could be omitted.
> 
> The virtual devices representing namespace will be created during the Qemu
> running session dynamically. QOM classes and instances will be created
> utilizing existing configuration scheme already used for Qemu's start-up.
> Back-end images will be created and associated with QOM namespaces (virtual
> instances) or disassociated deleted or renamed. Also it is assured that all
> settings will remain persistent over Qemu start-ups and shutdowns.
> The implementation makes it possible to combine the existing
> "Additional Namespace" implementation with the new "Managed Namespaces",
> those will coexist with obvious restrictions, like both will share the same
> NsIds space, "static" namespaces will not be deleted etc..
> 

This should go into your "cover letter". See `git format-patch
--cover-letter`.

Your two patches should be sent as a series with the above as the cover
letter. Each patch should carry a commit message explaining why it is
being done and why it is being done in a particular way, but the cover
letter should (if required) elaborate on your motivation and approach
for the series as a whole.

`git format-patch` and `git send-email` can do this properly for you.
Alternatively, you can use git-publish which is a nice frontend for
working with patch series.

Now, as for the patches themselves. There is actually already a patch
for namespace management on the list - see[1] for a 7.1-based tree with
this applied. The approach there is to pre-add a set of namespaces with
zero sized images (indicating that they are "unallocated") and then
resize them on namespace create (using blk_truncate). This limits the
number of creatable namespaces, but this
is indicated in the MN field.

The core issue with that patch is that it blocks the QEMU main thread
while the truncation is going on, which is not good and which is why
this has never been merged. However, I just peeked into
block/block-backend.c and it looks like truncate could be used with AIO
(though currently not wired up).

The same core blocking issues exist in your approach. Also, doing
filesystem stuff is a layering violation. The emulated devices really
should not break out of their "bubble", even though they can.

However! I'm a little intrigued by your approach to storing persistent
state by writing out a config file and auto loading it. I have been
considering various ways of doing that for some time since there are
other areas where we need persistent state like that (format nvm is one
example, zone state another).


  [1]: 
https://github.com/OpenMPDK/qemu/commit/a7357a4c553f4106e76e3ec052205659bcc5d741


Cheers,
Klaus