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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <89e306d4-bb30-96bd-0fe0-a5ff4e8012ef@huaweicloud.com>
Date: Mon, 12 May 2025 16:05:58 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Christoph Hellwig <hch@....de>, Yu Kuai <yukuai1@...weicloud.com>
Cc: xni@...hat.com, colyli@...nel.org, agk@...hat.com, snitzer@...nel.org,
 mpatocka@...hat.com, song@...nel.org, linux-kernel@...r.kernel.org,
 dm-devel@...ts.linux.dev, linux-raid@...r.kernel.org, yi.zhang@...wei.com,
 yangerkun@...wei.com, johnny.chenyi@...wei.com,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH RFC md-6.16 v3 10/19] md/md-llbitmap: add data structure
 definition and comments

Hi,

在 2025/05/12 13:09, Christoph Hellwig 写道:
> On Mon, May 12, 2025 at 09:19:18AM +0800, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> Signed-off-by: Yu Kuai <yukuai3@...wei.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.

I'm not sure, this way is simpiler, however do you prefer add structure
and comments by following patches gradually?
> 
>> +#include <linux/buffer_head.h>
> 
> This shouldn't be needed here I think.

Yes, I just copy the headers from old bitmap file.
> 
>> + * llbitmap state machine: transitions between states
> 
> Can you split the table below into two columns so that it's easily
> readabe?

Sure
> 
>> +#define LLBITMAP_MAJOR_HI 6
> 
> I think you'll want to consolidate all the different version in
> md-bitmap.h and document them.

Ok, then I'll also move other values from md-bitmap.c.

> 
>> +#define BITMAP_MAX_SECTOR (128 * 2)
> 
> This appears unused even with the later series.

Yes, this is used in the old version, I'll remove it.
> 
>> +#define BITMAP_MAX_PAGES 32
> 
> Can you comment on how we ended up with this number?

Ok, the bitmap superblock is created by mdadm, and mdadm limit
bitmap size max to 128k, then
> 
>> +#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.

Ok
> 
>> +#define DEFAULT_DAEMON_SLEEP 30
>> +
>> +#define BARRIER_IDLE 5
> 
> Can you document these?

Ok, this is used for daemon to clean dirty bits when user doesn't issue
IO to the bit for more than 5 seconds.
> 
>> +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.

How about llbitmap_page_ctl ?
> 
>> +	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.

Yes, and BTW, I probably should allocate necessary memory based on real
number of pages, instead of embedded max pages. I do this first to
eazy coding.

> 
>> +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.

Sure, this is a good suggestion.

Thanks for the review!
Kuai

> 
> .
>