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..
> +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.
> +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?
> + /*
> + * 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?
> +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.
> + 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.
> + 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?
