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

[Su Yue <l@damenly.org>]

On Tue 10 Mar 2026 at 09:15, Xiao Ni <xni@redhat.com> wrote:

> 在 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?
>

Yes. This's what I mean.

--
Su
>
> 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)
>>