Re: [PATCH] nvme: keep ctrl->namespaces ordered

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/09/16 0:43, Christoph Hellwig wrote:
> Various places in the nvme code that rely on ctrl->namespace to be
> ordered.  Ensure that the namespae is inserted into the list at the
> right position from the start instead of sorting it after the fact.
> 
> Fixes: 540c801c65eb ("NVMe: Implement namespace list scanning")
> Reported-by: Anton Eidelman <anton.eidelman@gmail.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvme/host/core.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 7efb31b87f37a..3894a8508065d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -13,7 +13,6 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/backing-dev.h>
> -#include <linux/list_sort.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/pr.h>
> @@ -3714,15 +3713,6 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
>  	return ret;
>  }
>  
> -static int ns_cmp(void *priv, const struct list_head *a,
> -		const struct list_head *b)
> -{
> -	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
> -	struct nvme_ns *nsb = container_of(b, struct nvme_ns, list);
> -
> -	return nsa->head->ns_id - nsb->head->ns_id;
> -}
> -
>  struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  {
>  	struct nvme_ns *ns, *ret = NULL;
> @@ -3743,6 +3733,22 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>  }
>  EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
>  
> +/*
> + * Add the namespace to the controller list while keeping the list ordered.
> + */

May be add a note here about the impossibility to ever get a duplicate NS IDs ?

> +static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
> +{
> +	struct nvme_ns *tmp;
> +
> +	list_for_each_entry_reverse(tmp, &ns->ctrl->namespaces, list) {
> +		if (tmp->head->ns_id < ns->head->ns_id) {
> +			list_add(&ns->list, &tmp->list);
> +			return;
> +		}
> +	}
> +	list_add(&ns->list, &ns->ctrl->namespaces);
> +}
> +
>  static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  		struct nvme_ns_ids *ids)
>  {
> @@ -3793,9 +3799,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
>  		goto out_unlink_ns;
>  
>  	down_write(&ctrl->namespaces_rwsem);
> -	list_add_tail(&ns->list, &ctrl->namespaces);
> +	nvme_ns_add_to_ctrl_list(ns);
>  	up_write(&ctrl->namespaces_rwsem);
> -
>  	nvme_get_ctrl(ctrl);
>  
>  	if (device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups))
> @@ -4083,10 +4088,6 @@ static void nvme_scan_work(struct work_struct *work)
>  	if (nvme_scan_ns_list(ctrl) != 0)
>  		nvme_scan_ns_sequential(ctrl);
>  	mutex_unlock(&ctrl->scan_lock);
> -
> -	down_write(&ctrl->namespaces_rwsem);
> -	list_sort(NULL, &ctrl->namespaces, ns_cmp);
> -	up_write(&ctrl->namespaces_rwsem);
>  }
>  
>  /*
> 

Apart from the nit above, looks OK to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>


-- 
Damien Le Moal
Western Digital Research

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