| Message ID | 20240828021828.63447-1-xni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [md-6.12,1/1] md: add new_level sysfs interface | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_11-PR | success | PR summary |
| mdraidci/vmtest-md-6_11-VM_Test-0 | success | Logs for build-kernel |
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
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, >
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,
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, > >
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
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,
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(+)