Re: [PATCH v13 04/12] bcache: bch_nvmpg_alloc_pages() of the buddy

[Coly Li <colyli@suse.de>]

On 12/13/21 4:14 AM, Jens Axboe wrote:
> On 12/12/21 10:05 AM, Coly Li wrote:
>> +/* If not found, it will create if create == true */
>> +static struct bch_nvmpg_head *find_nvmpg_head(const char *uuid, bool create)
>> +{
>> +	struct bch_nvmpg_set_header *set_header = global_nvmpg_set->set_header;
>> +	struct bch_nvmpg_head *head = NULL;
>> +	int i;
>> +
>> +	if (set_header == NULL)
>> +		goto out;
>> +
>> +	for (i = 0; i < set_header->size; i++) {
>> +		struct bch_nvmpg_head *h = &set_header->heads[i];
>> +
>> +		if (h->state != BCH_NVMPG_HD_STAT_ALLOC)
>> +			continue;
>> +
>> +		if (!memcmp(uuid, h->uuid, 16)) {
>> +			head = h;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!head && create) {
>> +		u32 used = set_header->used;
>> +
>> +		if (set_header->size > used) {
>> +			head = &set_header->heads[used];
>> +			memset(head, 0, sizeof(struct bch_nvmpg_head));
>> +			head->state = BCH_NVMPG_HD_STAT_ALLOC;
>> +			memcpy(head->uuid, uuid, 16);
>> +			global_nvmpg_set->heads_used++;
>> +			set_header->used++;
>> +		} else
>> +			pr_info("No free bch_nvmpg_head\n");
>> +	}
> Use {} consistently. Again probably just some printk that should go
> away.

Copied.

>> +static struct bch_nvmpg_recs *find_nvmpg_recs(struct bch_nvmpg_ns *ns,
>> +					      struct bch_nvmpg_head *head,
>> +					      bool create)
>> +{
>> +	int ns_id = ns->sb->this_ns;
>> +	struct bch_nvmpg_recs *prev_recs = NULL, *recs = NULL;
>> +
>> +	recs = bch_nvmpg_offset_to_ptr(head->recs_offset[ns_id]);
>> +
>> +	/* If create=false, we return recs[nr] */
>> +	if (!create)
>> +		return recs;
> Would this be cleaner to handle in the caller?

Cure, I will suggest Jianpeng and Qiaowei to change this.

>> +static void add_nvmpg_rec(struct bch_nvmpg_ns *ns,
>> +			  struct bch_nvmpg_recs *recs,
>> +			  unsigned long nvmpg_offset,
>> +			  int order)
>> +{
>> +	int i, ns_id;
>> +	unsigned long pgoff;
>> +
>> +	pgoff = bch_nvmpg_offset_to_pgoff(nvmpg_offset);
>> +	ns_id = ns->sb->this_ns;
>> +
>> +	for (i = 0; i < recs->size; i++) {
>> +		if (recs->recs[i].pgoff == 0) {
>> +			recs->recs[i].pgoff = pgoff;
>> +			recs->recs[i].order = order;
>> +			recs->recs[i].ns_id = ns_id;
>> +			recs->used++;
>> +			break;
>> +		}
>> +	}
>> +	BUG_ON(i == recs->size);
> No BUG_ON's, please. It only truly belongs in core code for cases where
> error handling isn't possible, does not apply here.

It is because currently only 1 single record allocated for bcache 
journal, and if i == recs->size happens it means the on-NVDIMM struct 
bch_nvmpg_recs is corrupted.

Currently we are working on storing Btree nodes on NVDIMM, such BUG_ON() 
is dropped.


>> diff --git a/drivers/md/bcache/nvmpg.h b/drivers/md/bcache/nvmpg.h
>> index 55778d4db7da..d03f3241b45a 100644
>> --- a/drivers/md/bcache/nvmpg.h
>> +++ b/drivers/md/bcache/nvmpg.h
>> @@ -76,6 +76,9 @@ struct bch_nvmpg_set {
>>   /* Indicate which field in bch_nvmpg_sb to be updated */
>>   #define BCH_NVMPG_TOTAL_NS	0	/* total_ns */
>>   
>> +#define BCH_PGOFF_TO_KVADDR(pgoff)					\
>> +	((void *)((unsigned long)(pgoff) << PAGE_SHIFT))
> Pretty sure we have a general kernel helper for this, better to use that
> rather than duplicate it.
>
>
Copied. Thank for pointing out this.

Coly Li