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

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

[Xiao Ni]

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);
+
+	mddev_unlock(mddev);
+	return err ?: len;
+}
+static struct md_sysfs_entry md_new_level =
+__ATTR(new_level, 0664, new_level_show, new_level_store);
+
 static ssize_t
 layout_show(struct mddev *mddev, char *page)
 {
@@ -5666,6 +5694,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR, serialize_policy_show,
 
 static struct attribute *md_default_attrs[] = {
 	&md_level.attr,
+	&md_new_level.attr,
 	&md_layout.attr,
 	&md_raid_disks.attr,
 	&md_uuid.attr,
-- 
2.32.0 (Apple Git-132)

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

[Paul Menzel]

Dear Xiao,


Thank you for your patch.

Am 28.08.24 um 04:18 schrieb Xiao Ni:
> This interface is used to updating new level during reshape progress.

to update

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

How can this be tested?

> 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);
> +
> +	mddev_unlock(mddev);
> +	return err ?: len;

Can `err` be set? Why return `len` if it’s not modified?

> +}
> +static struct md_sysfs_entry md_new_level =
> +__ATTR(new_level, 0664, new_level_show, new_level_store);
> +
>   static ssize_t
>   layout_show(struct mddev *mddev, char *page)
>   {
> @@ -5666,6 +5694,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR, serialize_policy_show,
>   
>   static struct attribute *md_default_attrs[] = {
>   	&md_level.attr,
> +	&md_new_level.attr,
>   	&md_layout.attr,
>   	&md_raid_disks.attr,
>   	&md_uuid.attr,


Kind regards,

Paul

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

[Xiao Ni]

On Wed, Aug 28, 2024 at 8:33 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Xiao,
>
>
> Thank you for your reply. Was it intentional, that you only replied to
> me off-list?

Hi Paul

I made a mistake. Thanks for reminding me.

>
> Am 28.08.24 um 10:44 schrieb Xiao Ni:
> > On Wed, Aug 28, 2024 at 1:19 PM Paul Menzel wrote:
>
> >> Am 28.08.24 um 04:18 schrieb Xiao Ni:
>
> […]
>
> >> How can this be tested?
> >
> > mdadm -CR /dev/md0 -l6 -n4 /dev/loop[0-3]
> > mdadm --wait /dev/md0
> > mdadm /dev/md0 --grow -l5 --backup=backup
> > cat /proc/mdstat
> > Personalities : [raid6] [raid5] [raid4] [raid0] [raid1] [raid10]
> > md0 : active raid6 loop3[3] loop2[2] loop1[1] loop0[0]
> >        98304 blocks super 1.2 level 6, 512k chunk, algorithm 18 [4/4] [UUUU]
> >
> > unused devices: <none>
> >
> > There is a tests directory in mdadm. The case 07changelevels have this
> > test. Now it can't run successfully. This patch is used to fix this.
>
> It’d be great if you mentioned this in the commit message. That this
> test fails, and now it works. Please also mention, if this was a
> regression. (I guess it is, as the test should have been passed, when it
> was added?)

Ok, I'll add them to the commit message. The test cases was created in
2009. So yes, it must be a regression, but it's hard to see what
changes cause the regression.

>
> >>> 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);
> >>> +
> >>> +     mddev_unlock(mddev);
> >>> +     return err ?: len;
> >>
> >> Can `err` be set? Why return `len` if it’s not modified?
> >
> > err is the return value from kstrtouint and mddev_lock. And len is the
> > input value of the buf length. If it can be set successfully, it
> > returns len. It's same with other sysfs functions. For example,
> > raid_disks_store, layout_store and so on.
>
> At least `layout_store` modifies `err` later on. In your new function,
> if there is an error the function returns, so there is no need to check it.

Ah I know what you mean. I'll fix this.
>
> Where is `len` used in your new function? It’s only in the function
> signature. A colleague also didn’t spot it.

Hmm, functions raid_disks_store, layout_store, chunk_size_sotre don't
use len too. So what's your suggestion here?

Best Regards
Xiao
>
>
> Kind regards,
>
> Paul
>
>
> >>> +}
> >>> +static struct md_sysfs_entry md_new_level =
> >>> +__ATTR(new_level, 0664, new_level_show, new_level_store);
> >>> +
> >>>    static ssize_t
> >>>    layout_show(struct mddev *mddev, char *page)
> >>>    {
> >>> @@ -5666,6 +5694,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR, serialize_policy_show,
> >>>
> >>>    static struct attribute *md_default_attrs[] = {
> >>>        &md_level.attr,
> >>> +     &md_new_level.attr,
> >>>        &md_layout.attr,
> >>>        &md_raid_disks.attr,
> >>>        &md_uuid.attr,
>

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

[Mariusz Tkaczyk]

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?

Thanks,
Mariusz

> +
> +	mddev_unlock(mddev);
> +	return err ?: len;
> +}
> +static struct md_sysfs_entry md_new_level =
> +__ATTR(new_level, 0664, new_level_show, new_level_store);
> +
>  static ssize_t
>  layout_show(struct mddev *mddev, char *page)
>  {
> @@ -5666,6 +5694,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR,
> serialize_policy_show, 
>  static struct attribute *md_default_attrs[] = {
>  	&md_level.attr,
> +	&md_new_level.attr,
>  	&md_layout.attr,
>  	&md_raid_disks.attr,
>  	&md_uuid.attr,

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

[Xiao Ni]

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?

Best Regards
Xiao
>
> Thanks,
> Mariusz
>
> > +
> > +     mddev_unlock(mddev);
> > +     return err ?: len;
> > +}
> > +static struct md_sysfs_entry md_new_level =
> > +__ATTR(new_level, 0664, new_level_show, new_level_store);
> > +
> >  static ssize_t
> >  layout_show(struct mddev *mddev, char *page)
> >  {
> > @@ -5666,6 +5694,7 @@ __ATTR(serialize_policy, S_IRUGO | S_IWUSR,
> > serialize_policy_show,
> >  static struct attribute *md_default_attrs[] = {
> >       &md_level.attr,
> > +     &md_new_level.attr,
> >       &md_layout.attr,
> >       &md_raid_disks.attr,
> >       &md_uuid.attr,
>
>

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

[Mariusz Tkaczyk]

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