Message-ID: <20250701155928.0000160a@huawei.com>
Date: Tue, 1 Jul 2025 15:59:28 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Dongsheng Yang <dongsheng.yang@...ux.dev>
CC: <mpatocka@...hat.com>, <agk@...hat.com>, <snitzer@...nel.org>,
	<axboe@...nel.dk>, <hch@....de>, <dan.j.williams@...el.com>,
	<linux-block@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-cxl@...r.kernel.org>, <nvdimm@...ts.linux.dev>,
	<dm-devel@...ts.linux.dev>
Subject: Re: [PATCH v1 05/11] dm-pcache: add cache_segment

On Tue, 24 Jun 2025 07:33:52 +0000
Dongsheng Yang <dongsheng.yang@...ux.dev> wrote:

> Introduce *cache_segment.c*, the in-memory/on-disk glue that lets a
> `struct pcache_cache` manage its array of data segments.
> 
> * Metadata handling
>   - Loads the most-recent replica of both the segment-info block
>     (`struct pcache_segment_info`) and per-segment generation counter
>     (`struct pcache_cache_seg_gen`) using `pcache_meta_find_latest()`.
>   - Updates those structures atomically with CRC + sequence rollover,
>     writing alternately to the two metadata slots inside each segment.
> 
> * Segment initialisation (`cache_seg_init`)
>   - Builds a `struct pcache_segment` pointing to the segment’s data
>     area, sets up locks, generation counters, and, when formatting a new
>     cache, zeroes the on-segment kset header.
> 
> * Linked-list of segments
>   - `cache_seg_set_next_seg()` stores the *next* segment id in
>     `seg_info->next_seg` and sets the HAS_NEXT flag, allowing a cache to
>     span multiple segments. This is important to allow other type of
>     segment added in future.
> 
> * Runtime life-cycle
>   - Reference counting (`cache_seg_get/put`) with invalidate-on-last-put
>     that clears the bitmap slot and schedules cleanup work.
>   - Generation bump (`cache_seg_gen_increase`) persists a new generation
>     record whenever the segment is modified.
> 
> * Allocator
>   - `get_cache_segment()` uses a bitmap and per-cache hint to pick the
>     next free segment, retrying with micro-delays when none are
>     immediately available.
> 
> Signed-off-by: Dongsheng Yang <dongsheng.yang@...ux.dev>
Minor stuff inline.

> ---
>  drivers/md/dm-pcache/cache_segment.c | 293 +++++++++++++++++++++++++++
>  1 file changed, 293 insertions(+)
>  create mode 100644 drivers/md/dm-pcache/cache_segment.c
> 
> diff --git a/drivers/md/dm-pcache/cache_segment.c b/drivers/md/dm-pcache/cache_segment.c
> new file mode 100644
> index 000000000000..298f881874d1
> --- /dev/null
> +++ b/drivers/md/dm-pcache/cache_segment.c
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include "cache_dev.h"
> +#include "cache.h"
> +#include "backing_dev.h"
> +#include "dm_pcache.h"
> +
> +static inline struct pcache_segment_info *get_seg_info_addr(struct pcache_cache_segment *cache_seg)
> +{
> +	struct pcache_segment_info *seg_info_addr;
> +	u32 seg_id = cache_seg->segment.seg_id;
> +	void *seg_addr;
> +
> +	seg_addr = CACHE_DEV_SEGMENT(cache_seg->cache->cache_dev, seg_id);
> +	seg_info_addr = seg_addr + PCACHE_SEG_INFO_SIZE * cache_seg->info_index;
> +
> +	return seg_info_addr;

Little point in this local variable.

	return seg_addr + PCACHE_SEG_INFO_SIZE * cache_seg->info_index;

> +}
> +
> +static void cache_seg_info_write(struct pcache_cache_segment *cache_seg)
> +{
> +	struct pcache_segment_info *seg_info_addr;
> +	struct pcache_segment_info *seg_info = &cache_seg->cache_seg_info;
> +
> +	mutex_lock(&cache_seg->info_lock);

guard() here to avoid need to release below.

> +	seg_info->header.seq++;
> +	seg_info->header.crc = pcache_meta_crc(&seg_info->header, sizeof(struct pcache_segment_info));
> +
> +	seg_info_addr = get_seg_info_addr(cache_seg);
> +	memcpy_flushcache(seg_info_addr, seg_info, sizeof(struct pcache_segment_info));
> +	pmem_wmb();
> +
> +	cache_seg->info_index = (cache_seg->info_index + 1) % PCACHE_META_INDEX_MAX;
> +	mutex_unlock(&cache_seg->info_lock);
> +}
> +
> +static int cache_seg_info_load(struct pcache_cache_segment *cache_seg)
> +{
> +	struct pcache_segment_info *cache_seg_info_addr_base, *cache_seg_info_addr;
> +	struct pcache_cache_dev *cache_dev = cache_seg->cache->cache_dev;
> +	struct dm_pcache *pcache = CACHE_DEV_TO_PCACHE(cache_dev);
> +	u32 seg_id = cache_seg->segment.seg_id;
> +	int ret = 0;
> +
> +	cache_seg_info_addr_base = CACHE_DEV_SEGMENT(cache_dev, seg_id);
> +
> +	mutex_lock(&cache_seg->info_lock);

As below guard() will improve this code though you will need to change how the error
message is printed.

> +	cache_seg_info_addr = pcache_meta_find_latest(&cache_seg_info_addr_base->header,
> +						sizeof(struct pcache_segment_info),
> +						PCACHE_SEG_INFO_SIZE,
> +						&cache_seg->cache_seg_info);
> +	if (IS_ERR(cache_seg_info_addr)) {
> +		ret = PTR_ERR(cache_seg_info_addr);
> +		goto out;
> +	} else if (!cache_seg_info_addr) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +	cache_seg->info_index = cache_seg_info_addr - cache_seg_info_addr_base;
> +out:
> +	mutex_unlock(&cache_seg->info_lock);
> +
> +	if (ret)
> +		pcache_dev_err(pcache, "can't read segment info of segment: %u, ret: %d\n",
> +			      cache_seg->segment.seg_id, ret);
> +	return ret;
> +}
> +
> +static int cache_seg_ctrl_load(struct pcache_cache_segment *cache_seg)
> +{
> +	struct pcache_cache_seg_ctrl *cache_seg_ctrl = cache_seg->cache_seg_ctrl;
> +	struct pcache_cache_seg_gen cache_seg_gen, *cache_seg_gen_addr;

Don't mix pointer and none pointer in one line of declaration. Just
a tiny bit tricky to read.


> +	int ret = 0;
> +
> +	mutex_lock(&cache_seg->ctrl_lock);

guard() to allow returns on error paths without worrying about the lock
being released as leaving the scope will do it for you.


> +	cache_seg_gen_addr = pcache_meta_find_latest(&cache_seg_ctrl->gen->header,
> +					     sizeof(struct pcache_cache_seg_gen),
sizeof(cache_seg_gen) perhaps?
> +					     sizeof(struct pcache_cache_seg_gen),
> +					     &cache_seg_gen);
> +	if (IS_ERR(cache_seg_gen_addr)) {
> +		ret = PTR_ERR(cache_seg_gen_addr);
> +		goto out;
> +	}
> +
> +	if (!cache_seg_gen_addr) {
> +		cache_seg->gen = 0;
> +		cache_seg->gen_seq = 0;
> +		cache_seg->gen_index = 0;
> +		goto out;
> +	}
> +
> +	cache_seg->gen = cache_seg_gen.gen;
> +	cache_seg->gen_seq = cache_seg_gen.header.seq;
> +	cache_seg->gen_index = (cache_seg_gen_addr - cache_seg_ctrl->gen);
> +out:
> +	mutex_unlock(&cache_seg->ctrl_lock);
> +
> +	return ret;
> +}

> +
> +static void cache_seg_ctrl_write(struct pcache_cache_segment *cache_seg)
> +{
> +	struct pcache_cache_seg_gen cache_seg_gen;
> +
> +	mutex_lock(&cache_seg->ctrl_lock);

Consider guard(mutex)()

> +	cache_seg_gen.gen = cache_seg->gen;
> +	cache_seg_gen.header.seq = ++cache_seg->gen_seq;
> +	cache_seg_gen.header.crc = pcache_meta_crc(&cache_seg_gen.header,
> +						 sizeof(struct pcache_cache_seg_gen));
> +
> +	memcpy_flushcache(get_cache_seg_gen_addr(cache_seg), &cache_seg_gen, sizeof(struct pcache_cache_seg_gen));
> +	pmem_wmb();
> +
> +	cache_seg->gen_index = (cache_seg->gen_index + 1) % PCACHE_META_INDEX_MAX;
> +	mutex_unlock(&cache_seg->ctrl_lock);
> +}

> +
> +static int cache_seg_meta_load(struct pcache_cache_segment *cache_seg)
> +{
> +	int ret;
> +
> +	ret = cache_seg_info_load(cache_seg);
> +	if (ret)
> +		goto err;

		return ret;

in these paths simpler to follow.  If DM always does this pattern fair enough
to follow local style.

> +
> +	ret = cache_seg_ctrl_load(cache_seg);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +err:
> +	return ret;
> +}

> +int cache_seg_init(struct pcache_cache *cache, u32 seg_id, u32 cache_seg_id,
> +		   bool new_cache)
> +{
> +	struct pcache_cache_dev *cache_dev = cache->cache_dev;
> +	struct pcache_cache_segment *cache_seg = &cache->segments[cache_seg_id];
> +	struct pcache_segment_init_options seg_options = { 0 };
> +	struct pcache_segment *segment = &cache_seg->segment;
> +	int ret;
> +
> +	cache_seg->cache = cache;
> +	cache_seg->cache_seg_id = cache_seg_id;
> +	spin_lock_init(&cache_seg->gen_lock);
> +	atomic_set(&cache_seg->refs, 0);
> +	mutex_init(&cache_seg->info_lock);
> +	mutex_init(&cache_seg->ctrl_lock);
> +
> +	/* init pcache_segment */
> +	seg_options.type = PCACHE_SEGMENT_TYPE_CACHE_DATA;
> +	seg_options.data_off = PCACHE_CACHE_SEG_CTRL_OFF + PCACHE_CACHE_SEG_CTRL_SIZE;
> +	seg_options.seg_id = seg_id;
> +	seg_options.seg_info = &cache_seg->cache_seg_info;
> +	pcache_segment_init(cache_dev, segment, &seg_options);
> +
> +	cache_seg->cache_seg_ctrl = CACHE_DEV_SEGMENT(cache_dev, seg_id) + PCACHE_CACHE_SEG_CTRL_OFF;
> +
> +	if (new_cache) {
I'd flip logic so
	if (!new_cache)
		return cache_seg_meta_load(cache_seg);

	cache_dev_zero_range()
etc.

Sometimes it's better to exist quickly in the simple case and have
the straight line code deal with the more complex stuff (indented a little less)

> +		cache_dev_zero_range(cache_dev, CACHE_DEV_SEGMENT(cache_dev, seg_id),
> +				     PCACHE_SEG_INFO_SIZE * PCACHE_META_INDEX_MAX +
> +				     PCACHE_CACHE_SEG_CTRL_SIZE);
> +
> +		cache_seg_ctrl_init(cache_seg);
> +
> +		cache_seg->info_index = 0;
> +		cache_seg_info_write(cache_seg);
> +
> +		/* clear outdated kset in segment */
> +		memcpy_flushcache(segment->data, &pcache_empty_kset, sizeof(struct pcache_cache_kset_onmedia));
> +		pmem_wmb();
> +	} else {
> +		ret = cache_seg_meta_load(cache_seg);
> +		if (ret)
> +			goto err;

In this case we return immediately whether good or bad, so

		return cache_seg_meta_load(cache_seg);

> +	}
> +
> +	return 0;
> +err:
> +	return ret;
As in earlier patch reviews, don't have a label that is just return.  Return instead
of the gotos.

> +}


> +static void cache_seg_invalidate(struct pcache_cache_segment *cache_seg)
> +{
> +	struct pcache_cache *cache;

Might as well do the more compact
	struct pcache_cache *cache = cache_seg->cache;

> +
> +	cache = cache_seg->cache;
> +	cache_seg_gen_increase(cache_seg);
> +
> +	spin_lock(&cache->seg_map_lock);
> +	if (cache->cache_full)
> +		cache->cache_full = false;

Perhaps just write cache->cache_full = false unconditionally?
If there is a reason to not do the write, then add a comment here.

> +	clear_bit(cache_seg->cache_seg_id, cache->seg_map);
> +	spin_unlock(&cache->seg_map_lock);
> +
> +	pcache_defer_reqs_kick(CACHE_TO_PCACHE(cache));
> +	/* clean_work will clean the bad key in key_tree*/
> +	queue_work(cache_get_wq(cache), &cache->clean_work);
> +}

Powered by blists - more mailing lists