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.cas...@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.cas...@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.jen...@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.cas...@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.cas...@wdc.com> [k.jensen: moved parameter to controller] Signed-off-by: Klaus Jensen <k.jen...@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
signature.asc
Description: PGP signature