Re: [RFC 3/3] nvme : Add ioctl to query nvme attributes

[Joel Granados <j.granados@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 4832 bytes --]

On Thu, Oct 27, 2022 at 10:55:38AM -0600, Keith Busch wrote:
> On Thu, Oct 27, 2022 at 05:57:24PM +0200, Joel Granados wrote:
> > +{
> > +	int ret;
> > +	struct nvme_id_ctrl *id_ctrl;
> > +	struct nvme_get_attr nvme_get_attr = {0};
> > +	struct nvme_id_ctrl_nvm *id_ctrl_nvm;
> > +	__u32 usize;
> > +
> > +	BUILD_BUG_ON(sizeof(struct nvme_get_attr) < NVME_IOCTL_GET_ATTR_V0SZ);
> > +	BUILD_BUG_ON(sizeof(struct nvme_get_attr) != NVME_IOCTL_GET_ATTR_CURSZ);
> > +
> > +	if (copy_from_user(&nvme_get_attr, arg, 2 * sizeof(__u32)))
> > +		return -EFAULT;
> > +
> > +	if (nvme_get_attr.flags != 0)
> > +		return -EINVAL;
> > +
> > +	switch (nvme_get_attr.argsz) {
> > +	case NVME_IOCTL_GET_ATTR_V0SZ:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	usize = nvme_get_attr.argsz;
> > +
> > +	ret = nvme_identify_ctrl(ctrl, &id_ctrl);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = nvme_identify_cs_ctrl(ctrl, &id_ctrl_nvm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	nvme_get_attr.argsz = NVME_IOCTL_GET_ATTR_CURSZ;
> > +	nvme_get_attr.mpsmin = NVME_CAP_MPSMIN(ctrl->cap);
> > +	nvme_get_attr.vsl = id_ctrl_nvm->vsl;
> > +	nvme_get_attr.wzsl = id_ctrl_nvm->wzsl;
> > +	nvme_get_attr.wusl = id_ctrl_nvm->wusl;
> > +	nvme_get_attr.dmrl = id_ctrl_nvm->dmrl;
> > +	nvme_get_attr.dmsl = id_ctrl_nvm->dmsl;
> > +	nvme_get_attr.dmrsl = id_ctrl_nvm->dmrsl;
> > +	nvme_get_attr.oncs = id_ctrl->oncs;
> > +	nvme_get_attr.mdts = id_ctrl->mdts;
> 
> You already have the 'struct nvme_ctrl' that saves nearly all these
> values. We shouldn't need to send new IO when you can just reference the
> cached values instead.

There are three types of variables here:
1. The variables that are adjusted from what the device actually
   returns. Like wzsl that is in max_zeroes_sectors but is already
   adjusted with MSPMIN. Here we can de-adjust them if we want to save
   an IO call
2. The variables that are not adjusted and are the same in struct
   nvme_ctrl in the device. Here we can just use the ctrl member.
3. And the variables that are not in struct nvme_ctrl. Here, we have not
   option but to make an IO.


> > +	nvme_get_attr.vsl = id_ctrl_nvm->vsl;
Not in ctrl (type 3)

> > +	nvme_get_attr.wzsl = id_ctrl_nvm->wzsl;
Exported as ctrl->max_zeroes_sector and adjusted with MPSMIN (type 1)

> > +	nvme_get_attr.wusl = id_ctrl_nvm->wusl;
Not in ctrl (type 3)

> > +	nvme_get_attr.dmrl = id_ctrl_nvm->dmrl;
In ctrl->max_discard_segments (type 2)

> > +	nvme_get_attr.dmsl = id_ctrl_nvm->dmsl;
Not in ctrl (type 3)

> > +	nvme_get_attr.dmrsl = id_ctrl_nvm->dmrsl;
In ctrl->dmrsl (type 2)

> > +	nvme_get_attr.oncs = id_ctrl->oncs;
In ctrl->oncs (type 2)

> > +	nvme_get_attr.mdts = id_ctrl->mdts;
Exported as ctrl->max_hw_sectors and adjusted with MPSMIN (type1)


So I see 3 members in struct id_ctrl_nvm that are not contained in
struct nvme_ctrl, therefore we need an IO to populate these.

To save an IO for the 2 last members (oncs and mdts), then I'll
de-adjust mdts and leave oncs as it is in nvme_ctrl.


>
> 
> > +struct nvme_get_attr {
> > +	__u32	argsz;
> > +	__u32	flags;
> > +
> > +	/*
> > +	 * Memory Page Size MINimum : The host should not configure a page size that
> > +	 * is larger than (2 ^ (12 + mpsmin)). Comes from [3]
> > +	 */
> > +	__u32	mpsmin;
> > +
> > +	/*
> > +	 * Verify Size Limit : Recommended or supported data size for a verify
> > +	 * command. From [2].
> > +	 */
> > +	__u8	vsl;
> > +
> > +	/*
> > +	 * Write Zeroes Size Limit : Recommended or supported data size for a
> > +	 * zeroes command. From [2].
> > +	 */
> > +	__u8	wzsl;
> > +
> > +	/*
> > +	 * Write Uncorrected Size Limit : Recommended or supported data size for
> > +	 * an uncorrected command. From [2].
> > +	 */
> > +	__u8	wusl;
> > +
> > +	/*
> > +	 * Dataset Management Ranges Limit : Recommended or supported maximum
> > +	 * number of logical block ranges for the Dataset Management Command.
> > +	 * From [2].
> > +	 */
> > +	__u8	dmrl;
> > +
> > +	/*
> > +	 * Dataset Management Size Limit : Recommended or supported maximum of
> > +	 * total number of logical blocks for a Dataset Management Command.
> > +	 * From [2].
> > +	 */
> > +	__le64	dmsl;
> > +
> > +	/*
> > +	 * Dataset Management Range Size Limit : Recommended or supported maximum
> > +	 * number of logical blocks in a range of a Dataset Management Command.
> > +	 * From [2].
> > +	 */
> > +	__le32	dmrsl;
> > +
> > +	/*
> > +	 * Optional NVM Command Support. Is needed to make sense of attributes
> > +	 * like vsl, wzsl, wusl... Comes from [1].
> > +	 */
> > +	__le16	oncs;
> 
> Don't use the little-endian format for values that are not going over
> the wire.

oops. That is mistake. thx

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