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

[Sagi Grimberg <sagi@grimberg.me>]

> +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?

> +
> +	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?