Re: [PATCH] hw/nvme: reattach subsystem namespaces on hotplug

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 3302 bytes --]

On Sep  9 11:43, Hannes Reinecke wrote:
> With commit 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
> namespaces get moved from the controller to the subsystem if one
> is specified.
> That keeps the namespaces alive after a controller hot-unplug, but
> after a controller hotplug we have to reconnect the namespaces
> from the subsystem to the controller.
> 
> Fixes: 5ffbaeed16 ("hw/nvme: fix controller hot unplugging")
> Cc: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  hw/nvme/subsys.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c
> index 93c35950d6..a9404f2b5e 100644
> --- a/hw/nvme/subsys.c
> +++ b/hw/nvme/subsys.c
> @@ -14,7 +14,7 @@
>  int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
>  {
>      NvmeSubsystem *subsys = n->subsys;
> -    int cntlid;
> +    int cntlid, nsid;
>  
>      for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) {
>          if (!subsys->ctrls[cntlid]) {
> @@ -29,12 +29,18 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp)
>  
>      subsys->ctrls[cntlid] = n;
>  
> +    for (nsid = 0; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) {
> +        if (subsys->namespaces[nsid]) {
> +            nvme_attach_ns(n, subsys->namespaces[nsid]);
> +        }

Thanks Hannes! I like it, keeping things simple.

But we should only attach namespaces that have the shared property or
have ns->attached == 0. Non-shared namespaces may already be attached to
another controller in the subsystem.

However...

The spec says that "The attach and detach operations are persistent
across all reset events.". This means that we should track those events
in the subsystem and only reattach namespaces that were attached at the
time of the "reset" event. Currently, we don't have anything mapping
that state. But the device already has to take some liberties with
regard to stuff that is considered persistent by the spec (SMART log
etc.) since we do not have any way to store stuff persistently across
qemu invocations, so I think the above is an acceptable compromise.

A potential (as good as it gets) fix would be to keep a map/list of
"persistently" attached controllers on the namespaces and re-attach
according to that when we see that controller joining the subsystem
again. CNTLID would be the obvious choice for the key here, but problem
is that we cant really use it since we assign it sequentially from the
subsystem, which now looks like a pretty bad choice. CNTLID should have
been a required property of the nvme device when subsystems are
involved. Maybe we can fix up the CNTLID assignment to take the serial
into account (we know that is defined and *should* be unique) and not
reuse CNTLIDs. This limits the subsystem to NVME_MAX_CONTROLLERS unique
controllers, but I think that is fair enough.

Sigh. Need to think this through.

Bottomline I think I'm partial to just accepting your patch (with the
addition of taking the shared property into account) and documenting the
limitation wrt. persistency of attach/detach events. No matter how
spec-compliant we do it on a live system, we still break compliance
across QEMU invocations.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]