Re: [PATCH] virtio_blk: Add support for lifetime feature

Re: [PATCH] virtio_blk: Add support for lifetime feature

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 4359 bytes --]

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 <egranata@google.com>
> ---
>  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.

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

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_blk: Add support for lifetime feature

[Christoph Hellwig]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_blk: Add support for lifetime feature

[Michael S. Tsirkin]

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?
linux-block@vger.kernel.org ?
Thanks,

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_blk: Add support for lifetime feature

[Christoph Hellwig]

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?
> linux-block@vger.kernel.org ?

At least for linux, yes.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_blk: Add support for lifetime feature

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 750 bytes --]

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

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

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_blk: Add support for lifetime feature

[Christoph Hellwig]

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.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization