Re: [PATCH 14/17] nvmet: Add metadata/T10-PI support

[Max Gurtovoy <maxg@mellanox.com>]

On 4/21/2020 6:30 PM, Christoph Hellwig wrote:
>> +	/*
>> +	 * Max command capsule size is sqe + single page of in-capsule data.
>> +	 * Disable inline data for Metadata capable controllers.
>> +	 */
>>   	id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
>> -				  req->port->inline_data_size) / 16);
>> +				  req->port->inline_data_size *
>> +				  !ctrl->pi_support) / 16);
> Can we de-obsfucated this a little?
>
> 	cmd_capsule_size = sizeof(struct nvme_command);
> 	if (!ctrl->pi_support)
> 		cmd_capsule_size += req->port->inline_data_size;
> 	id->ioccsz = cpu_to_le32(cmd_capsule_size / 16);

Yes good idea.


>
>> +	if (ctrl->subsys->pi_support && ctrl->port->pi_enable) {
>> +		if (ctrl->port->pi_capable) {
>> +			ctrl->pi_support = true;
>> +			pr_info("controller %d T10-PI enabled\n", ctrl->cntlid);
>> +		} else {
>> +			ctrl->pi_support = false;
>> +			pr_warn("T10-PI is not supported on controller %d\n",
>> +				ctrl->cntlid);
>> +		}
> I think the printks are a little verbose.  Also why can we set
> ctrl->port->pi_enable if ctrl->port->pi_capable is false?  Shoudn't
> we reject that earlier?  In that case this could simply become:
>
> 	ctrl->pi_support = ctrl->subsys->pi_support && ctrl->port->pi_enable;

for that we'll need to check pi_capable during add_port process and 
disable pi_enable bit if user set it.

User should set it before enable the port (this will always succeed).

I'll make this change as well.

re the verbosity, sometimes I get many requests from users to get 
indication for some features.

We can remove this as well if needed.

>> +#ifdef CONFIG_BLK_DEV_INTEGRITY
>> +static inline u32 nvmet_rw_md_len(struct nvmet_req *req)
>> +{
>> +	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) * req->ns->ms;
>> +}
>> +
>> +static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
>> +{
>> +	return ns->md_type && ns->ms == sizeof(struct t10_pi_tuple);
>> +}
>> +#else
>> +static inline u32 nvmet_rw_md_len(struct nvmet_req *req)
>> +{
>> +	return 0;
> Do we really need a stub for nvmet_rw_md_len?  Also for nvmet_ns_has_pi
> we could probably reword it as:
>
> static inline bool nvmet_ns_has_pi(struct nvmet_ns *ns)
> {
> 	if (!IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY))
> 		return false;
> 	return ns->pi_type && ns->metadata_size == sizeof(struct t10_pi_tuple);
> }
>
> and avoid the need for a stub as well.

yup.



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