Re: [PATCH RFC md-6.16 v3 10/19] md/md-llbitmap: add data structure definition and comments

[Christoph Hellwig <hch@lst.de>]

On Mon, May 12, 2025 at 09:19:18AM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

You'll need a commit message here.  Also given how tiny this is
vs the rest of the file I'm not entirely sure there is much of a
point in splittng it out.

> +#include <linux/buffer_head.h>

This shouldn't be needed here I think.

> + * llbitmap state machine: transitions between states

Can you split the table below into two columns so that it's easily
readabe?

> +#define LLBITMAP_MAJOR_HI 6

I think you'll want to consolidate all the different version in
md-bitmap.h and document them.

> +#define BITMAP_MAX_SECTOR (128 * 2)

This appears unused even with the later series.

> +#define BITMAP_MAX_PAGES 32

Can you comment on how we ended up with this number?

> +#define BITMAP_SB_SIZE 1024

I'd add this to md-bitmap.h as it's part of the on-disk format, and
also make md-bitmap.c use it.

> +#define DEFAULT_DAEMON_SLEEP 30
> +
> +#define BARRIER_IDLE 5

Can you document these?

> +enum llbitmap_action {
> +	/* User write new data, this is the only acton from IO fast path */

s/acton/action/

> +/*
> + * page level barrier to synchronize between dirty bit by write IO and clean bit
> + * by daemon.

Overly lon line.  Also please stat full sentences with a capitalized
word.

> + */
> +struct llbitmap_barrier {
> +	char *data;

This is really a states array as far as I can tell.  Maybe name it
more descriptively and throw in a comment.

> +	struct percpu_ref active;
> +	unsigned long expire;
> +	unsigned long flags;
> +	/* Per block size dirty state, maximum 64k page / 512 sector = 128 */
> +	DECLARE_BITMAP(dirty, 128);
> +	wait_queue_head_t wait;
> +} ____cacheline_aligned_in_smp;
> +
> +struct llbitmap {
> +	struct mddev *mddev;
> +	int nr_pages;
> +	struct page *pages[BITMAP_MAX_PAGES];
> +	struct llbitmap_barrier barrier[BITMAP_MAX_PAGES];

Should the bitmap and the page be in the same array to have less
cache line sharing between the users/  The above
____cacheline_aligned_in_smp suggests you are at least somewhat
woerried about cache line sharing.

> +static char state_machine[nr_llbitmap_state][nr_llbitmap_action] = {
> +	[BitUnwritten] = {BitDirty, BitNone, BitNone, BitNone, BitNone, BitNone, BitNone, BitNone},

Maybe used named initializers to make this more readable.  In fact that
might remove the need for the big table in the comment at the top of the
file because it would be just as readable.