[PATCH v9 1/1] nvmet: support reservation feature
Guixin Liu
kanie at linux.alibaba.com
Mon Sep 23 23:18:07 PDT 2024
在 2024/9/24 13:54, Dmitry Bogdanov 写道:
> On Tue, Sep 24, 2024 at 10:49:58AM +0800, Guixin Liu wrote:
>> 在 2024/9/24 00:32, Dmitry Bogdanov 写道:
>>>> 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 at 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 | 58 +-
>>>> drivers/nvme/target/nvmet.h | 49 ++
>>>> drivers/nvme/target/pr.c | 1214 +++++++++++++++++++++++++++++++
>>>> include/linux/nvme.h | 54 ++
>>>> 7 files changed, 1419 insertions(+), 9 deletions(-)
>>>> create mode 100644 drivers/nvme/target/pr.c
>>> ...
>>>> +
>>>> +static void nvmet_pr_unregister_one(struct nvmet_pr *pr,
>>>> + struct nvmet_pr_registrant *reg)
>>>> +{
>>>> + struct nvmet_pr_registrant *first_reg;
>>>> + struct nvmet_pr_registrant *holder;
>>>> + u8 original_rtype;
>>>> +
>>>> + lockdep_assert_held(&pr->pr_lock);
>>>> + list_del_rcu(®->entry);
>>>> +
>>>> + holder = rcu_dereference_protected(pr->holder,
>>>> + lockdep_is_held(&pr->pr_lock));
>>>> + if (reg != holder)
>>>> + goto out;
>>>> +
>>>> + original_rtype = holder->rtype;
>>>> + if (original_rtype == NVME_PR_WRITE_EXCLUSIVE_ALL_REGS ||
>>>> + original_rtype == NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS) {
>>>> + first_reg = list_first_or_null_rcu(&pr->registrant_list,
>>>> + struct nvmet_pr_registrant, entry);
>>>> + if (first_reg)
>>>> + first_reg->rtype = original_rtype;
>>>> + rcu_assign_pointer(pr->holder, first_reg);
>>>> + } else {
>>>> + rcu_assign_pointer(pr->holder, NULL);
>>>> +
>>>> + if (original_rtype == NVME_PR_WRITE_EXCLUSIVE_REG_ONLY ||
>>>> + original_rtype == NVME_PR_WRITE_EXCLUSIVE_REG_ONLY)
>>> copy&past error?
>>> The second check should be against NVME_PR_EXCLUSIVE_ACCESS_REG_ONLY?
>> Indeed, my mistake, it will be fixed in v10.
>>>> + nvmet_pr_resv_released(pr, ®->hostid);
>>>> + }
>>>> +out:
>>>> + synchronize_rcu();
>>>> + kfree(reg);
>>>> +}
>>>> +
>>>> ......
>>>> +
>>>> +static u16 nvmet_pr_preempt(struct nvmet_req *req,
>>>> + struct nvmet_pr_registrant *reg,
>>>> + u8 rtype,
>>>> + struct nvmet_pr_acquire_data *d,
>>>> + bool abort)
>>>> +{
>>>> + struct nvmet_ctrl *ctrl = req->sq->ctrl;
>>>> + struct nvmet_pr *pr = &req->ns->pr;
>>>> + struct nvmet_pr_registrant *holder;
>>>> + enum nvme_pr_type original_rtype;
>>>> + u64 prkey = le64_to_cpu(d->prkey);
>>>> + u16 status;
>>>> +
>>>> + lockdep_assert_held(&pr->pr_lock);
>>>> + holder = rcu_dereference_protected(pr->holder,
>>>> + lockdep_is_held(&pr->pr_lock));
>>>> + if (!holder)
>>>> + return nvmet_pr_unreg_all_host_by_prkey(req, prkey,
>>>> + &ctrl->hostid, abort);
>>>> +
>>>> + original_rtype = holder->rtype;
>>>> + if (original_rtype == NVME_PR_WRITE_EXCLUSIVE_ALL_REGS ||
>>>> + original_rtype == NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS) {
>>>> + if (!prkey) {
>>>> + nvmet_pr_unreg_all_others(req, &ctrl->hostid, abort);
>>>> + nvmet_pr_set_new_holder(pr, rtype, reg);
>>> You didnot address the reservation atomicity here. You still remove an
>>> old holder and then after a some time you set the new holder.
>>> In this time gap an incomming WRITE command from a non holder will write
>>> to the media.
>> Right, I will change it in v10.
>>>> + return NVME_SC_SUCCESS;
>>>> + }
>>>> + return nvmet_pr_unreg_all_host_by_prkey(req, prkey,
>>>> + &ctrl->hostid, abort);
>>>> + }
>>>> +
>>>> + if (holder == reg) {
>>>> + status = nvmet_pr_update_reg_attr(pr, holder,
>>>> + nvmet_pr_update_holder_rtype, &rtype);
>>>> + if (!status && original_rtype != rtype)
>>>> + nvmet_pr_resv_released(pr, ®->hostid);
>>>> + return status;
>>>> + }
>>>> +
>>>> + if (prkey == holder->rkey) {
>>>> + status = nvmet_pr_unreg_all_others_by_prkey(req, prkey,
>>>> + &ctrl->hostid, abort);
>>> And here too that timegap with released reservation.
>> I will fix this too, and also the
>>
>> nvmet_pr_unreg_all_others_by_prkey() does not need a return value,
>>
>> I remove it.
>>
>>>> + if (status)
>>>> + return status;
>>>> +
>>>> + nvmet_pr_set_new_holder(pr, rtype, reg);
>>>> + if (original_rtype != rtype)
>>>> + nvmet_pr_resv_released(pr, ®->hostid);
>>> This function is(may be) already invoked in nvmet_pr_unregister_one.
>>>
>> Not, here the original_rtype is not NVME_PR_WRITE_EXCLUSIVE_REG_ONLY or
>> NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS, so the nvmet_pr_unregister_one will
>> not call nvmet_pr_resv_released.
> nvmet_pr_unregister_one sends an event for original_rtype == *_REG_ONLY
> Here you have only original_rtype != ALL_REGS, so _REG_ONLY is possible.
My mistake, it will be removed in v10, thanks.
>>>> + return status;
>>>> + }
>>>> +
>>>> + if (prkey)
>>>> + return nvmet_pr_unreg_all_host_by_prkey(req, prkey,
>>>> + &ctrl->hostid, abort);
>>>> + return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
>>>> +}
>>>> +
>>>> +static void nvmet_pr_confirm_ns_pc_ref(struct percpu_ref *ref)
>>>> +{
>>>> + struct nvmet_pr_per_ctrl_ref *pc_ref =
>>>> + container_of(ref, struct nvmet_pr_per_ctrl_ref, ref);
>>>> +
>>>> + complete(&pc_ref->confirm_done);
>>>> +}
>>>> +
>>>> +static void nvmet_pr_do_abort(struct nvmet_req *req)
>>>> +{
>>>> + struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
>>>> + struct nvmet_pr_per_ctrl_ref *pc_ref;
>>>> + struct nvmet_ctrl *ctrl = NULL;
>>>> +
>>>> +find_next:
>>>> + mutex_lock(&subsys->lock);
>>>> + list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
>>>> + if (ctrl->pr_abort)
>>>> + goto do_abort;
>>>> + }
>>>> + mutex_unlock(&subsys->lock);
>>>> + return;
>>>> +
>>>> +do_abort:
>>>> + mutex_unlock(&subsys->lock);
>>>> + /*
>>>> + * The target dose not support abort, just wait ref to 0.
>>> typo "dose"->"does"
>>>
>> Sure.
>>>> + */
>>>> + pc_ref = xa_load(&req->ns->pr_per_ctrl_refs, ctrl->cntlid);
>>>> + percpu_ref_kill_and_confirm(&pc_ref->ref, nvmet_pr_confirm_ns_pc_ref);
>>>> + wait_for_completion(&pc_ref->confirm_done);
>>>> + wait_for_completion(&pc_ref->free_done);
>>>> + reinit_completion(&pc_ref->confirm_done);
>>>> + reinit_completion(&pc_ref->free_done);
>>>> + percpu_ref_resurrect(&pc_ref->ref);
>>>> +
>>>> + ctrl->pr_abort = false;
>>> pr_abort flag is per controller, but this function is per namespace.
>>> The use case of preempt_and_abort is to preempt reservation of a "zomby" host
>>> for the all namespaces. It means that preempt_and_abort command will be sent
>>> for the each namespace and there will be several parallel abort processes for
>>> the same controllers. And you should not mix them.
>>>
>>> May be doing ref_kill instead of setting ctlr->pr_abort=true will more
>>> accuratelly set the barier for incoming IO. You may use Mark feature in
>>> XArrays to mark pc_ref to wait the completion for and use xa_for_each_marked
>>> for going through such pc_refs.
>> Good idea, thanks very much, will changed in v10.
>>
>> Best Regards,
>>
>> Guixin Liu
>>
>>>> + nvmet_ctrl_put(ctrl);
>>>> + goto find_next;
>>>> +}
>>>> +
>>>> +static u16 __nvmet_execute_pr_acquire(struct nvmet_req *req,
>>>> + struct nvmet_pr_registrant *reg,
>>>> + u8 acquire_act,
>>>> + u8 rtype,
>>>> + struct nvmet_pr_acquire_data *d)
>>>> +{
>>>> + u16 status;
>>>> +
>>>> + switch (acquire_act) {
>>>> + case NVME_PR_ACQUIRE_ACT_ACQUIRE:
>>>> + status = nvmet_pr_acquire(req, reg, rtype);
>>>> + goto out;
>>>> + case NVME_PR_ACQUIRE_ACT_PREEMPT:
>>>> + status = nvmet_pr_preempt(req, reg, rtype, d, false);
>>>> + goto inc_gen;
>>>> + case NVME_PR_ACQUIRE_ACT_PREEMPT_AND_ABORT:
>>>> + status = nvmet_pr_preempt(req, reg, rtype, d, true);
>>>> + if (!status)
>>>> + nvmet_pr_do_abort(req);
>>>> + goto inc_gen;
>>>> + default:
>>>> + req->error_loc = offsetof(struct nvme_common_command, cdw10);
>>>> + status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
>>>> + goto out;
>>>> + }
>>>> +inc_gen:
>>>> + if (!status)
>>>> + atomic_inc(&req->ns->pr.generation);
>>>> +out:
>>>> + return status;
>>>> +}
>>>> +
More information about the Linux-nvme
mailing list