Re: [PATCH md-6.12 1/1] md: add new_level sysfs interface

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Tue, 3 Sep 2024 08:18:19 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Mon, Sep 2, 2024 at 6:14 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Wed, 28 Aug 2024 10:18:28 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > This interface is used to updating new level during reshape progress.
> > > Now it doesn't update new level during reshape. So it can't know the
> > > new level when systemd service mdadm-grow-continue runs. And it can't
> > > finally change to new level.
> > >
> > > Signed-off-by: Xiao Ni <xni@redhat.com>
> > > ---
> > >  drivers/md/md.c | 29 +++++++++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > > index d3a837506a36..c639eca03df9 100644
> > > --- a/drivers/md/md.c
> > > +++ b/drivers/md/md.c
> > > @@ -4141,6 +4141,34 @@ level_store(struct mddev *mddev, const char *buf,
> > > size_t len) static struct md_sysfs_entry md_level =
> > >  __ATTR(level, S_IRUGO|S_IWUSR, level_show, level_store);
> > >
> > > +static ssize_t
> > > +new_level_show(struct mddev *mddev, char *page)
> > > +{
> > > +     return sprintf(page, "%d\n", mddev->new_level);
> > > +}
> > > +
> > > +static ssize_t
> > > +new_level_store(struct mddev *mddev, const char *buf, size_t len)
> > > +{
> > > +     unsigned int n;
> > > +     int err;
> > > +
> > > +     err = kstrtouint(buf, 10, &n);
> > > +     if (err < 0)
> > > +             return err;
> > > +     err = mddev_lock(mddev);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     mddev->new_level = n;
> > > +     md_update_sb(mddev, 1);  
> >
> > I don't see any code behind mddev->new_level handling so I suspect that
> > md_update_sb() does nothing in this case. Is there something I'm missing?  
> 
> You mean the calling path md_update_sb->sync_sbs->super_1_sync?
> 

No, I missed that mddev->new_level is a exiting property and it is already
implemented in kernel. Now, I understand that it is super0 metadata property
and you just presented it. This is fine.

What I can see is that calling md_update_super() in case of
external might be unnecessary but it is fine anyway I think.

maybe: if (!mddev->external)
               md_update_sb(mddev, 1);

but I don't see it used in the code anywhere. metadata update path is
still black box to me. Maybe Kuai can comment?

Thanks,
Mariusz