Re: [PATCH] hw/nvme: add param to control auto zone transitioning to zone state closed

[Klaus Jensen <its@irrelevant.dk>]

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

On May 31 21:39, Klaus Jensen wrote:
>On May 31 15:42, Niklas Cassel wrote:
>>On Fri, May 28, 2021 at 01:22:38PM +0200, Klaus Jensen wrote:
>>>On May 28 11:05, Niklas Cassel wrote:
>>>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>>>
>>>> In the Zoned Namespace Command Set Specification, chapter
>>>> 2.5.1 Managing resources
>>>>
>>>> "The controller may transition zones in the ZSIO:Implicitly Opened state
>>>> to the ZSC:Closed state for resource management purposes."
>>>>
>>>> The word may in this sentence means that automatically transitioning
>>>> an implicitly opened zone to closed is completely optional.
>>>>
>>>> Add a new parameter so that the user can control if this automatic
>>>> transitioning should be performed or not.
>>>>
>>>> Being able to control this can help with verifying that e.g. a user-space
>>>> program behaves properly even without this optional ZNS feature.
>>>>
>>>> The default value is set to true, in order to not change the existing
>>>> behavior.
>>>>
>>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>>> ---
>>>> hw/nvme/ctrl.c | 9 ++++++++-
>>>> hw/nvme/ns.c   | 2 ++
>>>> hw/nvme/nvme.h | 1 +
>>>> 3 files changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>>>> index 40a7efcea9..d00f0297a5 100644
>>>> --- a/hw/nvme/ctrl.c
>>>> +++ b/hw/nvme/ctrl.c
>>>> @@ -141,6 +141,11 @@
>>>>  *
>>>>  *     zoned.cross_read=<enable RAZB, default: false>
>>>>  *         Setting this property to true enables Read Across Zone Boundaries.
>>>> + *
>>>> + *     zoned.auto_transition=<enable auto resource management, default: true>
>>>> + *         Indicates if zones in zone state implicitly opened can be
>>>> + *         automatically transitioned to zone state closed for resource
>>>> + *         management purposes.
>>>>  */
>>>>
>>>> #include "qemu/osdep.h"
>>>> @@ -1699,7 +1704,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>>>>         /* fallthrough */
>>>>
>>>>     case NVME_ZONE_STATE_CLOSED:
>>>> -        nvme_zrm_auto_transition_zone(ns);
>>>> +        if (ns->params.auto_transition_zones) {
>>>> +            nvme_zrm_auto_transition_zone(ns);
>>>> +        }
>>>>         status = nvme_aor_check(ns, act, 1);
>>>>         if (status) {
>>>>             return status;
>>>> diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
>>>> index 3fec9c6273..31dee43d30 100644
>>>> --- a/hw/nvme/ns.c
>>>> +++ b/hw/nvme/ns.c
>>>> @@ -531,6 +531,8 @@ static Property nvme_ns_props[] = {
>>>>                        params.max_open_zones, 0),
>>>>     DEFINE_PROP_UINT32("zoned.descr_ext_size", NvmeNamespace,
>>>>                        params.zd_extension_size, 0),
>>>> +    DEFINE_PROP_BOOL("zoned.auto_transition", NvmeNamespace,
>>>> +                     params.auto_transition_zones, true),
>>>>     DEFINE_PROP_END_OF_LIST(),
>>>> };
>>>>
>>>> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>>>> index 81a35cda14..bd86054db2 100644
>>>> --- a/hw/nvme/nvme.h
>>>> +++ b/hw/nvme/nvme.h
>>>> @@ -100,6 +100,7 @@ typedef struct NvmeNamespaceParams {
>>>>     uint32_t max_active_zones;
>>>>     uint32_t max_open_zones;
>>>>     uint32_t zd_extension_size;
>>>> +    bool     auto_transition_zones;
>>>> } NvmeNamespaceParams;
>>>>
>>>> typedef struct NvmeNamespace {
>>>> --
>>>> 2.31.1
>>>>
>>>
>>>Looks good Niklas!
>>>
>>>Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>>
>>In reality, it is the controller that does the auto transitioning.
>>
>>In theory, one namespace could be attached to two different controllers,
>>and I guess, in that case, it depends on if the controller that we used
>>when doing the write supports auto transitioning or not, that determines
>>if a zone will be auto transitioned or not.
>>
>>If we were to change this to be a parameter of the controller instead
>>of a parameter of the namespace, we would require to refactor a lot of
>>code in the regular write path. As we currently don't have any NvmeRequest
>>object in nvme_zrm_open_flags().
>>
>>Thoughts?
>>
>
>I think you are right. This should be controller-specific behavior. I 
>took the liberty of moving the parameter; the refactor is minimal.
>
>
>From: Niklas Cassel <niklas.cassel@wdc.com>
>
>In the Zoned Namespace Command Set Specification, chapter
>2.5.1 Managing resources
>
>"The controller may transition zones in the ZSIO:Implicitly Opened state
>to the ZSC:Closed state for resource management purposes."
>
>The word may in this sentence means that automatically transitioning
>an implicitly opened zone to closed is completely optional.
>
>Add a new parameter so that the user can control if this automatic
>transitioning should be performed or not.
>
>Being able to control this can help with verifying that e.g. a user-space
>program behaves properly even without this optional ZNS feature.
>
>The default value is set to true, in order to not change the existing
>behavior.
>
>Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>[k.jensen: moved parameter to controller]
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/nvme/nvme.h |  1 +
> hw/nvme/ctrl.c | 32 ++++++++++++++++++++++----------
> 2 files changed, 23 insertions(+), 10 deletions(-)
>
>diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
>index 81a35cda142b..93a7e0e5380e 100644
>--- a/hw/nvme/nvme.h
>+++ b/hw/nvme/nvme.h
>@@ -382,6 +382,7 @@ typedef struct NvmeParams {
>     uint8_t  vsl;
>     bool     use_intel_id;
>     uint8_t  zasl;
>+    bool     auto_transition_zones;
>     bool     legacy_cmb;
> } NvmeParams;
>diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
>index 40a7efcea914..8dd9cb2ccbf3 100644
>--- a/hw/nvme/ctrl.c
>+++ b/hw/nvme/ctrl.c
>@@ -34,6 +34,7 @@
>  *              aerl=<N[optional]>,aer_max_queued=<N[optional]>, \
>  *              mdts=<N[optional]>,vsl=<N[optional]>, \
>  *              zoned.zasl=<N[optional]>, \
>+ *              zoned.auto_transition=<on|off[optional]>, \
>  *              subsys=<subsys_id>
>  *      -device nvme-ns,drive=<drive_id>,bus=<bus_name>,nsid=<nsid>,\
>  *              zoned=<true|false[optional]>, \
>@@ -100,6 +101,11 @@
>  *   the minimum memory page size (CAP.MPSMIN). The default value is 0 (i.e.
>  *   defaulting to the value of `mdts`).
>  *
>+ * - `zoned.auto_transition`
>+ *   Indicates if zones in zone state implicitly opened can be automatically
>+ *   transitioned to zone state closed for resource management purposes.
>+ *   Defaults to 'on'.
>+ *
>  * nvme namespace device parameters
>  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  * - `shared`
>@@ -1686,8 +1692,8 @@ enum {
>     NVME_ZRM_AUTO = 1 << 0,
> };
>-static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>-                                    int flags)
>+static uint16_t nvme_zrm_open_flags(NvmeCtrl *n, NvmeNamespace *ns,
>+                                    NvmeZone *zone, int flags)
> {
>     int act = 0;
>     uint16_t status;
>@@ -1699,7 +1705,9 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>         /* fallthrough */
>     case NVME_ZONE_STATE_CLOSED:
>-        nvme_zrm_auto_transition_zone(ns);
>+        if (n->params.auto_transition_zones) {
>+            nvme_zrm_auto_transition_zone(ns);
>+        }
>         status = nvme_aor_check(ns, act, 1);
>         if (status) {
>             return status;
>@@ -1735,14 +1743,16 @@ static uint16_t nvme_zrm_open_flags(NvmeNamespace *ns, NvmeZone *zone,
>     }
> }
>-static inline uint16_t nvme_zrm_auto(NvmeNamespace *ns, NvmeZone *zone)
>+static inline uint16_t nvme_zrm_auto(NvmeCtrl *n, NvmeNamespace *ns,
>+                                     NvmeZone *zone)
> {
>-    return nvme_zrm_open_flags(ns, zone, NVME_ZRM_AUTO);
>+    return nvme_zrm_open_flags(n, ns, zone, NVME_ZRM_AUTO);
> }
>-static inline uint16_t nvme_zrm_open(NvmeNamespace *ns, NvmeZone *zone)
>+static inline uint16_t nvme_zrm_open(NvmeCtrl *n, NvmeNamespace *ns,
>+                                     NvmeZone *zone)
> {
>-    return nvme_zrm_open_flags(ns, zone, 0);
>+    return nvme_zrm_open_flags(n, ns, zone, 0);
> }
> static void nvme_advance_zone_wp(NvmeNamespace *ns, NvmeZone *zone,
>@@ -2283,7 +2293,7 @@ static void nvme_copy_in_complete(NvmeRequest *req)
>             goto invalid;
>         }
>-        status = nvme_zrm_auto(ns, zone);
>+        status = nvme_zrm_auto(nvme_ctrl(req), ns, zone);
>         if (status) {
>             goto invalid;
>         }
>@@ -3080,7 +3090,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
>             goto invalid;
>         }
>-        status = nvme_zrm_auto(ns, zone);
>+        status = nvme_zrm_auto(n, ns, zone);
>         if (status) {
>             goto invalid;
>         }
>@@ -3169,7 +3179,7 @@ enum NvmeZoneProcessingMask {
> static uint16_t nvme_open_zone(NvmeNamespace *ns, NvmeZone *zone,
>                                NvmeZoneState state, NvmeRequest *req)
> {
>-    return nvme_zrm_open(ns, zone);
>+    return nvme_zrm_open(nvme_ctrl(req), ns, zone);
> }
> static uint16_t nvme_close_zone(NvmeNamespace *ns, NvmeZone *zone,
>@@ -6259,6 +6269,8 @@ static Property nvme_props[] = {
>     DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false),
>     DEFINE_PROP_BOOL("legacy-cmb", NvmeCtrl, params.legacy_cmb, false),
>     DEFINE_PROP_UINT8("zoned.zasl", NvmeCtrl, params.zasl, 0),
>+    DEFINE_PROP_BOOL("zoned.auto_transition", NvmeCtrl,
>+                     params.auto_transition_zones, true),
>     DEFINE_PROP_END_OF_LIST(),
> };
>-- 
>2.31.1
>

Modified patch applied to nvme-next!


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