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

Coly Li
2021-12-28T05:30:34Z

On 12/13/21 4:16 AM, Jens Axboe wrote:
> 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...

Copied. It will be improved as you suggested.

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

Could you please give me more detailed explanation of the above
question? Do you mention the bch_register_namespace() function, or the
code block which initializes members of ns?

Thank you.

Coly Li