Re: [PATCH 3/5] md: add fallback to correct bitmap_ops on version mismatch

[Yu Kuai]

Hi,

在 2026/2/17 16:54, Su Yue 写道:
> On Sat 14 Feb 2026 at 14:10, Yu Kuai <yukuai@xxxxxxxxx> wrote:
>
>> If default bitmap version and on-disk version doesn't match, and mdadm
>> is not the latest version to set bitmap_type, set bitmap_ops based on
>> the disk version.
>>
> Why not just let old version mdadm fails  since llbitmap is a new 
> feature.

The original use case is that we found llbitmap array fails to assemble in
some corner cases, and with the respect I'm not quite familiar with mdadm
code, so I think this patch is the best solution for now.

On the other hand, this should also be helpful if we decide to make llbitmap
the default option in the future.

>
>> Signed-off-by: Yu Kuai <yukuai@xxxxxxxxx>
>> ---
>>  drivers/md/md.c | 103  +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 59cd303548de..d2607ed5c2e9 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -6447,15 +6447,116 @@ static void md_safemode_timeout(struct 
>> timer_list *t)
>>
>>  static int start_dirty_degraded;
>>
>> +/*
>> + * Read bitmap superblock and return the bitmap_id based on disk 
>> version.
>> + * This is used as fallback when default bitmap version and on-disk 
>> version
>> + * doesn't match, and mdadm is not the latest version to set 
>> bitmap_type.
>> + */
>> +static enum md_submodule_id md_bitmap_get_id_from_sb(struct mddev 
>> *mddev)
>> +{
>> +    struct md_rdev *rdev;
>> +    struct page *sb_page;
>> +    bitmap_super_t *sb;
>> +    enum md_submodule_id id = ID_BITMAP_NONE;
>> +    sector_t sector;
>> +    u32 version;
>> +
>> +    if (!mddev->bitmap_info.offset)
>> +        return ID_BITMAP_NONE;
>> +
>> +    sb_page = alloc_page(GFP_KERNEL);
>> +    if (!sb_page)
>> +        return ID_BITMAP_NONE;
>> +
>>
> Personally I don't like the way treating error as ID_BITMAP_NONE.
> When wrong things happen everything looks fine, no error code, no 
> error message.

Ok, sounds reasonable.

>
>> +    sector = mddev->bitmap_info.offset;
>> +
>> +    rdev_for_each(rdev, mddev) {
>> +        u32 iosize;
>> +
>> +        if (!test_bit(In_sync, &rdev->flags) ||
>> +            test_bit(Faulty, &rdev->flags) ||
>> +            test_bit(Bitmap_sync, &rdev->flags))
>> +            continue;
>> +
>> +        iosize = roundup(sizeof(bitmap_super_t),
>> +                 bdev_logical_block_size(rdev->bdev));
>> +        if (sync_page_io(rdev, sector, iosize, sb_page, REQ_OP_READ,
>> +                 true))
>> +            goto read_ok;
>> +    }
>>
> And here.
>
>> +    goto out;
>> +
>> +read_ok:
>> +    sb = kmap_local_page(sb_page);
>> +    if (sb->magic != cpu_to_le32(BITMAP_MAGIC))
>> +        goto out_unmap;
>> +
>> +    version = le32_to_cpu(sb->version);
>> +    switch (version) {
>> +    case BITMAP_MAJOR_LO:
>> +    case BITMAP_MAJOR_HI:
>> +    case BITMAP_MAJOR_CLUSTERED:
>>
> For BITMAP_MAJOR_CLUSTERED, why not ID_CLUSTER ?

Because there is no optional bitmap_ops for md-cluster, it's still
the old bitmap, and llbitmap does not support md-cluster for now.

>
> -- 
> Su
>> +        id = ID_BITMAP;
>> +        break;
>> +    case BITMAP_MAJOR_LOCKLESS:
>> +        id = ID_LLBITMAP;
>> +        break;
>> +    default:
>> +        pr_warn("md: %s: unknown bitmap version %u\n",
>> +            mdname(mddev), version);
>> +        break;
>> +    }
>> +
>> +out_unmap:
>> +    kunmap_local(sb);
>> +out:
>> +    __free_page(sb_page);
>> +    return id;
>> +}
>> +
>>  static int md_bitmap_create(struct mddev *mddev)
>>  {
>> +    enum md_submodule_id orig_id = mddev->bitmap_id;
>> +    enum md_submodule_id sb_id;
>> +    int err;
>> +
>>      if (mddev->bitmap_id == ID_BITMAP_NONE)
>>          return -EINVAL;
>>
>>      if (!mddev_set_bitmap_ops(mddev))
>>          return -ENOENT;
>>
>> -    return mddev->bitmap_ops->create(mddev);
>> +    err = mddev->bitmap_ops->create(mddev);
>> +    if (!err)
>> +        return 0;
>>
>> +
>> +    /*
>> +     * Create failed, if default bitmap version and on-disk version
>> +     * doesn't match, and mdadm is not the latest version to set
>> +     * bitmap_type, set bitmap_ops based on the disk version.
>> +     */
>> +    mddev_clear_bitmap_ops(mddev);
>> +
>> +    sb_id = md_bitmap_get_id_from_sb(mddev);
>> +    if (sb_id == ID_BITMAP_NONE || sb_id == orig_id)
>> +        return err;
>> +
>> +    pr_info("md: %s: bitmap version mismatch, switching from %d to 
>> %d\n",
>> +        mdname(mddev), orig_id, sb_id);
>> +
>> +    mddev->bitmap_id = sb_id;
>> +    if (!mddev_set_bitmap_ops(mddev)) {
>> +        mddev->bitmap_id = orig_id;
>> +        return -ENOENT;
>> +    }
>> +
>> +    err = mddev->bitmap_ops->create(mddev);
>> +    if (err) {
>> +        mddev_clear_bitmap_ops(mddev);
>> +        mddev->bitmap_id = orig_id;
>> +    }
>> +
>> +    return err;
>>  }
>>
>>  static void md_bitmap_destroy(struct mddev *mddev)

-- 
Thansk,
Kuai