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

["Ma, Jianpeng" <jianpeng.ma@intel.com>]

> -----Original Message-----
> From: yukuai (C) <yukuai3@huawei.com>
> Sent: Monday, February 21, 2022 8:36 PM
> To: Li, Coly <colyli@suse.de>; axboe@kernel.dk
> Cc: linux-bcache@vger.kernel.org; linux-block@vger.kernel.org; Ma, Jianpeng
> <jianpeng.ma@intel.com>; Ren, Qiaowei <qiaowei.ren@intel.com>; Christoph
> Hellwig <hch@lst.de>; Williams, Dan J <dan.j.williams@intel.com>; Hannes
> Reinecke <hare@suse.de>
> Subject: Re: [PATCH v13 05/12] bcache: bch_nvmpg_free_pages() of the buddy
> allocator
> 
> 在 2021/12/13 1:05, Coly Li 写道:
> > From: Jianpeng Ma <jianpeng.ma@intel.com>
> >
> > This patch implements the bch_nvmpg_free_pages() of the buddy allocator.
> >
> > The difference between this and page-buddy-free:
> > it need owner_uuid to free owner allocated pages, and must persistent
> > after free.
> >
> > Signed-off-by: Jianpeng Ma <jianpeng.ma@intel.com>
> > Co-developed-by: Qiaowei Ren <qiaowei.ren@intel.com>
> > Signed-off-by: Qiaowei Ren <qiaowei.ren@intel.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > ---
> >   drivers/md/bcache/nvmpg.c | 164
> ++++++++++++++++++++++++++++++++++++--
> >   drivers/md/bcache/nvmpg.h |   3 +
> >   2 files changed, 160 insertions(+), 7 deletions(-)
> >
> > 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;
> > +}
> > +
> >   static void bch_nvmpg_init_free_space(struct bch_nvmpg_ns *ns)
> >   {
> >   	unsigned int start, end, pages;
> > @@ -261,21 +312,19 @@ static void bch_nvmpg_init_free_space(struct
> bch_nvmpg_ns *ns)
> >   		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);
> > +			page = bch_nvmpg_va_to_pg(
> > +					bch_nvmpg_pgoff_to_ptr(ns, pgoff_start));
> >   			set_page_private(page, i);
> >   			page->index = pgoff_start;
> > -			__SetPageBuddy(page);
> > -			list_add((struct list_head *)&page->zone_device_data,
> > -				 &ns->free_area[i]);
> > +
> > +			/* In order to update ns->free */
> > +			__free_space(ns, pgoff_start, i);
> 
> Hi,
> 
> While testing this patchset, we found that this is probably wrong, pgoff_start
> represent page number here, however, __free_space expect this to be page
> offset. Maybe this should be:
> 
> __free_space(ns, pgoff_start << PAGE_SHIFT, i);
> 
Yes，you are right. I'll fix in the next version.

Thanks!
Jianpeng.
> Thanks,
> Kuai