Re: [PATCH v1 04/11] dm-pcache: add segment layer

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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

> Introduce segment.{c,h}, an internal abstraction that encapsulates
> everything related to a single pcache *segment* (the fixed-size
> allocation unit stored on the cache-device).
> 
> * On-disk metadata (`struct pcache_segment_info`)
>   - Embedded `struct pcache_meta_header` for CRC/sequence handling.
>   - `flags` field encodes a “has-next” bit and a 4-bit *type* class
>     (`CACHE_DATA` added as the first type).
> 
> * Initialisation
>   - `pcache_segment_init()` populates the in-memory
>     `struct pcache_segment` from a given segment id, data offset and
>     metadata pointer, computing the usable `data_size` and virtual
>     address within the DAX mapping.
> 
> * IO helpers
>   - `segment_copy_to_bio()` / `segment_copy_from_bio()` move data
>     between pmem and a bio, using `_copy_mc_to_iter()` and
>     `_copy_from_iter_flushcache()` to tolerate hw memory errors and
>     ensure durability.
>   - `segment_pos_advance()` advances an internal offset while staying
>     inside the segment’s data area.
> 
> These helpers allow upper layers (cache key management, write-back
> logic, GC, etc.) to treat a segment as a contiguous byte array without
> knowing about DAX mappings or persistence details.
> 
> Signed-off-by: Dongsheng Yang <dongsheng.yang@linux.dev>
Hi

Just one trivial comment.

> diff --git a/drivers/md/dm-pcache/segment.h b/drivers/md/dm-pcache/segment.h
> new file mode 100644
> index 000000000000..9675951ffaf8
> --- /dev/null
> +++ b/drivers/md/dm-pcache/segment.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _PCACHE_SEGMENT_H
> +#define _PCACHE_SEGMENT_H
> +
> +#include <linux/bio.h>
> +
> +#include "pcache_internal.h"
> +
> +struct pcache_segment_info {
> +	struct pcache_meta_header	header;	/* Metadata header for the segment */

The comment is fairly obvious given the type of the field. I'd drop the comment.

> +	__u32			flags;
> +	__u32			next_seg;
> +};