Re: [virtio-dev] [PATCH v3] Add lifetime metrics to virtio-blk

[Enrico Granata <egranata@google.com>]

On Fri, Mar 5, 2021 at 4:38 AM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Wed, 3 Mar 2021 11:01:50 -0700
> Enrico Granata <egranata@google.com> wrote:
>
> > On Wed, Mar 3, 2021 at 10:18 AM Cornelia Huck <cohuck@redhat.com> wrote:
> > >
> > > On Mon, 1 Mar 2021 10:51:03 -0700
> > > Enrico Granata <egranata@google.com> wrote:
> > >
> > > > In many embedded systems, virtio-blk implementations are
> > > > backed by eMMC or UFS storage devices, which are subject to
> > > > predictable and measurable wear over time due to repeated write
> > > > cycles.
> > > >
> > > > For such systems, it can be important to be able to track
> > > > accurately the amount of wear imposed on the storage over
> > > > time and surface it to applications. In a native deployments
> > > > this is generally handled by the physical block device driver
> > > > but no such provision is made in virtio-blk to expose these
> > > > metrics for devices where it makes sense to do so.
> > > >
> > > > This patch adds support to virtio-blk for lifetime and wear
> > > > metrics to be exposed to the guest when a deployment of
> > > > virtio-blk is done over compatible eMMC or UFS storage.
> > > >
> > > > Signed-off-by: Enrico Granata <egranata@google.com>
> > > > ---
> > > >  content.tex | 31 +++++++++++++++++++++++++++++--
> > > >  1 file changed, 29 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/content.tex b/content.tex
> > > > index 835f1ea..47e3566 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -4418,6 +4418,9 @@ \subsection{Feature bits}\label{sec:Device Types
> > > > / Block Device / Feature bits}
> > >
> > > [something seems to have caused line wrapping in this patch]
> > >
> >
> > Hmm... I can try applying again and sending out as a v4?
>
> Please do, if we don't have any more changes on top.
>

Respun v4 at https://lists.oasis-open.org/archives/virtio-dev/202103/msg00038.html

> >
> > > >  \item[VIRTIO_BLK_F_WRITE_ZEROES (14)] Device can support write zeroes command,
> > > >       maximum write zeroes sectors size in \field{max_write_zeroes_sectors} and
> > > >       maximum write zeroes segment number in \field{max_write_zeroes_seg}.
> > > > +
> > > > +\item[VIRTIO_BLK_F_LIFETIME (15)] Device supports providing storage lifetime
> > > > +     information.
> > > >  \end{description}
> > > >
> > > >  \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types
> > > > / Block Device / Feature bits / Legacy Interface: Feature bits}
> > > > @@ -4601,14 +4604,16 @@ \subsection{Device Operation}\label{sec:Device
> > > > Types / Block Device / Device Ope
> > > >
> > > >  The type of the request is either a read (VIRTIO_BLK_T_IN), a write
> > > >  (VIRTIO_BLK_T_OUT), a discard (VIRTIO_BLK_T_DISCARD), a write zeroes
> > > > -(VIRTIO_BLK_T_WRITE_ZEROES), a flush (VIRTIO_BLK_T_FLUSH), or a get device ID
> > > > -string command (VIRTIO_BLK_T_GET_ID).
> > > > +(VIRTIO_BLK_T_WRITE_ZEROES), a flush (VIRTIO_BLK_T_FLUSH), a get device ID
> > > > +string command (VIRTIO_BLK_T_GET_ID), or a get device lifetime
> > > > +command (VIRTIO_BLK_T_GET_LIFETIME).
> > > >
> > > >  \begin{lstlisting}
> > > >  #define VIRTIO_BLK_T_IN           0
> > > >  #define VIRTIO_BLK_T_OUT          1
> > > >  #define VIRTIO_BLK_T_FLUSH        4
> > > >  #define VIRTIO_BLK_T_GET_ID       8
> > > > +#define VIRTIO_BLK_T_GET_LIFETIME 10
> > > >  #define VIRTIO_BLK_T_DISCARD      11
> > > >  #define VIRTIO_BLK_T_WRITE_ZEROES 13
> > > >  \end{lstlisting}
> > > > @@ -4648,6 +4653,23 @@ \subsection{Device Operation}\label{sec:Device
> > > > Types / Block Device / Device Ope
> > > >  \field{data}.  The device ID string is a NUL-padded ASCII string up to 20 bytes
> > > >  long.  If the string is 20 bytes long then there is no NUL terminator.
> > > >
> > > > +The \field{data} used for VIRTIO_BLK_T_GET_LIFETIME requests is populated by
> > > > +the device, and is of the form:
> > > > +
> > > > +\begin{lstlisting}
> > > > +struct virtio_blk_lifetime {
> > > > +    le16 pre_eol_info;
> > > > +    le16 device_lifetime_est_a;
> > > > +    le16 device_lifetime_est_b;
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +The device lifetime metrics \field{pre_eol_info}, \field{device_lifetime_est_a}
> > > > +and \field{device_lifetime_est_b} have the semantics described by the JEDEC
> > > > +standard No.84-B50 for the extended CSD register fields \field{PRE_EOL_INFO}
> > > > +\field{DEVICE_LIFETIME_EST_TYP_A} and \field{DEVICE_LIFETIME_EST_TYP_B}
> > > > +respectively.
> > >
> > > Do we have an explicit link to that JEDEC standard?
> > >
> >
> > I believe the specification is under a JEDEC paywall. I have access to
> > it by means of my employer, but it doesn't seem to be publicly
> > available.
>
> That's unfortunate... how much of the information in there is needed
> for someone to come up with a compliant implementation? Can we condense
> that to a sentence or two that would be unproblematic to put into the
> standard?
>
> If JEDEC is providing reasonable licensing terms, I guess it could also
> be an option to just refer to their homepage and specifying that
> standard (I think the full name is JESD84-B50 -- at least that's what
> my searching pointed to.)
>

I have added an explicit mention of finding the spec at JEDEC website.
If you think it is worthwhile to try and condense the spec in here, I
can take a pass at it.
I do slightly prefer just pointing the interested reader at JEDEC for
a couple reasons:
- in case of ambiguity, the JEDEC spec would be normative  and the
content here just "FYI" at best, so the interested implementor would
be best advised to consult the actual spec anyway;
- in a lot of common cases, the hardware will provide this data via
the host system's interface (e.g. sysfs in Linux) and need no further
processing by the virtio-blki implementation.

Up to you though. I can work with either just the link, or adding a
brief description

> >
> > > > +
> > > >  The final \field{status} byte is written by the device: either
> > > >  VIRTIO_BLK_S_OK for success, VIRTIO_BLK_S_IOERR for device or driver
> > > >  error or VIRTIO_BLK_S_UNSUPP for a request unsupported by device:
> > > > @@ -4754,6 +4776,11 @@ \subsection{Device Operation}\label{sec:Device
> > > > Types / Block Device / Device Ope
> > > >  (case~\ref{item:flush3}).  Failure to do so can cause data loss
> > > >  in case of a crash.
> > > >
> > > > +If the device is backed by eMMC or UFS persistent storage, the device SHOULD
> > > > +offer the VIRTIO_BLK_F_LIFETIME flag. The flag MUST NOT be offered if
> > > > the device
> > > > +is backed by storage for which the lifetime metrics as described in
> > > > this document
> > > > +cannot be obtained or have no useful meaning.
> > >
> > > Isn't that outside of the normative sections? If so, please make this a
> > > description without SHOULD and MUST NOT, and add them to the normative
> > > clauses.
> > >
> >
> > Hmm.. when I look at my local copy, I see this section right below the clause:
> >
> > "If the device is backed by persistent storage, the device MUST ensure that
> > stable writes are committed to it, before reporting completion of the write
> > (cases~\ref{item:flush1} and~\ref{item:flush2}) or the flush
> > (case~\ref{item:flush3}).  Failure to do so can cause data loss
> > in case of a crash."
> >
> > It looked like the right place to add this kind of clause to me, but
> > if I am wrong and you would like to see it moved, please do let me
> > know.
>
> Ah, sorry, my mistake; I must have slipped in the document, your
> addition is fine.
>
> > Maybe I really do just need to apply the patch again on a clean-slate
> > repository, if what I see locally does not align?
> >
> > > Also, are eMMC/UFS just examples (i.e. may other types of persistent
> > > storage provide these metrics as well?)
> > >
> >
> > I can't say I know of other hardware that provides identical metrics,
> > but it could be possible and maybe could be done by software
> > adaptation for other storage systems?
>
> If we can get potentially get the same metrics from other hardware,
> that sentence should probably be reworded a bit. What about:
>
> "If the device is backed by storage providing life time metrics (such
> as eMMC or UFS persistent storage), the device SHOULD offer the
> VIRTIO_BLK_F_LIFETIME flag."
>

Reworded as you suggest in v4. Thanks for the feedback

> ?
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>