Re: [PATCH v11 1/1] nvmet: support reservation feature

[Guixin Liu <kanie@linux.alibaba.com>]

在 2024/9/30 20:53, Dmitry Bogdanov 写道:
> On Sun, Sep 29, 2024 at 11:14:10AM +0800, 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.
>> 5. set feature and get feature of reservation notify mask.
>> 6. get log page of reservation event.
>>
>> 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>
>> ---
>>   drivers/nvme/target/Makefile      |    2 +-
>>   drivers/nvme/target/admin-cmd.c   |   24 +-
>>   drivers/nvme/target/configfs.c    |   27 +
>>   drivers/nvme/target/core.c        |   56 +-
>>   drivers/nvme/target/fabrics-cmd.c |    4 +-
>>   drivers/nvme/target/nvmet.h       |   50 +-
>>   drivers/nvme/target/pr.c          | 1177 +++++++++++++++++++++++++++++
>>   include/linux/nvme.h              |   54 ++
>>   8 files changed, 1382 insertions(+), 12 deletions(-)
>>   create mode 100644 drivers/nvme/target/pr.c
>>
>> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
>> index bd87dfd173a4..9a56269cefd0 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -748,6 +748,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);
> sysfs_emit is to be used in show()
OK
>> +}
>> +
>
>> +
>> +static void nvmet_execute_pr_report(struct nvmet_req *req)
>> +{
>> +       u32 cdw11 = le32_to_cpu(req->cmd->common.cdw11);
>> +       u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
>> +       u32 num_bytes = 4 * (cdw10 + 1); /* cdw10 is number of dwords */
>> +       u8 eds = cdw11 & 1; /* Extended data structure, bit 00 */
>> +       struct nvme_registered_ctrl_ext *ctrl_eds;
>> +       struct nvme_reservation_status_ext *data;
>> +       struct nvmet_pr *pr = &req->ns->pr;
>> +       struct nvmet_pr_registrant *holder;
>> +       struct nvmet_pr_registrant *reg;
>> +       u16 num_ctrls = 0;
>> +       u16 status;
>> +       u8 rtype;
>> +
>> +       /* nvmet hostid(uuid_t) is 128 bit. */
>> +       if (!eds) {
>> +               req->error_loc = offsetof(struct nvme_common_command, cdw11);
>> +               status = NVME_SC_HOST_ID_INCONSIST | NVME_SC_DNR;
>> +               goto out;
>> +       }
>> +
>> +       if (num_bytes < sizeof(struct nvme_reservation_status_ext)) {
>> +               req->error_loc = offsetof(struct nvme_common_command, cdw10);
>> +               status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>> +               goto out;
>> +       }
>> +
>> +       data = kmalloc(num_bytes, GFP_KERNEL);
>> +       if (!data) {
>> +               status = NVME_SC_INTERNAL;
>> +               goto out;
>> +       }
>> +       memset(data, 0, num_bytes);
>> +       data->gen = cpu_to_le32(atomic_read(&pr->generation));
>> +       data->ptpls = 0;
>> +       ctrl_eds = data->regctl_eds;
>> +
>> +       rcu_read_lock();
>> +       holder = rcu_dereference(pr->holder);
>> +       rtype = holder ? holder->rtype : 0;
>> +       data->rtype = rtype;
>> +
>> +       list_for_each_entry_rcu(reg, &pr->registrant_list, entry) {
>> +               if ((void *)ctrl_eds >= (void *)(data + num_bytes))
>> +                       break;
>> +               /*
>> +                * Dynamic controller, set cntlid to 0xffff.
>> +                */
>> +               ctrl_eds->cntlid = 0xffff;
>> +               if (rtype == NVME_PR_WRITE_EXCLUSIVE_ALL_REGS ||
>> +                   rtype == NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS)
>> +                       ctrl_eds->rcsts = 1;
>> +               if (holder && uuid_equal(&reg->hostid, &holder->hostid))
> is just (reg == holder) not enough here?
Yes.
>> +                       ctrl_eds->rcsts = 1;
>> +               uuid_copy((uuid_t *)&ctrl_eds->hostid, &reg->hostid);
>> +               ctrl_eds->rkey = cpu_to_le64(reg->rkey);
>> +               ctrl_eds++;
>> +               num_ctrls++;
>> +       }
>> +       rcu_read_unlock();
>> +
>> +       put_unaligned_le16(num_ctrls, data->regctl);
> Here you should report the number of all registered registrants in ns,
> not the only reported. It is used by the host to understand that it got
> not a full response.
> NUMD field (cdw11) is about transfering the response. But the structure itsef in
> the reposnse has to be complete.

The NVMe spec says that:

     "This indicates the number of Registered Controller data structures 
and/or Registered Controller extended data structures contained in this 
data structure."

It means that host use this filed to obtain the reported registrants number.

Best Regards,

Guixin Liu

>> +       status = nvmet_copy_to_sgl(req, 0, data, num_bytes);
>> +       kfree(data);
>> +out:
>> +       nvmet_req_complete(req, status);
>> +}
>> +
> BR,
>   Dmitry