Re: [PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
On Thu, Dec 29, 2022 at 02:08:34PM +0200, Alvaro Karsz wrote: > > So due to my mistake this got pushed out to next linux release. > > Sorry :( > > No problem at all :) > > > I'd like to use this opportunity to write a new better > > interface that actually works with modern backends, > > and add it to the virtio spec. > > > > Do you think you can take up this task? > > Sure, I can do it. > So, the idea is to: > > * Drop this patch > * Implement a new, more general virtio block feature for virtio spec > * Add linux support for the new feature > > right? That's certainly an option. Let's start with point 2, and get ack from people who objected to this feature. -- MST ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
> So due to my mistake this got pushed out to next linux release. > Sorry :( No problem at all :) > I'd like to use this opportunity to write a new better > interface that actually works with modern backends, > and add it to the virtio spec. > > Do you think you can take up this task? Sure, I can do it. So, the idea is to: * Drop this patch * Implement a new, more general virtio block feature for virtio spec * Add linux support for the new feature right? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
On Mon, Dec 19, 2022 at 02:21:55PM +0200, Alvaro Karsz wrote:
> Implement the VIRTIO_BLK_F_LIFETIME feature for Virtio block devices.
>
> This commit introduces a new ioctl command, VIRTIO_BLK_IOCTL_GET_LIFETIME.
>
> VIRTIO_BLK_IOCTL_GET_LIFETIME ioctl asks for the block device to provide
> lifetime information by sending a VIRTIO_BLK_T_GET_LIFETIME command to
> the device.
>
> lifetime information fields:
>
> - pre_eol_info: specifies the percentage of reserved blocks that are
> consumed.
> optional values following virtio spec:
> *) 0 - undefined.
> *) 1 - normal, < 80% of reserved blocks are consumed.
> *) 2 - warning, 80% of reserved blocks are consumed.
> *) 3 - urgent, 90% of reserved blocks are consumed.
>
> - device_lifetime_est_typ_a: this field refers to wear of SLC cells and
>is provided in increments of 10used, and so
>on, thru to 11 meaning estimated lifetime
>exceeded. All values above 11 are reserved.
>
> - device_lifetime_est_typ_b: this field refers to wear of MLC cells and is
>provided with the same semantics as
>device_lifetime_est_typ_a.
>
> The data received from the device will be sent as is to the user.
> No data check/decode is done by virtblk.
>
> Signed-off-by: Alvaro Karsz
> Reviewed-by: Stefan Hajnoczi
So due to my mistake this got pushed out to next linux release.
Sorry :(
I'd like to use this opportunity to write a new better
interface that actually works with modern backends,
and add it to the virtio spec.
Do you think you can take up this task?
> --
> v2:
> - Remove (void *) casting.
> - Fix comments format in virtio_blk.h.
> - Set ioprio value for legacy devices for REQ_OP_DRV_IN
> operations.
>
> v3:
> - Initialize struct virtio_blk_lifetime to 0 instead of memset.
> - Rename ioctl from VBLK_LIFETIME to VBLK_GET_LIFETIME.
> - Return EOPNOTSUPP insted of ENOTTY if ioctl is not supported.
> - Check if process is CAP_SYS_ADMIN capable in lifetime ioctl.
> - Check if vdev is not NULL before accessing it in lifetime ioctl.
>
> v4:
> - Create a dedicated virtio_blk_ioctl.h header for the ioctl command
> and add this file to MAINTAINERS.
> - Rename the ioctl to VIRTIO_BLK_IOCTL_GET_LIFETIME.
> - Document in virtio_blk_ioctl.h which backend device can supply
> this lifetime information.
>
> v5:
> - Rebase patch on top of mst tree.
> - Add a comment explaining the capable(CAP_SYS_ADMIN) part.
> --
> ---
> MAINTAINERS | 1 +
> drivers/block/virtio_blk.c| 102 +-
> include/uapi/linux/virtio_blk.h | 28 +++
> include/uapi/linux/virtio_blk_ioctl.h | 44 +++
> 4 files changed, 173 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/virtio_blk_ioctl.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4aee796cc..3250f7753 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21724,6 +21724,7 @@ F:drivers/block/virtio_blk.c
> F: drivers/scsi/virtio_scsi.c
> F: drivers/vhost/scsi.c
> F: include/uapi/linux/virtio_blk.h
> +F: include/uapi/linux/virtio_blk_ioctl.h
> F: include/uapi/linux/virtio_scsi.h
>
> VIRTIO CONSOLE DRIVER
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 3c9dae237..1e03bfa9f 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -112,6 +113,18 @@ struct virtblk_req {
> struct scatterlist sg[];
> };
>
> +static inline int virtblk_ioctl_result(struct virtblk_req *vbr)
> +{
> + switch (vbr->status) {
> + case VIRTIO_BLK_S_OK:
> + return 0;
> + case VIRTIO_BLK_S_UNSUPP:
> + return -EOPNOTSUPP;
> + default:
> + return -EIO;
> + }
> +}
> +
> static inline blk_status_t virtblk_result(u8 status)
> {
> switch (status) {
> @@ -840,6 +853,90 @@ static int virtblk_getgeo(struct block_device *bd,
> struct hd_geometry *geo)
> return ret;
> }
>
> +/* Get lifetime information from device */
> +static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
> +{
> + struct request_queue *q = vblk->disk->queue;
> + struct request *req = NULL;
> + struct virtblk_req *vbr;
> + struct virtio_blk_lifetime lifetime = {};
> + int ret;
> +
> + /* It's not clear whether exposing lifetime info to userspace
> + * has any security implications but out of abundance of caution
> + * we limit it to privileged users.
> + */
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + /* The virtio_blk_lifetime struct fields
[PATCH v5] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support
Implement the VIRTIO_BLK_F_LIFETIME feature for Virtio block devices.
This commit introduces a new ioctl command, VIRTIO_BLK_IOCTL_GET_LIFETIME.
VIRTIO_BLK_IOCTL_GET_LIFETIME ioctl asks for the block device to provide
lifetime information by sending a VIRTIO_BLK_T_GET_LIFETIME command to
the device.
lifetime information fields:
- pre_eol_info: specifies the percentage of reserved blocks that are
consumed.
optional values following virtio spec:
*) 0 - undefined.
*) 1 - normal, < 80% of reserved blocks are consumed.
*) 2 - warning, 80% of reserved blocks are consumed.
*) 3 - urgent, 90% of reserved blocks are consumed.
- device_lifetime_est_typ_a: this field refers to wear of SLC cells and
is provided in increments of 10used, and so
on, thru to 11 meaning estimated lifetime
exceeded. All values above 11 are reserved.
- device_lifetime_est_typ_b: this field refers to wear of MLC cells and is
provided with the same semantics as
device_lifetime_est_typ_a.
The data received from the device will be sent as is to the user.
No data check/decode is done by virtblk.
Signed-off-by: Alvaro Karsz
Reviewed-by: Stefan Hajnoczi
--
v2:
- Remove (void *) casting.
- Fix comments format in virtio_blk.h.
- Set ioprio value for legacy devices for REQ_OP_DRV_IN
operations.
v3:
- Initialize struct virtio_blk_lifetime to 0 instead of memset.
- Rename ioctl from VBLK_LIFETIME to VBLK_GET_LIFETIME.
- Return EOPNOTSUPP insted of ENOTTY if ioctl is not supported.
- Check if process is CAP_SYS_ADMIN capable in lifetime ioctl.
- Check if vdev is not NULL before accessing it in lifetime ioctl.
v4:
- Create a dedicated virtio_blk_ioctl.h header for the ioctl command
and add this file to MAINTAINERS.
- Rename the ioctl to VIRTIO_BLK_IOCTL_GET_LIFETIME.
- Document in virtio_blk_ioctl.h which backend device can supply
this lifetime information.
v5:
- Rebase patch on top of mst tree.
- Add a comment explaining the capable(CAP_SYS_ADMIN) part.
--
---
MAINTAINERS | 1 +
drivers/block/virtio_blk.c| 102 +-
include/uapi/linux/virtio_blk.h | 28 +++
include/uapi/linux/virtio_blk_ioctl.h | 44 +++
4 files changed, 173 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/virtio_blk_ioctl.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 4aee796cc..3250f7753 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21724,6 +21724,7 @@ F: drivers/block/virtio_blk.c
F: drivers/scsi/virtio_scsi.c
F: drivers/vhost/scsi.c
F: include/uapi/linux/virtio_blk.h
+F: include/uapi/linux/virtio_blk_ioctl.h
F: include/uapi/linux/virtio_scsi.h
VIRTIO CONSOLE DRIVER
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 3c9dae237..1e03bfa9f 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -112,6 +113,18 @@ struct virtblk_req {
struct scatterlist sg[];
};
+static inline int virtblk_ioctl_result(struct virtblk_req *vbr)
+{
+ switch (vbr->status) {
+ case VIRTIO_BLK_S_OK:
+ return 0;
+ case VIRTIO_BLK_S_UNSUPP:
+ return -EOPNOTSUPP;
+ default:
+ return -EIO;
+ }
+}
+
static inline blk_status_t virtblk_result(u8 status)
{
switch (status) {
@@ -840,6 +853,90 @@ static int virtblk_getgeo(struct block_device *bd, struct
hd_geometry *geo)
return ret;
}
+/* Get lifetime information from device */
+static int virtblk_ioctl_lifetime(struct virtio_blk *vblk, unsigned long arg)
+{
+ struct request_queue *q = vblk->disk->queue;
+ struct request *req = NULL;
+ struct virtblk_req *vbr;
+ struct virtio_blk_lifetime lifetime = {};
+ int ret;
+
+ /* It's not clear whether exposing lifetime info to userspace
+* has any security implications but out of abundance of caution
+* we limit it to privileged users.
+*/
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ /* The virtio_blk_lifetime struct fields follow virtio spec.
+* There is no check/decode on values received from the device.
+* The data is sent as is to the user.
+*/
+
+ /* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
+* feature is negotiated.
+*/
+ if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
+ return -EOPNOTSUPP;
+
+ req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
+ if (IS_ERR(req))
+
