Re: [PATCH 07/23] md/md-bitmap: delay registration of bitmap_ops until creating bitmap

[Yu Kuai]

Hi,

在 2025/05/27 14:13, Hannes Reinecke 写道:
> On 5/24/25 08:13, Yu Kuai wrote:
> > From: Yu Kuai <yukuai3@xxxxxxxxxx>
> >
> > Currently bitmap_ops is registered while allocating mddev, this is fine
> > when there is only one bitmap_ops, however, after introduing a new
> > bitmap_ops, user space need a time window to choose which bitmap_ops to
> > use while creating new array.
> >
> > Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> > ---
> >   drivers/md/md.c | 86 +++++++++++++++++++++++++++++++------------------
> >   1 file changed, 55 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 4eb0c6effd5b..dc4b85f30e13 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -674,39 +674,50 @@ static void no_op(struct percpu_ref *r) {}
> >   static bool mddev_set_bitmap_ops(struct mddev *mddev)
> >   {
> > +    struct bitmap_operations *old = mddev->bitmap_ops;
> > +    struct md_submodule_head *head;
> > +
> > +    if (mddev->bitmap_id == ID_BITMAP_NONE ||
> > +        (old && old->head.id == mddev->bitmap_id))
> > +        return true;
> > +
> >       xa_lock(&md_submodule);
> > -    mddev->bitmap_ops = xa_load(&md_submodule, mddev->bitmap_id);
> > +    head = xa_load(&md_submodule, mddev->bitmap_id);
> >       xa_unlock(&md_submodule);
> > -    if (!mddev->bitmap_ops) {
> > -        pr_warn_once("md: can't find bitmap id %d\n", mddev->bitmap_id);
> > +    if (WARN_ON_ONCE(!head || head->type != MD_BITMAP)) {
> > +        pr_err("md: can't find bitmap id %d\n", mddev->bitmap_id);
> >           return false;
> >       }
> > +    if (old && old->group)
> > +        sysfs_remove_group(&mddev->kobj, old->group);
> > +
> > +    mddev->bitmap_ops = (void *)head;
> > +    if (mddev->bitmap_ops && mddev->bitmap_ops->group &&
> > +        sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> > +        pr_warn("md: cannot register extra bitmap attributes for %s\n",
> > +            mdname(mddev));
> > +
> >       return true;
> >   }
> >   static void mddev_clear_bitmap_ops(struct mddev *mddev)
> >   {
> > +    if (mddev->bitmap_ops && mddev->bitmap_ops->group)
> > +        sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
> > +
> >       mddev->bitmap_ops = NULL;
> >   }
> >   int mddev_init(struct mddev *mddev)
> >   {
> > -    mddev->bitmap_id = ID_BITMAP;
> > -
> > -    if (!mddev_set_bitmap_ops(mddev))
> > -        return -EINVAL;
> > -
> >       if (percpu_ref_init(&mddev->active_io, active_io_release,
> > -                PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> > -        mddev_clear_bitmap_ops(mddev);
> > +                PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
> >           return -ENOMEM;
> > -    }
> >       if (percpu_ref_init(&mddev->writes_pending, no_op,
> >                   PERCPU_REF_ALLOW_REINIT, GFP_KERNEL)) {
> > -        mddev_clear_bitmap_ops(mddev);
> >           percpu_ref_exit(&mddev->active_io);
> >           return -ENOMEM;
> >       }
> > @@ -734,6 +745,7 @@ int mddev_init(struct mddev *mddev)
> >       mddev->resync_min = 0;
> >       mddev->resync_max = MaxSector;
> >       mddev->level = LEVEL_NONE;
> > +    mddev->bitmap_id = ID_BITMAP;
> >       INIT_WORK(&mddev->sync_work, md_start_sync);
> >       INIT_WORK(&mddev->del_work, mddev_delayed_delete);
> > @@ -744,7 +756,6 @@ EXPORT_SYMBOL_GPL(mddev_init);
> >   void mddev_destroy(struct mddev *mddev)
> >   {
> > -    mddev_clear_bitmap_ops(mddev);
> >       percpu_ref_exit(&mddev->active_io);
> >       percpu_ref_exit(&mddev->writes_pending);
> >   }
> > @@ -6093,11 +6104,6 @@ struct mddev *md_alloc(dev_t dev, char *name)
> >           return ERR_PTR(error);
> >       }
> > -    if (md_bitmap_registered(mddev) && mddev->bitmap_ops->group)
> > -        if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> > -            pr_warn("md: cannot register extra bitmap attributes for 
> > %s\n",
> > -                mdname(mddev));
> > -
> >       kobject_uevent(&mddev->kobj, KOBJ_ADD);
> >       mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, 
> > "array_state");
> >       mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, 
> > "level");
>
> But now you've killed udev event processing.
> Once the 'add' event is sent _all_ sysfs attributes must be present,
> otherwise you'll have a race condition where udev is checking for
> attributes which are present only later.
>
> So when moving things around ensure to move the kobject_uevent() call, too.

I do not expect the bitmap entries are checked by udev, otherwise this
set can introduce regressions since the bitmap entries are no longer
existed after using the new biltmap.

And the above KOBJ_ADD uevent is used for mddev->kobj, right? In this
case, we're creating new entries under mddev->kobj, should this be
KOBJ_CHANGE?

Thanks,
Kuai

> (ideally you would set the sysfs attributes when calling 'add_device()',
> but not sure if that's possible here.)
>
> Cheers,
>
> Hannes