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

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Sat, Oct 05, 2024 at 10:26:58PM +0800, Guixin Liu wrote:
> 在 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>
> > > ---
> > > +
> > > +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."

You read not the latest Spec again. Spec v2.1 has another statement:
  Number of Registrants (REGSTRNT): This field indicates the number of registrants of the
  namespace. This indicates the number of Registrant data structures or Registrant Extended data
  structures contained in this data structure.

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

Think from the host point of view - how are you going to understand
how much registrants are there in the namespace? 

BR,
 Dmitry