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

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

在 2024/10/22 22:20, Christoph Hellwig 写道:
> On Tue, Oct 22, 2024 at 04:18:52PM +0300, Sagi Grimberg wrote:
>>> +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...
> It's actually invalid for a mutex.  You'd need a semaphore for that.
>
> Guixin, can you test the code with lockdep enabled and see if there
> is other fallout as well?  Thanks!

You mean use semaphore to replace mutex pr_lock? If this works, I

can send a v18.

Best Regards,

Guixin Liu