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

[Dongsheng Yang <dongsheng.yang@linux.dev>]

在 7/1/2025 10:46 PM, Jonathan Cameron 写道:
> 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.

Hi Jonathan,

Sounds not bad, will change it.

Thanx

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