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

["Michael S. Tsirkin" <mst@redhat.com>]

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.

-- 
MST

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