Re: [PATCH] nvmet: support reservation feature

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

On 1/9/24 21:58, Guixin Liu wrote:
>
> 在 2024/1/10 12:34, Chaitanya Kulkarni 写道:
>> On 1/9/24 04:10, Guixin Liu wrote:
>>> This patch implements the reservation feature, includes:
>>> 1. reservation register(register, unregister and replace).
>>> 2. reservation acquire(acquire, preempt, preempt and abort).
>>> 3. reservation release(release and clear).
>>> 4. reservation report.
>>>
>>> And also make reservation configurable, one can set ns to support
>>> reservation before enable ns. The default of resv_enable is false.
>>>
>>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>>> ---
>>> Hi guys,
>>>       I've implemented the NVMe reservation feature. Please review 
>>> it, all
>>> comments are welcome.
>>>       In addtion, I didn't implement event reporting because I 
>>> didn't see
>>> any handling of these events on the host side. If these events are 
>>> mandatory
>>> to report, please let me know so that I can implement them.
>>>
>>>    drivers/nvme/target/Makefile    |   2 +-
>>>    drivers/nvme/target/admin-cmd.c |  14 +-
>>>    drivers/nvme/target/configfs.c  |  27 ++
>>>    drivers/nvme/target/core.c      |  37 +-
>>>    drivers/nvme/target/nvmet.h     |  26 ++
>>>    drivers/nvme/target/pr.c        | 806 
>>> ++++++++++++++++++++++++++++++++
>>>    include/linux/nvme.h            |  30 ++
>>>    7 files changed, 939 insertions(+), 3 deletions(-)
>>>    create mode 100644 drivers/nvme/target/pr.c
>>>
>>> diff --git a/drivers/nvme/target/Makefile 
>>> b/drivers/nvme/target/Makefile
>>> index c66820102493..f9bfc904a5b3 100644
>>> --- a/drivers/nvme/target/Makefile
>>> +++ b/drivers/nvme/target/Makefile
>>> @@ -10,7 +10,7 @@ obj-$(CONFIG_NVME_TARGET_FCLOOP)    += nvme-fcloop.o
>>>    obj-$(CONFIG_NVME_TARGET_TCP)        += nvmet-tcp.o
>>>       nvmet-y        += core.o configfs.o admin-cmd.o fabrics-cmd.o \
>>> -            discovery.o io-cmd-file.o io-cmd-bdev.o
>>> +            discovery.o io-cmd-file.o io-cmd-bdev.o pr.o
>>>    nvmet-$(CONFIG_NVME_TARGET_PASSTHRU)    += passthru.o
>>>    nvmet-$(CONFIG_BLK_DEV_ZONED)        += zns.o
>>>    nvmet-$(CONFIG_NVME_TARGET_AUTH)    += fabrics-cmd-auth.o auth.o
>>> diff --git a/drivers/nvme/target/admin-cmd.c 
>>> b/drivers/nvme/target/admin-cmd.c
>>> index 39cb570f833d..7da6f3085a4c 100644
>>> --- a/drivers/nvme/target/admin-cmd.c
>>> +++ b/drivers/nvme/target/admin-cmd.c
>>> @@ -550,7 +550,13 @@ static void nvmet_execute_identify_ns(struct 
>>> nvmet_req *req)
>>>         */
>>>        id->nmic = NVME_NS_NMIC_SHARED;
>>>        id->anagrpid = cpu_to_le32(req->ns->anagrpid);
>>> -
>>> +    id->rescap = NVME_PR_SUPPORT_WRITE_EXCLUSIVE |
>>> +             NVME_PR_SUPPORT_EXCLUSIVE_ACCESS |
>>> +             NVME_PR_SUPPORT_WRITE_EXCLUSIVE_REG_ONLY |
>>> +             NVME_PR_SUPPORT_EXCLUSIVE_ACCESS_REG_ONLY |
>>> +             NVME_PR_SUPPORT_WRITE_EXCLUSIVE_ALL_REGS |
>>> +             NVME_PR_SUPPORT_EXCLUSIVE_ACCESS_ALL_REGS |
>>> +             NVME_PR_SUPPORT_IEKEY_DEF_LATER_VER_1_3;
>>>        memcpy(&id->nguid, &req->ns->nguid, sizeof(id->nguid));
>>>           id->lbaf[0].ds = req->ns->blksize_shift;
>>> @@ -1017,6 +1023,12 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
>>>        if (nvmet_is_passthru_req(req))
>>>            return nvmet_parse_passthru_admin_cmd(req);
>>>    +    ret = nvmet_pr_check_cmd_access(req);
>>> +    if (unlikely(ret)) {
>>> +        req->error_loc = offsetof(struct nvme_common_command, opcode);
>>> +        return ret;
>>> +    }
>>> +
>>>        switch (cmd->common.opcode) {
>>>        case nvme_admin_get_log_page:
>>>            req->execute = nvmet_execute_get_log_page;
>>> diff --git a/drivers/nvme/target/configfs.c 
>>> b/drivers/nvme/target/configfs.c
>>> index d937fe05129e..1ac4802ec818 100644
>>> --- a/drivers/nvme/target/configfs.c
>>> +++ b/drivers/nvme/target/configfs.c
>>> @@ -714,6 +714,32 @@ static ssize_t 
>>> nvmet_ns_revalidate_size_store(struct config_item *item,
>>>       CONFIGFS_ATTR_WO(nvmet_ns_, revalidate_size);
>>>    +static ssize_t nvmet_ns_resv_enable_show(struct config_item 
>>> *item, char *page)
>>> +{
>>> +    return sprintf(page, "%d\n", to_nvmet_ns(item)->pr.enable);
>>> +}
>>> +
>>> +static ssize_t nvmet_ns_resv_enable_store(struct config_item *item,
>>> +                    const char *page, size_t count)
>>> +{
>>> +    struct nvmet_ns *ns = to_nvmet_ns(item);
>>> +    bool val;
>>> +
>>> +    if (kstrtobool(page, &val))
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&ns->subsys->lock);
>>> +    if (ns->enabled) {
>>> +        pr_err("the ns:%d is already enabled.\n", ns->nsid);
>>> +        mutex_unlock(&ns->subsys->lock);
>>> +        return -EINVAL;
>>> +    }
>>> +    ns->pr.enable = val;
>>> +    mutex_unlock(&ns->subsys->lock);
>>> +    return count;
>>> +}
>>> +CONFIGFS_ATTR(nvmet_ns_, resv_enable);
>>> +
>>>    static struct configfs_attribute *nvmet_ns_attrs[] = {
>>>        &nvmet_ns_attr_device_path,
>>>        &nvmet_ns_attr_device_nguid,
>>> @@ -722,6 +748,7 @@ static struct configfs_attribute 
>>> *nvmet_ns_attrs[] = {
>>>        &nvmet_ns_attr_enable,
>>>        &nvmet_ns_attr_buffered_io,
>>>        &nvmet_ns_attr_revalidate_size,
>>> +    &nvmet_ns_attr_resv_enable,
>>>    #ifdef CONFIG_PCI_P2PDMA
>>>        &nvmet_ns_attr_p2pmem,
>>>    #endif
>>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>>> index 3935165048e7..8eab81804b14 100644
>>> --- a/drivers/nvme/target/core.c
>>> +++ b/drivers/nvme/target/core.c
>>> @@ -598,6 +598,7 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
>>>        subsys->nr_namespaces++;
>>>           nvmet_ns_changed(subsys, ns->nsid);
>>> +    nvmet_pr_init_ns(ns);
>>>        ns->enabled = true;
>>>        ret = 0;
>>>    out_unlock:
>>> @@ -651,6 +652,7 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
>>>           subsys->nr_namespaces--;
>>>        nvmet_ns_changed(subsys, ns->nsid);
>>> +    nvmet_pr_clean_all_registrants(&ns->pr);
>>>        nvmet_ns_dev_disable(ns);
>>>    out_unlock:
>>>        mutex_unlock(&subsys->lock);
>>> @@ -904,6 +906,16 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req 
>>> *req)
>>>            return ret;
>>>        }
>>>    +    ret = nvmet_pr_check_cmd_access(req);
>>> +    if (unlikely(ret)) {
>>> +        req->error_loc = offsetof(struct nvme_common_command, opcode);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = nvmet_parse_pr_cmd(req);
>>> +    if (!ret)
>>> +        return ret;
>>> +
>> Can we make this feature configurable via Kconfig? If someone doesn't
>> want to
>> use PR, they will have to bear the cost of these checks in the fast 
>> path.
>
> Yeah, I have added a resv_enable in configfs, the default is false, 
> one can
>
> make reservation enable before enable namespace.

Why can't we make it KConfig option ? Is there any particular reason for
not doing that ? That will also allow user to avoid kernel compilation
of code if they want to turn it off.

-ck