Re: [PATCH v1 01/11] dm-pcache: add pcache_internal.h

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

On Tue, 24 Jun 2025 07:33:48 +0000
Dongsheng Yang <dongsheng.yang@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.yang@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 */