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

[Su Yue]

On Mon 23 Feb 2026 at 10:22, "Yu Kuai" <yukuai@xxxxxxxxx> wrote:

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

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