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

[Xiao Ni]

在 2025/5/26 下午3:57, Yu Kuai 写道:
> Hi,
>
> 在 2025/05/26 14:52, Xiao Ni 写道:
> > On Sat, May 24, 2025 at 2:18 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> 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.
> >
> > Could you give more explanation about what the time window is? Is it
> > between setting llbitmap by bitmap_type and md_bitmap_create?
>
> The window after this patch is that user can write the new sysfs after
> allocating mddev, and before running the array.


Thanks for the explanation. Is it ok to add it in the commit log message?

> > > 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);
> >
> > I think you're handling a competition problem here. But I don't know
> > how the old/old->group is already created when creating an array.
> > Could you explain this?
>
> It's not possible now, this is because I think we want to be able to
> switch existing array with old bitmap to new bitmap.


Can we add the check of old when we really want it?

Regards

Xiao

> Thanks,
> Kuai
>
> > Regards
> > Xiao