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

[Christoph Hellwig <hch@lst.de>]

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.

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

Also did you do an audit that none of the lookups will do something
funny with the insered but not fully initialized controller?

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

> +	xa_init(&nvme_loop_ctrls);

DEFINE_XARRAY seems like the btter choice for nvme_loop_ctrls.


>  	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);

What replace the protection previously provided by nvme_loop_ctrl_mutex?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme