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

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

在 2024/10/2 16:09, Christoph Hellwig 写道:
> 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.
> The explanation feels a bit sparse.  It could also mentioned that
> no support for persistent reservation exists, and how this code
> was tested.
I will add more detail in commit in v12.
>
> Also, do you have a corresponding nvmetcli patch?
Sure, I think so.
>> +struct nvmet_pr_register_data {
>> +	__le64	crkey;
>> +	__le64	nrkey;
>> +};
>> +
>> +struct nvmet_pr_acquire_data {
>> +	__le64	crkey;
>> +	__le64	prkey;
>> +};
>> +
>> +struct nvmet_pr_release_data {
>> +	__le64	crkey;
>> +};
> Using little endian fields for purely in-memory data feels strange.
> Is there a good reason for this?
>
I use nvmet_copy_from_sgl to obtain the command's detail, just like

nvme_dsm_range, maybe I can put them in to include/linux/nvme.h.

>> +static u16 nvmet_pr_update_reg_attr(struct nvmet_pr *pr,
>> +				    struct nvmet_pr_registrant *reg,
>> +				    void (*change_attr)(struct nvmet_pr_registrant *reg,
>> +					void *attr),
>> +				    void *attr)
> Please avoid the overly long line here.  That's easiest done by
> following the style used elsewhere in the nvme code using two
> tab continuations:
>
> static u16 nvmet_pr_update_reg_attr(struct nvmet_pr *pr,
> 		struct nvmet_pr_registrant *reg,
> 		void (*change_attr)(struct nvmet_pr_registrant *reg,
> 				void *attr),
> 		void *attr)
OK.
>> +	change_attr(new, attr);
>> +	list_replace_rcu(&holder->entry, &new->entry);
>> +	rcu_assign_pointer(pr->holder, new);
>> +	synchronize_rcu();
>> +	kfree(holder);
> Does this really need a full blown expensive synchronize_rcu vs just a
> cheaper kfree_rcu_mightsleep or kfree_rcu?
Yes, I will use kfree_rcu instead.
>> +	bool ignore_key = (bool)((cdw10 >> 3) & 1); /* Ignore existing key, bit 03 */
> Overly long line.  This might also benefit from adding symbolic constants
> and/or extraction helpers.
OK.
>
> The explicit cast to bool should also not be needed.
>
OK.
>> +	struct nvmet_pr_registrant *reg, *tmp;
>> +	struct nvmet_pr *pr = &req->ns->pr;
>> +	LIST_HEAD(free_list);
>> +
>> +	lockdep_assert_held(&pr->pr_lock);
>> +
>> +	rcu_assign_pointer(pr->holder, NULL);
>> +
>> +	list_for_each_entry_safe(reg, tmp, &pr->registrant_list, entry) {
>> +		list_del_rcu(&reg->entry);
>> +		if (!uuid_equal(&req->sq->ctrl->hostid, &reg->hostid))
>> +			nvmet_pr_resv_preempted(pr, &reg->hostid);
>> +		list_add(&reg->entry, &free_list);
>> +	}
>> +	synchronize_rcu();
>> +	list_for_each_entry_safe(reg, tmp, &free_list, entry) {
>> +		kfree(reg);
>> +	}
> No nee for the outer braces here.  But why do we we need the expensive
> synchronize_rcu and two-step operation here anyway vs just using
> kfree_rcu?
Sure.
>> +		/*
>> +		 * Dynamic controller, set cntlid to 0xffff.
>> +		 */
>> +		ctrl_eds->cntlid = 0xffff;
> NVME_CNTLID_DYNAMIC
>
OK, will be changed in v12.
>> +	req->pc_ref = xa_load(&req->ns->pr_per_ctrl_refs, req->sq->ctrl->cntlid);
> Overly long line.
Changed.
>
>> +	if (unlikely(!percpu_ref_tryget_live(&req->pc_ref->ref)))
>> +		return NVME_SC_INTERNAL;
>> +	return NVME_SC_SUCCESS;
>> +}
>> +
>> +void nvmet_pr_put_ns_pc_ref(struct nvmet_req *req)
>> +{
>> +	if (req->pc_ref)
>> +		percpu_ref_put(&req->pc_ref->ref);
>> +}
> It would be niceto have the NULL check inline to avoid the call for
> for namespaces without reservation support.
Understood, I will change nvmet_pr_put_ns_pc_ref to inline.
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index 425573202295..b1be3d313bee 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
> Please split out adding the new code points to nvme.h to a separate
> prep patch.

OK, I will split it in v12.

Best regards,

Guixin Liu