Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Nov 24, 2022 at 01:42:37AM -0500, Michael S. Tsirkin wrote:
> On Wed, Nov 23, 2022 at 10:41:46PM +0000, Chaitanya Kulkarni wrote:
> > 
> > > +/* 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;
> > > +
> > > +	/* 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.
> > > +	 */
> > 
> > Odd comment style, why not :-
> > 
> > 	/*
> > 	 * 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.
> > 	 */
> 
> most of virtio blk is like this. I don't really care much.
> 
> > > +
> > > +	/* This ioctl is allowed only if VIRTIO_BLK_F_LIFETIME
> > > +	 * feature is negotiated.
> > > +	 */
> > 
> > above comment is redundant to the below code as following code is
> > self explanatory.,..
> 
> i find it helpful personally - this point is important enough to
> stress at cost of some duplication.
> 
> > > +	if (!virtio_has_feature(vblk->vdev, VIRTIO_BLK_F_LIFETIME))
> > > +		return -ENOTTY;
> > > +
> > > +	memset(&lifetime, 0, sizeof(lifetime));
> > > +
> > > +	req = blk_mq_alloc_request(q, REQ_OP_DRV_IN, 0);
> > > +	if (IS_ERR(req))
> > > +		return PTR_ERR(req);
> > > +
> > > +	/* Write the correct type */
> > 
> > same here for above comment...
> 
> it's pretty harmless though
> 
> > > +	vbr = blk_mq_rq_to_pdu(req);
> > > +	vbr->out_hdr.type = cpu_to_virtio32(vblk->vdev, VIRTIO_BLK_T_GET_LIFETIME);
> > > +
> > > +	ret = blk_rq_map_kern(q, req, &lifetime, sizeof(lifetime), GFP_KERNEL);
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	blk_execute_rq(req, false);
> > > +
> > > +	ret = virtblk_ioctl_result(blk_mq_rq_to_pdu(req));
> > > +	if (ret)
> > > +		goto out;
> > > +
> > > +	/* Pass the data to the user */
> > > +	if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime))) {
> > > +		ret = -EFAULT;
> > > +		goto out;
> > 
> > there nothing here to "goto out" following is sufficient I guess :-
> > 
> > if (copy_to_user((void __user *)arg, &lifetime, sizeof(lifetime)))
> > 	ret = -EFAULT;
> 
> error handling with goto seems cleaner, easier to add code down
> the road.
> 
> > > +	}
> > > +
> > > +out:
> > > +	blk_mq_free_request(req);
> > > +	return ret;
> > > +}
> > > +
> > 
> > [...]
> > 
> >    };
> > >   
> > > +/* Get lifetime information struct for each request */
> > > +struct virtio_blk_lifetime {
> > > +	/*
> > > +	 * 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
> > > +	 */
> > > +	__le16 pre_eol_info;
> > > +	/*
> > > +	 * 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
> > > +	 */
> > > +	__le16 device_lifetime_est_typ_a;
> > > +	/*
> > > +	 * this field refers to wear of MLC cells and is provided with the same semantics as
> > > +	 * device_lifetime_est_typ_a
> > > +	 */
> > > +	__le16 device_lifetime_est_typ_b;
> > > +};
> > > +
> > 
> > are you sure there won't be any new members ever be added in future ?
> > to make it is futuresafe I'd add padding to this structure and keep
> > the reserve bytes to the meaningful size...
> > 
> > -ck
> 
> 
> virtio has feature bits for this kind of thing, we don't do padding.

The struct defined in the VIRTIO spec is being reused in the ioctl
userspace ABI. While feature bits are used by VIRTIO, padding is common
in the Linux UAPI.

Adding padding would require decoupling the VIRTIO spec struct from the
ioctl struct. That's probably a good idea because I noticed endianness
is left to userspace due to the __le fields in the VIRTIO spec struct
and that's likely to cause userspace bugs/confusion.

I suggest defining a separate UAPI struct for this ioctl.

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