[PATCH V2 1/2] nvme-loop: use xarray for loop ctrl tracking

[Chaitanya Kulkarni]

On 10/5/20 05:46, Christoph Hellwig wrote:
> On Tue, Sep 29, 2020 at 09:55:56PM -0700, Chaitanya Kulkarni wrote:
>> @@ -262,12 +260,10 @@ static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
>>  {
>>  	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(nctrl);
>>  
>> -	if (list_empty(&ctrl->list))
>> +	if (!xa_load(&nvme_loop_ctrls, nctrl->cntlid))
>>  		goto free_ctrl;
>>  
>> -	mutex_lock(&nvme_loop_ctrl_mutex);
>> -	list_del(&ctrl->list);
>> -	mutex_unlock(&nvme_loop_ctrl_mutex);
>> +	xa_erase(&nvme_loop_ctrls, nctrl->cntlid);
> xa_erase is fine with an already deleted object, so the above xa_load
> check doesn't make any sense.
Okay, we can certainly get rid of the xa_load().
>> @@ -599,6 +590,12 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
>>  	if (ret)
>>  		goto out_free_queues;
>>  
>> +	/* unusual place to update xarray, makes unwind code simple */
>> +	ret = xa_insert(&nvme_loop_ctrls, ctrl->ctrl.cntlid, &ctrl,
>> +			GFP_KERNEL);
>> +	if (ret)
>> +		goto out_remove_ctrl;
>> +
>>  out_remove_admin_queue:
>>  	nvme_loop_destroy_admin_queue(ctrl);
>> +out_remove_ctrl:
>> +	xa_erase(&nvme_loop_ctrls, ctrl->ctrl.cntlid);
> This looks weird.  Why would you remove the controller when inserting
> it fails?
This is wrong it should be "goto out_free_queues:".
> Also did you do an audit that none of the lookups will do something
> funny with the insered but not fully initialized controller?

Now looking at the code xa_insert() should just replace original

list_add_tail() and do the right error handling for destructing the io

queues if any. Will adjust this patch in the next version.

>> @@ -678,7 +673,7 @@ static struct nvmf_transport_ops nvme_loop_transport = {
>>  	.name		= "loop",
>>  	.module		= THIS_MODULE,
>>  	.create_ctrl	= nvme_loop_create_ctrl,
>> -	.allowed_opts	= NVMF_OPT_TRADDR,
>> +	.allowed_opts	= NVMF_OPT_TRADDR | NVMF_OPT_CTRL_LOSS_TMO,
> This looks unrelated?
Yes, will remove.
>> +	xa_init(&nvme_loop_ctrls);
> DEFINE_XARRAY seems like the btter choice for nvme_loop_ctrls.
>
True.
>>  	return ret;
>>  }
>>  
>>  static void __exit nvme_loop_cleanup_module(void)
>>  {
>> -	struct nvme_loop_ctrl *ctrl, *next;
>> +	struct nvme_loop_ctrl *ctrl;
>> +	unsigned long idx;
>>  
>>  	nvmf_unregister_transport(&nvme_loop_transport);
>>  	nvmet_unregister_transport(&nvme_loop_ops);
>>  
>> -	mutex_lock(&nvme_loop_ctrl_mutex);
>> -	list_for_each_entry_safe(ctrl, next, &nvme_loop_ctrl_list, list)
>> +	xa_for_each(&nvme_loop_ctrls, idx, ctrl)
>>  		nvme_delete_ctrl(&ctrl->ctrl);
>> -	mutex_unlock(&nvme_loop_ctrl_mutex);
>> +
>> +	xa_destroy(&nvme_loop_ctrls);

Hmm, let me see if we can fix this in the next version without adding more

complexity.