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