On Wed, Aug 28, 2024 at 8:33 PM Paul Menzel <pmenzel@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>> ---
> >>> 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,
>
