[PATCH v9 1/1] nvmet: support reservation feature
Dmitry Bogdanov
d.bogdanov at yadro.com
Tue Sep 24 01:24:25 PDT 2024
On Tue, Sep 24, 2024 at 02:38:40PM +0800, Guixin Liu wrote:
>
> 在 2024/9/24 14:18, Guixin Liu 写道:
> >
> > 在 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.
>
> I take a look again, if we set self new holder before call
> nvmet_pr_unreg_all_others_by_prkey(), the
> nvmet_pr_unreg_all_others_by_prkey() will
>
> not unregister self, so this will not goto nvmet_pr_unregister_one()'s
> calling nvmet_pr_resv_released().
Yes, and this is a reason not to try to fix non-atomicity (anothter my
comment) by setting new holder before unregistering.
Regarding this place, here nvmet_pr_resv_released should be called for
original_rtype !=*_REG_ONLY with a note that _REG_ONLY handled in nvmet_pr_unregister_one.
Please, do not take my suggestions "how to fix" as a direct order, it's
just suggestion.
> > > > > > + 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);
> > > > > > +}
> > > > > > +
More information about the Linux-nvme
mailing list