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

[Christoph Hellwig <hch@lst.de>]

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.

Also, do you have a corresponding nvmetcli patch?

> +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?

> +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)

> +	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?

> +	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.

The explicit cast to bool should also not be needed.

> +	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?

> +		/*
> +		 * Dynamic controller, set cntlid to 0xffff.
> +		 */
> +		ctrl_eds->cntlid = 0xffff;

NVME_CNTLID_DYNAMIC

> +	req->pc_ref = xa_load(&req->ns->pr_per_ctrl_refs, req->sq->ctrl->cntlid);

Overly long line.

> +	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.

> 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.