[v2] Add OCP extended log to nvme QEMU

[PATCH v2 0/3] Add OCP extended log to nvme QEMU

Posted by Joel Granados 1 year, 5 months ago

The motivation and description are contained in the last patch in this set.
Will copy paste it here for convenience:

    In order to evaluate write amplification factor (WAF) within the storage
    stack it is important to know the number of bytes written to the
    controller. The existing SMART log value of Data Units Written is too
    coarse (given in units of 500 Kb) and so we add the SMART health
    information extended from the OCP specification (given in units of bytes).

    To accommodate different vendor specific specifications like OCP, we add a
    multiplexing function (nvme_vendor_specific_log) which will route to the
    different log functions based on arguments and log ids. We only return the
    OCP extended smart log when the command is 0xC0 and ocp has been turned on
    in the args.

    Though we add the whole nvme smart log extended structure, we only populate
    the physical_media_units_{read,written}, log_page_version and
    log_page_uuid.

V1 changes:
1. I moved the ocp parameter from the namespace to the subsystem as it is
   defined there in the OCP specification
2. I now accumulate statistics from all namespaces and report them back on
   the extended log as per the spec.
3. I removed the default case in the switch in nvme_vendor_specific_log as
   it does not have any special function.

Joel Granados (3):
  nvme: Move adjustment of data_units{read,written}
  nvme: Add ocp to the subsys
  nvme: Add physical writes/reads from OCP log

 hw/nvme/ctrl.c       | 70 ++++++++++++++++++++++++++++++++++++++++----
 hw/nvme/nvme.h       |  1 +
 hw/nvme/subsys.c     |  4 +--
 include/block/nvme.h | 36 +++++++++++++++++++++++
 4 files changed, 103 insertions(+), 8 deletions(-)

-- 
2.30.2

Re: [PATCH v2 0/3] Add OCP extended log to nvme QEMU

Posted by Christoph Hellwig 1 year, 4 months ago

Please don't do this.  OCP is acting as a counter standard to the
proper NVMe standard here and should in absolutely no way be supported
by open source projects that needs to stick to the actual standards.

Please work with the NVMe technical working group to add this (very
useful) functionality to NVMe proper first.

On Mon, Nov 14, 2022 at 02:50:40PM +0100, Joel Granados wrote:
> The motivation and description are contained in the last patch in this set.
> Will copy paste it here for convenience:
> 
>     In order to evaluate write amplification factor (WAF) within the storage
>     stack it is important to know the number of bytes written to the
>     controller. The existing SMART log value of Data Units Written is too
>     coarse (given in units of 500 Kb) and so we add the SMART health
>     information extended from the OCP specification (given in units of bytes).
> 
>     To accommodate different vendor specific specifications like OCP, we add a
>     multiplexing function (nvme_vendor_specific_log) which will route to the
>     different log functions based on arguments and log ids. We only return the
>     OCP extended smart log when the command is 0xC0 and ocp has been turned on
>     in the args.
> 
>     Though we add the whole nvme smart log extended structure, we only populate
>     the physical_media_units_{read,written}, log_page_version and
>     log_page_uuid.
> 
> V1 changes:
> 1. I moved the ocp parameter from the namespace to the subsystem as it is
>    defined there in the OCP specification
> 2. I now accumulate statistics from all namespaces and report them back on
>    the extended log as per the spec.
> 3. I removed the default case in the switch in nvme_vendor_specific_log as
>    it does not have any special function.
> 
> Joel Granados (3):
>   nvme: Move adjustment of data_units{read,written}
>   nvme: Add ocp to the subsys
>   nvme: Add physical writes/reads from OCP log
> 
>  hw/nvme/ctrl.c       | 70 ++++++++++++++++++++++++++++++++++++++++----
>  hw/nvme/nvme.h       |  1 +
>  hw/nvme/subsys.c     |  4 +--
>  include/block/nvme.h | 36 +++++++++++++++++++++++
>  4 files changed, 103 insertions(+), 8 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
---end quoted text---

Re: [PATCH v2 0/3] Add OCP extended log to nvme QEMU

Posted by Joel Granados 1 year, 3 months ago

On Thu, Dec 22, 2022 at 11:39:54PM -0800, Christoph Hellwig wrote:
> Please don't do this.  OCP is acting as a counter standard to the
> proper NVMe standard here and should in absolutely no way be supported
> by open source projects that needs to stick to the actual standards.
> 
> Please work with the NVMe technical working group to add this (very
> useful) functionality to NVMe proper first.

This is a very good point. Regardless of what OCP's ultimate objective,
having this in the NVMe specification would reach more cases. We can
even use existing values like the "Media Bytes with Metadata Written" in
the statistics log page of the newly ratified FDP TP.

Thx for the review

Best

Joel

> 
> On Mon, Nov 14, 2022 at 02:50:40PM +0100, Joel Granados wrote:
> > The motivation and description are contained in the last patch in this set.
> > Will copy paste it here for convenience:
> > 
> >     In order to evaluate write amplification factor (WAF) within the storage
> >     stack it is important to know the number of bytes written to the
> >     controller. The existing SMART log value of Data Units Written is too
> >     coarse (given in units of 500 Kb) and so we add the SMART health
> >     information extended from the OCP specification (given in units of bytes).
> > 
> >     To accommodate different vendor specific specifications like OCP, we add a
> >     multiplexing function (nvme_vendor_specific_log) which will route to the
> >     different log functions based on arguments and log ids. We only return the
> >     OCP extended smart log when the command is 0xC0 and ocp has been turned on
> >     in the args.
> > 
> >     Though we add the whole nvme smart log extended structure, we only populate
> >     the physical_media_units_{read,written}, log_page_version and
> >     log_page_uuid.
> > 
> > V1 changes:
> > 1. I moved the ocp parameter from the namespace to the subsystem as it is
> >    defined there in the OCP specification
> > 2. I now accumulate statistics from all namespaces and report them back on
> >    the extended log as per the spec.
> > 3. I removed the default case in the switch in nvme_vendor_specific_log as
> >    it does not have any special function.
> > 
> > Joel Granados (3):
> >   nvme: Move adjustment of data_units{read,written}
> >   nvme: Add ocp to the subsys
> >   nvme: Add physical writes/reads from OCP log
> > 
> >  hw/nvme/ctrl.c       | 70 ++++++++++++++++++++++++++++++++++++++++----
> >  hw/nvme/nvme.h       |  1 +
> >  hw/nvme/subsys.c     |  4 +--
> >  include/block/nvme.h | 36 +++++++++++++++++++++++
> >  4 files changed, 103 insertions(+), 8 deletions(-)
> > 
> > -- 
> > 2.30.2
> > 
> > 
> ---end quoted text---