Re: [PATCH v17 2/2] nvmet: support reservation feature

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

On Wed, Oct 23, 2024 at 03:06:10PM +0800, Guixin Liu wrote:
> 在 2024/10/22 21:18, Sagi Grimberg 写道:
> > > +static void nvmet_execute_pr_acquire(struct nvmet_req *req)
> > > +{
> > > +    u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
> > > +    bool ignore_key = nvmet_pr_parse_ignore_key(cdw10);
> > > +    /* Reservation type, bit 15:08 */
> > > +    u8 rtype = (u8)((cdw10 >> 8) & 0xff);
> > > +    /* Reservation acquire action, bit 02:00 */
> > > +    u8 acquire_act = cdw10 & 0x07;
> > > +    struct nvmet_ctrl *ctrl = req->sq->ctrl;
> > > +    struct nvmet_pr_acquire_data *d = NULL;
> > > +    struct nvmet_pr *pr = &req->ns->pr;
> > > +    struct nvmet_pr_registrant *reg;
> > > +    u16 status = NVME_SC_SUCCESS;
> > > +
> > > +    if (ignore_key ||
> > > +        rtype < NVME_PR_WRITE_EXCLUSIVE ||
> > > +        rtype > NVME_PR_EXCLUSIVE_ACCESS_ALL_REGS) {
> > > +        status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
> > > +        goto out;
> > > +    }
> > > +
> > > +    d = kmalloc(sizeof(*d), GFP_KERNEL);
> > > +    if (!d) {
> > > +        status = NVME_SC_INTERNAL;
> > > +        goto out;
> > > +    }
> > > +
> > > +    status = nvmet_copy_from_sgl(req, 0, d, sizeof(*d));
> > > +    if (status)
> > > +        goto free_data;
> > > +
> > > +    status = NVME_SC_RESERVATION_CONFLICT | NVME_STATUS_DNR;
> > > +    mutex_lock(&pr->pr_lock);
> > > +    list_for_each_entry_rcu(reg, &pr->registrant_list, entry,
> > > +                lockdep_is_held(&pr->pr_lock)) {
> > > +        if (uuid_equal(&reg->hostid, &ctrl->hostid) &&
> > > +            reg->rkey == le64_to_cpu(d->crkey)) {
> > > +            status = __nvmet_execute_pr_acquire(req, reg,
> > > +                    acquire_act, rtype, d);
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    if (!status && acquire_act ==
> > > NVME_PR_ACQUIRE_ACT_PREEMPT_AND_ABORT) {
> > > +        kfree(d);
> > > +        INIT_WORK(&req->r.abort_work, nvmet_pr_do_abort);
> > > +        queue_work(nvmet_wq, &req->r.abort_work);
> > > +        return;
> > > +    }
> > 
> > Is there a reason why you queue this here and not inside
> > __nvmet_execute_pr_acquire
> > like before?
> > 
> Because I should add a "if" branch here to prevent unlock the pr_lock
> and complete
> 
> the request. Instead of twice "if", I merge them to one, just let
> __nvmet_execute_pr_acquire()
> 
> return status.
> 
> > > +
> > > +    mutex_unlock(&pr->pr_lock);
> > 
> > Hmm... you keep this mutex taken and release it from the work element
> > async... Not a
> > great practice...
> > 
> > Any easy way you see to avoid this?
> 
> Sure, this is not a greate practice, I will think deeply and find
> another way.
>

Guixin, please, take a look whether keeping that mutex locked during
aborting is actually needed or not. Probably there is no such need -
pr_lock protects ns.pr.registrants_list which is not touched in abort
procedure.

BR,
 Dmitry