The VirtIO TC has adopted a new feature in virtio-blk enabling
discovery of lifetime information.
This commit adds support for the VIRTIO_BLK_T_LIFETIME command
to the virtio_blk driver, and adds two new attributes to the
sysfs entry for virtio_blk:
* pre_eol_info
* life_time
which are defined in the same manner as the files of the same name
for the eMMC driver, in line with the VirtIO specification.
Signed-off-by: Enrico Granata <[email protected]>
---
drivers/block/virtio_blk.c | 76 ++++++++++++++++++++++++++++++++-
include/uapi/linux/virtio_blk.h | 11 +++++
2 files changed, 86 insertions(+), 1 deletion(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b9fa3ef5b57c..1fc0ec000b4f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -246,7 +246,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
unmap = !(req->cmd_flags & REQ_NOUNMAP);
break;
case REQ_OP_DRV_IN:
- type = VIRTIO_BLK_T_GET_ID;
+ type = vbr->out_hdr.type;
break;
default:
WARN_ON_ONCE(1);
@@ -310,11 +310,14 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
struct virtio_blk *vblk = disk->private_data;
struct request_queue *q = vblk->disk->queue;
struct request *req;
+ struct virtblk_req *vbreq;
int err;
req = blk_get_request(q, REQ_OP_DRV_IN, 0);
if (IS_ERR(req))
return PTR_ERR(req);
+ vbreq = blk_mq_rq_to_pdu(req);
+ vbreq->out_hdr.type = VIRTIO_BLK_T_GET_ID;
err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
if (err)
@@ -327,6 +330,34 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
return err;
}
+static int virtblk_get_lifetime(struct gendisk *disk, struct virtio_blk_lifetime *lifetime)
+{
+ struct virtio_blk *vblk = disk->private_data;
+ struct request_queue *q = vblk->disk->queue;
+ struct request *req;
+ struct virtblk_req *vbreq;
+ int err;
+
+ if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
+ return -EOPNOTSUPP;
+
+ req = blk_get_request(q, REQ_OP_DRV_IN, 0);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+ vbreq = blk_mq_rq_to_pdu(req);
+ vbreq->out_hdr.type = VIRTIO_BLK_T_GET_LIFETIME;
+
+ err = blk_rq_map_kern(q, req, lifetime, sizeof(*lifetime), GFP_KERNEL);
+ if (err)
+ goto out;
+
+ blk_execute_rq(vblk->disk, req, false);
+ err = blk_status_to_errno(virtblk_result(blk_mq_rq_to_pdu(req)));
+out:
+ blk_put_request(req);
+ return err;
+}
+
static void virtblk_get(struct virtio_blk *vblk)
{
refcount_inc(&vblk->refs);
@@ -435,6 +466,46 @@ static ssize_t serial_show(struct device *dev,
static DEVICE_ATTR_RO(serial);
+static ssize_t pre_eol_info_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+ struct virtio_blk_lifetime lft;
+ int err;
+
+ /* sysfs gives us a PAGE_SIZE buffer */
+ BUILD_BUG_ON(sizeof(lft) >= PAGE_SIZE);
+
+ err = virtblk_get_lifetime(disk, &lft);
+ if (err)
+ return 0;
+
+ return sprintf(buf, "0x%02x\n", le16_to_cpu(lft.pre_eol_info));
+}
+
+static DEVICE_ATTR_RO(pre_eol_info);
+
+static ssize_t life_time_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+ struct virtio_blk_lifetime lft;
+ int err;
+
+ /* sysfs gives us a PAGE_SIZE buffer */
+ BUILD_BUG_ON(sizeof(lft) >= PAGE_SIZE);
+
+ err = virtblk_get_lifetime(disk, &lft);
+ if (err)
+ return 0;
+
+ return sprintf(buf, "0x%02x 0x%02x\n",
+ le16_to_cpu(lft.device_life_time_est_typ_a),
+ le16_to_cpu(lft.device_life_time_est_typ_b));
+}
+
+static DEVICE_ATTR_RO(life_time);
+
/* The queue's logical block size must be set before calling this */
static void virtblk_update_capacity(struct virtio_blk *vblk, bool resize)
{
@@ -638,6 +709,8 @@ static DEVICE_ATTR_RW(cache_type);
static struct attribute *virtblk_attrs[] = {
&dev_attr_serial.attr,
+ &dev_attr_pre_eol_info.attr,
+ &dev_attr_life_time.attr,
&dev_attr_cache_type.attr,
NULL,
};
@@ -984,6 +1057,7 @@ static unsigned int features[] = {
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
+ VIRTIO_BLK_F_LIFETIME,
};
static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index d888f013d9ff..bbd3978b9d08 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -40,6 +40,7 @@
#define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
#define VIRTIO_BLK_F_DISCARD 13 /* DISCARD is supported */
#define VIRTIO_BLK_F_WRITE_ZEROES 14 /* WRITE ZEROES is supported */
+#define VIRTIO_BLK_F_LIFETIME 15 /* LIFETIME is supported */
/* Legacy feature bits */
#ifndef VIRTIO_BLK_NO_LEGACY
@@ -149,6 +150,9 @@ struct virtio_blk_config {
/* Get device ID command */
#define VIRTIO_BLK_T_GET_ID 8
+/* Get device lifetime command */
+#define VIRTIO_BLK_T_GET_LIFETIME 10
+
/* Discard command */
#define VIRTIO_BLK_T_DISCARD 11
@@ -196,6 +200,13 @@ struct virtio_scsi_inhdr {
};
#endif /* !VIRTIO_BLK_NO_LEGACY */
+/* Lifetime information for virtio_blk device */
+struct virtio_blk_lifetime {
+ __le16 pre_eol_info;
+ __le16 device_life_time_est_typ_a;
+ __le16 device_life_time_est_typ_b;
+};
+
/* And this is the final byte of the write scatter-gather list. */
#define VIRTIO_BLK_S_OK 0
#define VIRTIO_BLK_S_IOERR 1
--
2.31.0.291.g576ba9dcdaf-goog
On Tue, Mar 30, 2021 at 11:16:02PM +0000, Enrico Granata wrote:
> The VirtIO TC has adopted a new feature in virtio-blk enabling
> discovery of lifetime information.
>
> This commit adds support for the VIRTIO_BLK_T_LIFETIME command
> to the virtio_blk driver, and adds two new attributes to the
> sysfs entry for virtio_blk:
> * pre_eol_info
> * life_time
>
> which are defined in the same manner as the files of the same name
> for the eMMC driver, in line with the VirtIO specification.
>
> Signed-off-by: Enrico Granata <[email protected]>
> ---
> drivers/block/virtio_blk.c | 76 ++++++++++++++++++++++++++++++++-
> include/uapi/linux/virtio_blk.h | 11 +++++
> 2 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index b9fa3ef5b57c..1fc0ec000b4f 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -246,7 +246,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
> unmap = !(req->cmd_flags & REQ_NOUNMAP);
> break;
> case REQ_OP_DRV_IN:
> - type = VIRTIO_BLK_T_GET_ID;
> + type = vbr->out_hdr.type;
This patch changes the endianness of vbr->out_hdr.type from
virtio-endian to cpu endian before virtio_queue_rq. That is error-prone
because someone skimming through the code will see some accesses with
cpu_to_virtio32() and others without it. They would have to audit the
code carefully to understand what is going on.
The following is cleaner:
case REQ_OP_DRV_IN:
break; /* type already set for custom requests */
...
if (req_op(req) != REQ_OP_DRV_IN)
vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, type);
Now vbr->out_hdr.type is virtio-endian everywhere. If we need to support
REQ_OP_DRV_OUT in the future it can use the same approach.
virtblk_get_id() and virtblk_get_lifetime() would be updated like this:
vbreq->out_hdr.type = cpu_to_virtio32(VIRTIO_BLK_T_GET_*);
> break;
> default:
> WARN_ON_ONCE(1);
> @@ -310,11 +310,14 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
> struct virtio_blk *vblk = disk->private_data;
> struct request_queue *q = vblk->disk->queue;
> struct request *req;
> + struct virtblk_req *vbreq;
It's called vbr elsewhere in the driver. It would be nice to keep naming
consistent.
> int err;
>
> req = blk_get_request(q, REQ_OP_DRV_IN, 0);
> if (IS_ERR(req))
> return PTR_ERR(req);
> + vbreq = blk_mq_rq_to_pdu(req);
> + vbreq->out_hdr.type = VIRTIO_BLK_T_GET_ID;
>
> err = blk_rq_map_kern(q, req, id_str, VIRTIO_BLK_ID_BYTES, GFP_KERNEL);
> if (err)
> @@ -327,6 +330,34 @@ static int virtblk_get_id(struct gendisk *disk, char *id_str)
> return err;
> }
>
> +static int virtblk_get_lifetime(struct gendisk *disk, struct virtio_blk_lifetime *lifetime)
> +{
> + struct virtio_blk *vblk = disk->private_data;
> + struct request_queue *q = vblk->disk->queue;
> + struct request *req;
> + struct virtblk_req *vbreq;
> + int err;
> +
> + if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> + return -EOPNOTSUPP;
> +
> + req = blk_get_request(q, REQ_OP_DRV_IN, 0);
> + if (IS_ERR(req))
> + return PTR_ERR(req);
> + vbreq = blk_mq_rq_to_pdu(req);
> + vbreq->out_hdr.type = VIRTIO_BLK_T_GET_LIFETIME;
> +
> + err = blk_rq_map_kern(q, req, lifetime, sizeof(*lifetime), GFP_KERNEL);
> + if (err)
> + goto out;
> +
> + blk_execute_rq(vblk->disk, req, false);
> + err = blk_status_to_errno(virtblk_result(blk_mq_rq_to_pdu(req)));
> +out:
> + blk_put_request(req);
> + return err;
> +}
> +
> static void virtblk_get(struct virtio_blk *vblk)
> {
> refcount_inc(&vblk->refs);
> @@ -435,6 +466,46 @@ static ssize_t serial_show(struct device *dev,
>
> static DEVICE_ATTR_RO(serial);
>
> +static ssize_t pre_eol_info_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct gendisk *disk = dev_to_disk(dev);
> + struct virtio_blk_lifetime lft;
> + int err;
> +
> + /* sysfs gives us a PAGE_SIZE buffer */
> + BUILD_BUG_ON(sizeof(lft) >= PAGE_SIZE);
Why is this necessary? In serial_show() it protects against a buffer
overflow. That's not the case here since sprintf() is used to write to
buf and the size of lft doesn't really matter.
A note to the virtio committee: eMMC is the worst of all the currently
active storage standards by a large margin. It defines very strange
ad-hoc interfaces that expose very specific internals and often provides
very poor abstractions. It would be great it you could reach out to the
wider storage community before taking bad ideas from the eMMC standard
and putting it into virtio.
On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote:
> A note to the virtio committee: eMMC is the worst of all the currently
> active storage standards by a large margin. It defines very strange
> ad-hoc interfaces that expose very specific internals and often provides
> very poor abstractions.
Are we talking about the lifetime feature here? UFS has it too right?
It's not too late to
change things if necessary... it would be great if you could provide
more of the feedback on this on the TC mailing list.
> It would be great it you could reach out to the
> wider storage community before taking bad ideas from the eMMC standard
> and putting it into virtio.
Noted. It would be great if we had more representation from the storage
community ... meanwhile what would a good forum for this be?
[email protected] ?
Thanks,
--
MST
On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote:
> A note to the virtio committee: eMMC is the worst of all the currently
> active storage standards by a large margin. It defines very strange
> ad-hoc interfaces that expose very specific internals and often provides
> very poor abstractions. It would be great it you could reach out to the
> wider storage community before taking bad ideas from the eMMC standard
> and putting it into virtio.
As Michael mentioned, there is still time to change the virtio-blk spec
since this feature hasn't been released yet.
Why exactly is exposing eMMC-style lifetime information problematic?
Can you and Enrico discuss the use case to figure out an alternative
interface?
Thanks,
Stefan
First and foremost thanks for the feedback on the code. I will send
out an up-to-date patch with those comments addressed ASAP
As for the broader issue, I am definitely happy to incorporate any
feedback and work to improve the spec, but looking at embedded storage
devices both eMMC and UFS seem to me to be exposing the attributes
that are included in the virtio-blk lifetime feature, and the same
data is also used by the Android Open Source Project for Health HAL.
It does seem like this format has a lot of practical adoption in the
wider industry and that makes it fairly trivial to implement in a lot
of common embedded systems. Having this immediate path to adoption in
a variety of scenarios seems to me in and of itself valuable.
Thanks,
- Enrico
On Wed, Apr 14, 2021 at 2:44 AM Stefan Hajnoczi <[email protected]> wrote:
>
> On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote:
> > A note to the virtio committee: eMMC is the worst of all the currently
> > active storage standards by a large margin. It defines very strange
> > ad-hoc interfaces that expose very specific internals and often provides
> > very poor abstractions. It would be great it you could reach out to the
> > wider storage community before taking bad ideas from the eMMC standard
> > and putting it into virtio.
>
> As Michael mentioned, there is still time to change the virtio-blk spec
> since this feature hasn't been released yet.
>
> Why exactly is exposing eMMC-style lifetime information problematic?
>
> Can you and Enrico discuss the use case to figure out an alternative
> interface?
>
> Thanks,
> Stefan
On Mon, Apr 12, 2021 at 08:00:24AM -0400, Michael S. Tsirkin wrote:
> On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote:
> > A note to the virtio committee: eMMC is the worst of all the currently
> > active storage standards by a large margin. It defines very strange
> > ad-hoc interfaces that expose very specific internals and often provides
> > very poor abstractions.
>
> Are we talking about the lifetime feature here? UFS has it too right?
Ok, the wide margin above ignores UFS, which has a lot of the same
issues as EMMC, just a little less cruft.
> It's not too late to
> change things if necessary... it would be great if you could provide
> more of the feedback on this on the TC mailing list.
I think the big main issue here is that it just forwards an awkwardly
specific concept through virtio. If we want a virtio feature it really
needs to stand a lone and be properly documented without referring to
external specifications that are not openly available.
> > It would be great it you could reach out to the
> > wider storage community before taking bad ideas from the eMMC standard
> > and putting it into virtio.
>
> Noted. It would be great if we had more representation from the storage
> community ... meanwhile what would a good forum for this be?
> [email protected] ?
At least for linux, yes.
On Wed, Apr 14, 2021 at 09:44:35AM +0100, Stefan Hajnoczi wrote:
> On Mon, Apr 12, 2021 at 10:42:17AM +0100, Christoph Hellwig wrote:
> > A note to the virtio committee: eMMC is the worst of all the currently
> > active storage standards by a large margin. It defines very strange
> > ad-hoc interfaces that expose very specific internals and often provides
> > very poor abstractions. It would be great it you could reach out to the
> > wider storage community before taking bad ideas from the eMMC standard
> > and putting it into virtio.
>
> As Michael mentioned, there is still time to change the virtio-blk spec
> since this feature hasn't been released yet.
>
> Why exactly is exposing eMMC-style lifetime information problematic?
>
> Can you and Enrico discuss the use case to figure out an alternative
> interface?
Mostly because it exposed a very awkward encoding that is not actually
documented in any publically available spec.
If you want to incorporate a more open definition doing something
like the NVMe 'Endurance Estimate' and 'Percentage Used' fields. But
the most important thing is to fully document the semantics in the
virtio document instead of refercing a closed standard.