Hi,
在 2025/05/12 13:15, Christoph Hellwig 写道:
On Mon, May 12, 2025 at 09:19:19AM +0800, Yu Kuai wrote:
+static bool is_raid456(struct mddev *mddev)
+{
+ return (mddev->level == 4 || mddev->level == 5 || mddev->level == 6);
+}
This really should be in a common helper somewhere..
Perhaps md.h?
+static int llbitmap_read(struct llbitmap *llbitmap, enum llbitmap_state *state,
+ loff_t pos)
+{
+ pos += BITMAP_SB_SIZE;
+ *state = llbitmap->barrier[pos >> PAGE_SHIFT].data[offset_in_page(pos)];
+ return 0;
+}
This always return 0, and could just return void.
Ok
+static void llbitmap_set_page_dirty(struct llbitmap *llbitmap, int idx, int offset)
Overly long line.
Also should the second and third argument be unsigned?
Ok
+ /*
+ * if the bit is already dirty, or other page bytes is the same bit is
+ * already BitDirty, then mark the whole bytes in the bit as dirty
+ */
+ if (test_and_set_bit(bit, barrier->dirty)) {
+ infectious = true;
+ } else {
+ for (pos = bit * io_size; pos < (bit + 1) * io_size - 1;
+ pos++) {
+ if (pos == offset)
+ continue;
+ if (barrier->data[pos] == BitDirty ||
+ barrier->data[pos] == BitNeedSync) {
+ infectious = true;
+ break;
+ }
+ }
+
+ }
+ if (!infectious)
+ return;
Mabe use a goto and/or a helper function containing the for loop to
clean up the control flow here a bit?
Ok, I;ll add a helper function.
+static int llbitmap_write(struct llbitmap *llbitmap, enum llbitmap_state state,
+ loff_t pos)
+{
+ int idx;
+ int offset;
Unsigned?
+
+ pos += BITMAP_SB_SIZE;
+ idx = pos >> PAGE_SHIFT;
+ offset = offset_in_page(pos);
+
+ llbitmap->barrier[idx].data[offset] = state;
+ if (state == BitDirty || state == BitNeedSync)
+ llbitmap_set_page_dirty(llbitmap, idx, offset);
+ return 0;
and this could also be a void return.
Ok.
+ sector = mddev->bitmap_info.offset + (idx << PAGE_SECTORS_SHIFT);
Overly long line.
+ if (rdev->raid_disk < 0 || test_bit(Faulty, &rdev->flags))
Same here.
+ int nr_pages = (llbitmap->chunks + BITMAP_SB_SIZE + PAGE_SIZE - 1) / PAGE_SIZE;
Unsigned for the type, and DIV_ROUND_UP for the calculation.
Ok.
+ struct page *page;
+ int i = 0;
+
+ llbitmap->nr_pages = nr_pages;
+ while (i < nr_pages) {
+ page = llbitmap_read_page(llbitmap, i);
+ if (IS_ERR(page)) {
+ llbitmap_free_pages(llbitmap);
+ return PTR_ERR(page);
+ }
+
+ if (percpu_ref_init(&llbitmap->barrier[i].active, active_release,
+ PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
+ __free_page(page);
+ return -ENOMEM;
Doesn't this also need a llbitmap_free_pages for the error case?
Of course, my bad that I forgot this.
Thanks again for the review!
Kuai
.
