Re: [PATCH v13 05/12] bcache: bch_nvmpg_free_pages() of the buddy allocator

[Jens Axboe <axboe@kernel.dk>]

On 12/12/21 10:05 AM, Coly Li wrote:
> diff --git a/drivers/md/bcache/nvmpg.c b/drivers/md/bcache/nvmpg.c
> index a920779eb548..8ce0c4389b42 100644
> --- a/drivers/md/bcache/nvmpg.c
> +++ b/drivers/md/bcache/nvmpg.c
> @@ -248,6 +248,57 @@ static int init_nvmpg_set_header(struct bch_nvmpg_ns *ns)
>  	return rc;
>  }
>  
> +static void __free_space(struct bch_nvmpg_ns *ns, unsigned long nvmpg_offset,
> +			 int order)
> +{
> +	unsigned long add_pages = (1L << order);
> +	pgoff_t pgoff;
> +	struct page *page;
> +	void *va;
> +
> +	if (nvmpg_offset == 0) {
> +		pr_err("free pages on offset 0\n");
> +		return;
> +	}
> +
> +	page = bch_nvmpg_va_to_pg(bch_nvmpg_offset_to_ptr(nvmpg_offset));
> +	WARN_ON((!page) || (page->private != order));
> +	pgoff = page->index;
> +
> +	while (order < BCH_MAX_ORDER - 1) {
> +		struct page *buddy_page;
> +
> +		pgoff_t buddy_pgoff = pgoff ^ (1L << order);
> +		pgoff_t parent_pgoff = pgoff & ~(1L << order);
> +
> +		if ((parent_pgoff + (1L << (order + 1)) > ns->pages_total))
> +			break;
> +
> +		va = bch_nvmpg_pgoff_to_ptr(ns, buddy_pgoff);
> +		buddy_page = bch_nvmpg_va_to_pg(va);
> +		WARN_ON(!buddy_page);
> +
> +		if (PageBuddy(buddy_page) && (buddy_page->private == order)) {
> +			list_del((struct list_head *)&buddy_page->zone_device_data);
> +			__ClearPageBuddy(buddy_page);
> +			pgoff = parent_pgoff;
> +			order++;
> +			continue;
> +		}
> +		break;
> +	}
> +
> +	va = bch_nvmpg_pgoff_to_ptr(ns, pgoff);
> +	page = bch_nvmpg_va_to_pg(va);
> +	WARN_ON(!page);
> +	list_add((struct list_head *)&page->zone_device_data,
> +		 &ns->free_area[order]);
> +	page->index = pgoff;
> +	set_page_private(page, order);
> +	__SetPageBuddy(page);
> +	ns->free += add_pages;
> +}

There are 3 WARN_ON's in here. If you absolutely must use a WARN_ON,
then make them WARN_ON_ONCE(). Ditto in other spots in this patch.

> +void bch_nvmpg_free_pages(unsigned long nvmpg_offset, int order,
> +			  const char *uuid)
> +{
> +	struct bch_nvmpg_ns *ns;
> +	struct bch_nvmpg_head *head;
> +	struct bch_nvmpg_recs *recs;
> +	int r;
> +
> +	mutex_lock(&global_nvmpg_set->lock);
> +
> +	ns = global_nvmpg_set->ns_tbl[BCH_NVMPG_GET_NS_ID(nvmpg_offset)];
> +	if (!ns) {
> +		pr_err("can't find namespace by given kaddr from namespace\n");
> +		goto unlock;
> +	}
> +
> +	head = find_nvmpg_head(uuid, false);
> +	if (!head) {
> +		pr_err("can't found bch_nvmpg_head by uuid\n");
> +		goto unlock;
> +	}
> +
> +	recs = find_nvmpg_recs(ns, head, false);
> +	if (!recs) {
> +		pr_err("can't find bch_nvmpg_recs by uuid\n");
> +		goto unlock;
> +	}
> +
> +	r = remove_nvmpg_rec(recs, ns->sb->this_ns, nvmpg_offset, order);
> +	if (r < 0) {
> +		pr_err("can't find bch_nvmpg_rec\n");
> +		goto unlock;
> +	}
> +
> +	__free_space(ns, nvmpg_offset, order);
> +
> +unlock:
> +	mutex_unlock(&global_nvmpg_set->lock);
> +}

Again tons of useless error prints. Make them return a proper error
instad of just making things void...

> @@ -686,6 +835,7 @@ struct bch_nvmpg_ns *bch_register_namespace(const char *dev_path)
>  	ns->pages_offset = sb->pages_offset;
>  	ns->pages_total = sb->pages_total;
>  	ns->sb = sb;
> +	/* increase by __free_space() */
>  	ns->free = 0;
>  	ns->bdev = bdev;
>  	ns->set = global_nvmpg_set;

Does that hunk belong in here?

-- 
Jens Axboe