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

[Xiao Ni <xni@redhat.com>]

在 2026/2/24 09:52, Su Yue 写道:
> On Mon 23 Feb 2026 at 10:22, "Yu Kuai" <yukuai@fnnas.com> wrote:
>
>> Hi,
>>
>> 在 2026/2/17 16:54, Su Yue 写道:
>>> On Sat 14 Feb 2026 at 14:10, Yu Kuai <yukuai@fnnas.com> 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.
>>
> Would you please elaborate which corner cases that llbitmap array 
> fails to assemble
> in? Do they happen in mdadm <= 4.5?
>
>> On the other hand, this should also be helpful if we decide to make 
>> llbitmap
>> the default option in the future.
>>
> But it's so far, right? llbitmap support is still on the way(mdadm 4.6 
> is not released).
>
> I am not opposed to the patch. It just looks strange to me that 
> changing kernel code to
> let old userspace work with *new* feature.
> Maybe the mdadm maintainers have words in another angles?


Yes. Is it better to upgrade mdadm to the version which supports llbitmap?


Regards

Xiao

>
> -- 
> Su
>>
>>>
>>>> Signed-off-by: Yu Kuai <yukuai@fnnas.com>
>>>> ---
>>>>  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)
>