On Tue, 24 Jun 2025 07:33:52 +0000
Dongsheng Yang <dongsheng.y...@linux.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.y...@linux.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);
> +}
