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

[Guixin Liu]

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

Best Regards,

Guixin Liu