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

[Christoph Hellwig <hch@lst.de>]

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!