Re: [PATCH 14/16] nvmet: add metadata/T10-PI support

[Max Gurtovoy <maxg@mellanox.com>]

On 5/13/2020 10:51 PM, James Smart wrote:
>
>
> On 5/4/2020 8:57 AM, Max Gurtovoy wrote:
>> From: Israel Rukshin <israelr@mellanox.com>
>>
>> Expose the namespace metadata format when PI is enabled. The user needs
>> to enable the capability per subsystem and per port. The other metadata
>> properties are taken from the namespace/bdev.
>>
>> Usage example:
>> echo 1 > /config/nvmet/subsystems/${NAME}/attr_pi_enable
>> echo 1 > /config/nvmet/ports/${PORT_NUM}/param_pi_enable
>>
>> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
>> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
>> ---
>>   drivers/nvme/target/admin-cmd.c   | 26 +++++++++++++++---
>>   drivers/nvme/target/configfs.c    | 58 
>> +++++++++++++++++++++++++++++++++++++++
>>   drivers/nvme/target/core.c        | 21 +++++++++++---
>>   drivers/nvme/target/fabrics-cmd.c |  7 +++--
>>   drivers/nvme/target/nvmet.h       | 19 +++++++++++++
>>   5 files changed, 121 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/nvme/target/admin-cmd.c 
>> b/drivers/nvme/target/admin-cmd.c
>> index 905aba8..9c8a00d 100644
>> --- a/drivers/nvme/target/admin-cmd.c
>> +++ b/drivers/nvme/target/admin-cmd.c
>> @@ -341,6 +341,7 @@ static void nvmet_execute_identify_ctrl(struct 
>> nvmet_req *req)
>>   {
>>       struct nvmet_ctrl *ctrl = req->sq->ctrl;
>>       struct nvme_id_ctrl *id;
>> +    u32 cmd_capsule_size;
>>       u16 status = 0;
>>         id = kzalloc(sizeof(*id), GFP_KERNEL);
>> @@ -433,9 +434,15 @@ static void nvmet_execute_identify_ctrl(struct 
>> nvmet_req *req)
>>         strlcpy(id->subnqn, ctrl->subsys->subsysnqn, 
>> sizeof(id->subnqn));
>>   -    /* Max command capsule size is sqe + single page of in-capsule 
>> data */
>> -    id->ioccsz = cpu_to_le32((sizeof(struct nvme_command) +
>> -                  req->port->inline_data_size) / 16);
>> +    /*
>> +     * Max command capsule size is sqe + in-capsule data size.
>> +     * Disable in-capsule data for Metadata capable controllers.
>> +     */
>> +    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);
>> +
>
> This seems odd to me, and should probably be referencing attributes of 
> the a transport, which should probably be set based on what transport 
> port the command was received on.

we set this in admin_connect:

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


>
>
>>       /* Max response capsule size is cqe */
>>       id->iorcsz = cpu_to_le32(sizeof(struct nvme_completion) / 16);
>>   @@ -465,6 +472,7 @@ static void nvmet_execute_identify_ctrl(struct 
>> nvmet_req *req)
>>     static void nvmet_execute_identify_ns(struct nvmet_req *req)
>>   {
>> +    struct nvmet_ctrl *ctrl = req->sq->ctrl;
>>       struct nvmet_ns *ns;
>>       struct nvme_id_ns *id;
>>       u16 status = 0;
>> @@ -482,7 +490,7 @@ static void nvmet_execute_identify_ns(struct 
>> nvmet_req *req)
>>       }
>>         /* return an all zeroed buffer if we can't find an active 
>> namespace */
>> -    ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
>> +    ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
>>       if (!ns)
>>           goto done;
>>   @@ -526,6 +534,16 @@ static void nvmet_execute_identify_ns(struct 
>> nvmet_req *req)
>>         id->lbaf[0].ds = ns->blksize_shift;
>>   +    if (ctrl->pi_support && nvmet_ns_has_pi(ns)) {
>> +        id->dpc = NVME_NS_DPC_PI_FIRST | NVME_NS_DPC_PI_LAST |
>> +              NVME_NS_DPC_PI_TYPE1 | NVME_NS_DPC_PI_TYPE2 |
>> +              NVME_NS_DPC_PI_TYPE3;
>
> Why is PI_TYPE2 getting set if nvmet_bdev_ns_enable_integrity() won't 
> set a metadata size ?

It wont:

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);
}

>
>> +        id->mc = NVME_MC_EXTENDED_LBA;
>> +        id->dps = ns->pi_type;
>> +        id->flbas = NVME_NS_FLBAS_META_EXT;
>
> I guess this is ok - always requiring extended lbas. Will this ever 
> change over time ?

This is according to the SPEC of NVMe/Fabrics


>
>> +        id->lbaf[0].ms = ns->metadata_size;
>> +    }
>> +
>>       if (ns->readonly)
>>           id->nsattr |= (1 << 0);
>>       nvmet_put_namespace(ns);
> ...
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index 50dfc60..9f6b0f9 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -322,12 +322,21 @@ int nvmet_enable_port(struct nvmet_port *port)
>>       if (!try_module_get(ops->owner))
>>           return -EINVAL;
>>   -    ret = ops->add_port(port);
>> -    if (ret) {
>> -        module_put(ops->owner);
>> -        return ret;
>> +    /*
>> +     * 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) {
>> +        pr_err("T10-PI is not supported by transport type %d\n",
>> +               port->disc_addr.trtype);
>> +        ret = -EINVAL;
>> +        goto out_put;
>>       }
>>   +    ret = ops->add_port(port);
>> +    if (ret)
>> +        goto out_put;
>> +
>
> ok for now - but it's going to have to be changed later to check the 
> port attributes, not the transport ops.  So it'll go back to add_port, 
> then validation w/ delete_port if fails.
>
>>       /* If the transport didn't set inline_data_size, then disable 
>> it. */
>>       if (port->inline_data_size < 0)
>>           port->inline_data_size = 0;
>> @@ -335,6 +344,10 @@ int nvmet_enable_port(struct nvmet_port *port)
>>       port->enabled = true;
>>       port->tr_ops = ops;
>>       return 0;
>> +
>> +out_put:
>> +    module_put(ops->owner);
>> +    return ret;
>>   }
>>     void nvmet_disable_port(struct nvmet_port *port)
>> diff --git a/drivers/nvme/target/fabrics-cmd.c 
>> b/drivers/nvme/target/fabrics-cmd.c
>> index 52a6f70..42bd12b 100644
>> --- a/drivers/nvme/target/fabrics-cmd.c
>> +++ b/drivers/nvme/target/fabrics-cmd.c
>> @@ -197,6 +197,8 @@ static void nvmet_execute_admin_connect(struct 
>> nvmet_req *req)
>>           goto out;
>>       }
>>   +    ctrl->pi_support = ctrl->port->pi_enable && 
>> ctrl->subsys->pi_support;
>> +
>>       uuid_copy(&ctrl->hostid, &d->hostid);
>>         status = nvmet_install_queue(ctrl, req);
>> @@ -205,8 +207,9 @@ static void nvmet_execute_admin_connect(struct 
>> nvmet_req *req)
>>           goto out;
>>       }
>>   -    pr_info("creating controller %d for subsystem %s for NQN %s.\n",
>> -        ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn);
>> +    pr_info("creating controller %d for subsystem %s for NQN %s%s.\n",
>> +        ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn,
>> +        ctrl->pi_support ? " T10-PI is enabled" : "");
>>       req->cqe->result.u16 = cpu_to_le16(ctrl->cntlid);
>>     out:
>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>> index ceedaaf..2986e2c 100644
>> --- a/drivers/nvme/target/nvmet.h
>> +++ b/drivers/nvme/target/nvmet.h
>> @@ -145,6 +145,7 @@ struct nvmet_port {
>>       bool                enabled;
>>       int                inline_data_size;
>>       const struct nvmet_fabrics_ops    *tr_ops;
>> +    bool                pi_enable;
>
> ok - but would have liked to have seen this generic-ized for metadata 
> support, then PI in the metadata.
>
>
> -- james
>
>
>

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