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

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

在 2024/10/23 21:30, Dmitry Bogdanov 写道:
> 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

Well, I should prevent there is no reinit_completion() between complete()

and wait_for_completion(), this may happen if there is another "registion"

and "preempt and abort" when doing abort, although this is a host's bug.

Best Regards,

Guixin Liu