On Tue, 24 Jun 2025 07:33:48 +0000
Dongsheng Yang <dongsheng.y...@linux.dev> wrote:
> Consolidate common PCACHE helpers into a new header so that subsequent
> patches can include them without repeating boiler-plate.
>
> - Logging macros with unified prefix and location info.
> - Common constants (KB/MB helpers, metadata replica count, CRC seed).
> - On-disk metadata header definition and CRC helper.
> - Sequence-number comparison that handles wrap-around.
> - pcache_meta_find_latest() to pick the newest valid metadata copy.
>
> Signed-off-by: Dongsheng Yang <dongsheng.y...@linux.dev>
Hi,
I'm taking a look out of curiosity only as this is far from an area I'm
confident in. So comments will be mostly superficial.
> ---
> drivers/md/dm-pcache/pcache_internal.h | 116 +++++++++++++++++++++++++
As a general rule I'd much rather see a header built up alongside the
code that uses it rather than as a separate patch at the start.
> 1 file changed, 116 insertions(+)
> create mode 100644 drivers/md/dm-pcache/pcache_internal.h
>
> diff --git a/drivers/md/dm-pcache/pcache_internal.h
> b/drivers/md/dm-pcache/pcache_internal.h
> new file mode 100644
> index 000000000000..4d3b55a22638
> --- /dev/null
> +++ b/drivers/md/dm-pcache/pcache_internal.h
> @@ -0,0 +1,116 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef _PCACHE_INTERNAL_H
> +#define _PCACHE_INTERNAL_H
> +
> +#include <linux/delay.h>
> +#include <linux/crc32c.h>
> +
> +#define pcache_err(fmt, ...)
> \
> + pr_err("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
> +#define pcache_info(fmt, ...)
> \
> + pr_info("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
> +#define pcache_debug(fmt, ...)
> \
> + pr_debug("dm-pcache: %s:%u " fmt, __func__, __LINE__, ##__VA_ARGS__)
Use pr_fmt() in appropriate files and drop these.
> +
> +#define PCACHE_KB (1024ULL)
> +#define PCACHE_MB (1024 * PCACHE_KB)
Generally avoid defines where they just match the numbers. 1024 * 1024 is
commonly used directly in kernel code as everyone can see it's a MiB.
> +
> +/* Maximum number of metadata indices */
> +#define PCACHE_META_INDEX_MAX 2
> +
> +#define PCACHE_CRC_SEED 0x3B15A
> +/*
> + * struct pcache_meta_header - PCACHE metadata header structure
> + * @crc: CRC checksum for validating metadata integrity.
> + * @seq: Sequence number to track metadata updates.
> + * @version: Metadata version.
> + * @res: Reserved space for future use.
> + */
> +struct pcache_meta_header {
> + __u32 crc;
> + __u8 seq;
> + __u8 version;
> + __u16 res;
Why not u32 and friends given this seems to be internal to the kernel?
> +};
> +
> +/*
You've formatted most of this stuff as kernel-doc so for all of them use
/**
And check for any warnings or errors using scripts/kernel-doc
It's a good way to pick up on subtle typos etc in docs that a reviewr
might miss.
> + * pcache_meta_crc - Calculate CRC for the given metadata header.
> + * @header: Pointer to the metadata header.
> + * @meta_size: Size of the metadata structure.
> + *
> + * Returns the CRC checksum calculated by excluding the CRC field itself.
> + */
> +static inline u32 pcache_meta_crc(struct pcache_meta_header *header, u32
> meta_size)
> +{
> + return crc32c(PCACHE_CRC_SEED, (void *)header + 4, meta_size - 4);
> +}
> +
> +/*
> + * pcache_meta_seq_after - Check if a sequence number is more recent,
> accounting for overflow.
> + * @seq1: First sequence number.
> + * @seq2: Second sequence number.
> + *
> + * Determines if @seq1 is more recent than @seq2 by calculating the signed
> + * difference between them. This approach allows handling sequence number
> + * overflow correctly because the difference wraps naturally, and any value
> + * greater than zero indicates that @seq1 is "after" @seq2. This method
> + * assumes 8-bit unsigned sequence numbers, where the difference wraps
> + * around if seq1 overflows past seq2.
I'd state the assumption behind this which think is that we will never
have a sequence number getting ahead by more than 128 values.
> + *
> + * Returns:
> + * - true if @seq1 is more recent than @seq2, indicating it comes "after"
> + * - false otherwise.
> + */
> +static inline bool pcache_meta_seq_after(u8 seq1, u8 seq2)
> +{
> + return (s8)(seq1 - seq2) > 0;
> +}
> +
> +/*
> + * pcache_meta_find_latest - Find the latest valid metadata.
> + * @header: Pointer to the metadata header.
> + * @meta_size: Size of each metadata block.
> + *
> + * Finds the latest valid metadata by checking sequence numbers. If a
> + * valid entry with the highest sequence number is found, its pointer
> + * is returned. Returns NULL if no valid metadata is found.
> + */
> +static inline void __must_check *pcache_meta_find_latest(struct
> pcache_meta_header *header,
> + u32 meta_size, u32 meta_max_size,
> + void *meta_ret)
Not sure why you can't type this as pcache_meta_header. Maybe that will
become obvious later int he series.
> +{
> + struct pcache_meta_header *meta, *latest = NULL;
Combining declarations that assign and those that don't is not greate
for readability. Also why not set meta where you declare it?
> + u32 i, seq_latest = 0;
> + void *meta_addr;
> +
> + meta = meta_ret;
> +
> + for (i = 0; i < PCACHE_META_INDEX_MAX; i++) {
> + meta_addr = (void *)header + (i * meta_max_size);
Brackets around i * meta_max_size not needed. Whilst we can't all remember
precedence of all operators, + and * are reasonable to assume.
> + if (copy_mc_to_kernel(meta, meta_addr, meta_size)) {
> + pcache_err("hardware memory error when copy meta");
> + return ERR_PTR(-EIO);
> + }
> +
> + /* Skip if CRC check fails */
Good to say why skipping is the right choice perhaps.
> + if (meta->crc != pcache_meta_crc(meta, meta_size))
> + continue;
> +
> + /* Update latest if a more recent sequence is found */
> + if (!latest || pcache_meta_seq_after(meta->seq, seq_latest)) {
> + seq_latest = meta->seq;
> + latest = (void *)header + (i * meta_max_size);
> + }
> + }
> +
> + if (latest) {
I'd flip
if (!latest)
return 0;
if (copy...)
> + if (copy_mc_to_kernel(meta_ret, latest, meta_size)) {
> + pcache_err("hardware memory error");
> + return ERR_PTR(-EIO);
> + }
> + }
> +
> + return latest;
> +}
> +
> +#endif /* _PCACHE_INTERNAL_H */
