Re: [PATCH v13 03/12] bcache: initialization of the buddy

[Jens Axboe <axboe@kernel.dk>]

> diff --git a/drivers/md/bcache/nvmpg.c b/drivers/md/bcache/nvmpg.c
> index b654bbbda03e..2b70ee4a6028 100644
> --- a/drivers/md/bcache/nvmpg.c
> +++ b/drivers/md/bcache/nvmpg.c
> @@ -50,6 +50,36 @@ unsigned long bch_nvmpg_ptr_to_offset(struct bch_nvmpg_ns *ns, void *ptr)
>  	return BCH_NVMPG_OFFSET(ns_id, offset);
>  }
>  
> +static struct page *bch_nvmpg_va_to_pg(void *addr)
> +{
> +	return virt_to_page(addr);
> +}

What's the purpose of this helper?

> +static inline void reserve_nvmpg_pages(struct bch_nvmpg_ns *ns,
> +				       pgoff_t pgoff, u64 nr)
> +{
> +	while (nr > 0) {
> +		unsigned int num = nr > UINT_MAX ? UINT_MAX : nr;

Surely UINT_MAX isn't anywhere near a valid limit?

> @@ -76,10 +110,73 @@ static void release_nvmpg_set(struct bch_nvmpg_set *set)
>  	kfree(set);
>  }
>  
> +static int validate_recs(int ns_id,
> +			 struct bch_nvmpg_head *head,
> +			 struct bch_nvmpg_recs *recs)
> +{
> +	if (memcmp(recs->magic, bch_nvmpg_recs_magic, 16)) {
> +		pr_err("Invalid bch_nvmpg_recs magic\n");
> +		return -EINVAL;
> +	}
> +
> +	if (memcmp(recs->uuid, head->uuid, 16)) {
> +		pr_err("Invalid bch_nvmpg_recs uuid\n");
> +		return -EINVAL;
> +	}
> +
> +	if (recs->head_offset !=
> +	    bch_nvmpg_ptr_to_offset(global_nvmpg_set->ns_tbl[ns_id], head)) {
> +		pr_err("Invalid recs head_offset\n");
> +		return -EINVAL;
> +	}

Same comments here on the frivilous error messaging, other places in
this file too. Check all the other patches as well, please.

>  /* Namespace 0 contains all meta data of the nvmpg allocation set */
>  static int init_nvmpg_set_header(struct bch_nvmpg_ns *ns)
>  {
>  	struct bch_nvmpg_set_header *set_header;
> +	struct bch_nvmpg_recs *sys_recs;
> +	int i, j, used = 0, rc = 0;
>  
>  	if (ns->ns_id != 0) {
>  		pr_err("unexpected ns_id %u for first nvmpg namespace.\n",
> @@ -93,9 +190,83 @@ static int init_nvmpg_set_header(struct bch_nvmpg_ns *ns)
>  	global_nvmpg_set->set_header = set_header;
>  	global_nvmpg_set->heads_size = set_header->size;
>  	global_nvmpg_set->heads_used = set_header->used;
> +
> +	/* Reserve the used space from buddy allocator */
> +	reserve_nvmpg_pages(ns, 0, div_u64(ns->pages_offset, ns->page_size));
> +
> +	sys_recs = ns->base_addr + BCH_NVMPG_SYSRECS_OFFSET;
> +	for (i = 0; i < set_header->size; i++) {
> +		struct bch_nvmpg_head *head;
> +
> +		head = &set_header->heads[i];
> +		if (head->state == BCH_NVMPG_HD_STAT_FREE)
> +			continue;
> +
> +		used++;
> +		if (used > global_nvmpg_set->heads_size) {
> +			pr_err("used heads %d > heads size %d.\n",
> +			       used, global_nvmpg_set->heads_size);
> +			goto unlock;
> +		}
> +
> +		for (j = 0; j < BCH_NVMPG_NS_MAX; j++) {
> +			struct bch_nvmpg_recs *recs;
> +
> +			recs = bch_nvmpg_offset_to_ptr(head->recs_offset[j]);
> +
> +			/* Iterate the recs list */
> +			while (recs) {
> +				rc = validate_recs(j, head, recs);
> +				if (rc < 0)
> +					goto unlock;
> +
> +				rc = reserve_nvmpg_recs(recs);
> +				if (rc < 0)
> +					goto unlock;
> +
> +				bitmap_set(ns->recs_bitmap, recs - sys_recs, 1);
> +				recs = bch_nvmpg_offset_to_ptr(recs->next_offset);
> +			}
> +		}
> +	}
> +unlock:
>  	mutex_unlock(&global_nvmpg_set->lock);
> +	return rc;
> +}
>  
> -	return 0;
> +static void bch_nvmpg_init_free_space(struct bch_nvmpg_ns *ns)
> +{
> +	unsigned int start, end, pages;
> +	int i;
> +	struct page *page;
> +	pgoff_t pgoff_start;
> +
> +	bitmap_for_each_clear_region(ns->pages_bitmap,
> +				     start, end, 0, ns->pages_total) {
> +		pgoff_start = start;
> +		pages = end - start;
> +
> +		while (pages) {
> +			void *addr;
> +
> +			for (i = BCH_MAX_ORDER - 1; i >= 0; i--) {
> +				if ((pgoff_start % (1L << i) == 0) &&
> +				    (pages >= (1L << i)))
> +					break;
> +			}
> +
> +			addr = bch_nvmpg_pgoff_to_ptr(ns, pgoff_start);
> +			page = bch_nvmpg_va_to_pg(addr);
> +			set_page_private(page, i);
> +			page->index = pgoff_start;
> +			__SetPageBuddy(page);
> +			list_add((struct list_head *)&page->zone_device_data,
> +				 &ns->free_area[i]);
> +
> +			pgoff_start += 1L << i;
> +			pages -= 1L << i;
> +		}
> +	}
>  }
>  
>  static int attach_nvmpg_set(struct bch_nvmpg_ns *ns)
> @@ -200,7 +371,7 @@ struct bch_nvmpg_ns *bch_register_namespace(const char *dev_path)
>  	char buf[BDEVNAME_SIZE];
>  	struct block_device *bdev;
>  	pgoff_t pgoff;
> -	int id, err;
> +	int id, i, err;
>  	char *path;
>  	long dax_ret = 0;
>  
> @@ -304,13 +475,48 @@ struct bch_nvmpg_ns *bch_register_namespace(const char *dev_path)
>  
>  	mutex_init(&ns->lock);
>  
> +	/*
> +	 * parameters of bitmap_set/clear are unsigned int.
> +	 * Given currently size of nvm is far from exceeding this limit,
> +	 * so only add a WARN_ON message.
> +	 */
> +	WARN_ON(BITS_TO_LONGS(ns->pages_total) > UINT_MAX);

Does this really need to be a WARN_ON()? Looks more like an -EINVAL
condition.


-- 
Jens Axboe