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

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

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