Re: [PATCH 13/15] nvmet: add metadata/T10-PI support

[Christoph Hellwig <hch@lst.de>]

> +static ssize_t nvmet_subsys_attr_pi_enable_store(struct config_item *item,
> +						 const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +	bool pi_enable;
> +
> +	if (strtobool(page, &pi_enable))
> +		return -EINVAL;
> +
> +	down_write(&nvmet_config_sem);
> +	subsys->pi_support = pi_enable;
> +	up_write(&nvmet_config_sem);

No need to take a lock for setting a single scalar value.

> +	/*
> +	 * If the user requested PI support and the transport isn't pi capable,
> +	 * don't enable the port.
> +	 */
> +	if (port->pi_enable && !ops->metadata_support) {
> +		ret = -EINVAL;

In this case it might be worth to print a messag to help with debugging.

> +	if (ctrl->port->pi_enable)
> +		ctrl->pi_support = ctrl->subsys->pi_support;

I find this a little weird to read due to the mix of styles.  Either:

	if (ctrl->port->pi_enable && ctrl->subsys->pi_support)
		ctrl->pi_support = true;

or

	ctrl->pi_support = ctrl->port->pi_enable && ctrl->subsys->pi_support;

seem ok to be, with a slight preference for the first one.

> +static inline u32 nvmet_rw_md_len(struct nvmet_req *req)

Please spell out metadata instead of md.

> +{
> +	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) *
> +			req->ns->metadata_size;
> +}

Also this could probably use another IS_ENABLED to optimize out
the code if CONFIG_BLK_DEV_INTEGRITY is not enabled.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme