[PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Li Feng]

When the running RAID1/RAID10 need to be set with the fail_fast flag,
we have to remove each device from RAID and re-add it again with the
--fail_fast flag.

Export the fail_fast flag to the userspace to support the read and
write.

Signed-off-by: Li Feng <fengli@smartx.com>
---
 drivers/md/md.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index ae8fe54ea358..ce63972a4555 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3583,6 +3583,35 @@ ppl_size_store(struct md_rdev *rdev, const char *buf, size_t len)
 static struct rdev_sysfs_entry rdev_ppl_size =
 __ATTR(ppl_size, S_IRUGO|S_IWUSR, ppl_size_show, ppl_size_store);
 
+static ssize_t
+fail_fast_show(struct md_rdev *rdev, char *page)
+{
+	return sprintf(page, "%d\n", test_bit(FailFast, &rdev->flags));
+}
+
+static ssize_t
+fail_fast_store(struct md_rdev *rdev, const char *buf, size_t len)
+{
+	int ret;
+	bool bit;
+
+	ret = kstrtobool(buf, &bit);
+	if (ret)
+		return ret;
+
+	if (test_bit(FailFast, &rdev->flags) && !bit) {
+		clear_bit(FailFast, &rdev->flags);
+		md_update_sb(rdev->mddev, 1);
+	} else if (!test_bit(FailFast, &rdev->flags) && bit) {
+		set_bit(FailFast, &rdev->flags);
+		md_update_sb(rdev->mddev, 1);
+	}
+	return len;
+}
+
+static struct rdev_sysfs_entry rdev_fail_fast =
+__ATTR(fail_fast, 0644, fail_fast_show, fail_fast_store);
+
 static struct attribute *rdev_default_attrs[] = {
 	&rdev_state.attr,
 	&rdev_errors.attr,
@@ -3595,6 +3624,7 @@ static struct attribute *rdev_default_attrs[] = {
 	&rdev_unack_bad_blocks.attr,
 	&rdev_ppl_sector.attr,
 	&rdev_ppl_size.attr,
+	&rdev_fail_fast.attr,
 	NULL,
 };
 static ssize_t
-- 
2.31.1

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Song Liu]

On Thu, Oct 7, 2021 at 8:22 PM Li Feng <fengli@smartx.com> wrote:
>
> When the running RAID1/RAID10 need to be set with the fail_fast flag,
> we have to remove each device from RAID and re-add it again with the
> --fail_fast flag.
>
> Export the fail_fast flag to the userspace to support the read and
> write.
>
> Signed-off-by: Li Feng <fengli@smartx.com>

Thanks for the patch! I applied it to md-next, with some changes in the
commit log.

Thanks,
Song

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Xiao Ni]

Hi all

Now the per device sysfs interface file state can change failfast. Do
we need a new file for failfast?

I did a test. The steps are:

mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
cd /sys/block/md0/md/dev-sdb
echo failfast > state
cat state
in_sync,failfast

Best Regards
Xiao

On Sat, Oct 9, 2021 at 7:36 AM Song Liu <song@kernel.org> wrote:
>
> On Thu, Oct 7, 2021 at 8:22 PM Li Feng <fengli@smartx.com> wrote:
> >
> > When the running RAID1/RAID10 need to be set with the fail_fast flag,
> > we have to remove each device from RAID and re-add it again with the
> > --fail_fast flag.
> >
> > Export the fail_fast flag to the userspace to support the read and
> > write.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
>
> Thanks for the patch! I applied it to md-next, with some changes in the
> commit log.
>
> Thanks,
> Song
>

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Li Feng]

Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
>
> Hi all
>
> Now the per device sysfs interface file state can change failfast. Do
> we need a new file for failfast?
>
> I did a test. The steps are:
>
> mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> cd /sys/block/md0/md/dev-sdb
> echo failfast > state
> cat state
> in_sync,failfast

This works,  will it be persisted to disk?

>
> Best Regards
> Xiao
>
> On Sat, Oct 9, 2021 at 7:36 AM Song Liu <song@kernel.org> wrote:
> >
> > On Thu, Oct 7, 2021 at 8:22 PM Li Feng <fengli@smartx.com> wrote:
> > >
> > > When the running RAID1/RAID10 need to be set with the fail_fast flag,
> > > we have to remove each device from RAID and re-add it again with the
> > > --fail_fast flag.
> > >
> > > Export the fail_fast flag to the userspace to support the read and
> > > write.
> > >
> > > Signed-off-by: Li Feng <fengli@smartx.com>
> >
> > Thanks for the patch! I applied it to md-next, with some changes in the
> > commit log.
> >
> > Thanks,
> > Song
> >
>

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Xiao Ni]

On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
>
> Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> >
> > Hi all
> >
> > Now the per device sysfs interface file state can change failfast. Do
> > we need a new file for failfast?
> >
> > I did a test. The steps are:
> >
> > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > cd /sys/block/md0/md/dev-sdb
> > echo failfast > state
> > cat state
> > in_sync,failfast
>
> This works,  will it be persisted to disk?
>

mdadm --detail /dev/md0 can show the failfast information. So it
should be written in superblock.
But I don't find how md does this. I'm looking at this.

Regards
Xiao

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Li Feng]

Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午2:58写道：
>
> On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
> >
> > Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> > >
> > > Hi all
> > >
> > > Now the per device sysfs interface file state can change failfast. Do
> > > we need a new file for failfast?
> > >
> > > I did a test. The steps are:
> > >
> > > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > > cd /sys/block/md0/md/dev-sdb
> > > echo failfast > state
> > > cat state
> > > in_sync,failfast
> >
> > This works,  will it be persisted to disk?
> >
>
> mdadm --detail /dev/md0 can show the failfast information. So it
> should be written in superblock.
> But I don't find how md does this. I'm looking at this.
>
Yes, I have tested that it has been persisted, but don't understand who does it.

> Regards
> Xiao
>

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Song Liu]

On Tue, Oct 12, 2021 at 1:07 AM Li Feng <fengli@smartx.com> wrote:
>
> Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午2:58写道：
> >
> > On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
> > >
> > > Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> > > >
> > > > Hi all
> > > >
> > > > Now the per device sysfs interface file state can change failfast. Do
> > > > we need a new file for failfast?
> > > >
> > > > I did a test. The steps are:
> > > >
> > > > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > > > cd /sys/block/md0/md/dev-sdb
> > > > echo failfast > state
> > > > cat state
> > > > in_sync,failfast
> > >
> > > This works,  will it be persisted to disk?
> > >
> >
> > mdadm --detail /dev/md0 can show the failfast information. So it
> > should be written in superblock.
> > But I don't find how md does this. I'm looking at this.
> >
> Yes, I have tested that it has been persisted, but don't understand who does it.

I think this is not guaranteed to be persistent:

[root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
in_sync,failfast
[root@eth50-1 ~]# echo -failfast >  /sys/block/md127/md/rd1/state
[root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
in_sync
[root@eth50-1 ~]# mdadm --stop /dev/md*
mdadm: /dev/md does not appear to be an md device
mdadm: stopped /dev/md127
[root@eth50-1 ~]# mdadm -As
mdadm: /dev/md/0_0 has been started with 4 drives.
[root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
in_sync,failfast

How about we fix state_store to make sure it is always persistent?

Thanks,
Song

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Li Feng]

Song Liu <song@kernel.org> 于2021年10月12日周二 下午4:17写道：
>
> On Tue, Oct 12, 2021 at 1:07 AM Li Feng <fengli@smartx.com> wrote:
> >
> > Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午2:58写道：
> > >
> > > On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
> > > >
> > > > Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> > > > >
> > > > > Hi all
> > > > >
> > > > > Now the per device sysfs interface file state can change failfast. Do
> > > > > we need a new file for failfast?
> > > > >
> > > > > I did a test. The steps are:
> > > > >
> > > > > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > > > > cd /sys/block/md0/md/dev-sdb
> > > > > echo failfast > state
> > > > > cat state
> > > > > in_sync,failfast
> > > >
> > > > This works,  will it be persisted to disk?
> > > >
> > >
> > > mdadm --detail /dev/md0 can show the failfast information. So it
> > > should be written in superblock.
> > > But I don't find how md does this. I'm looking at this.
> > >
> > Yes, I have tested that it has been persisted, but don't understand who does it.
>
> I think this is not guaranteed to be persistent:
>
> [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> in_sync,failfast
> [root@eth50-1 ~]# echo -failfast >  /sys/block/md127/md/rd1/state
> [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> in_sync
> [root@eth50-1 ~]# mdadm --stop /dev/md*
> mdadm: /dev/md does not appear to be an md device
> mdadm: stopped /dev/md127
> [root@eth50-1 ~]# mdadm -As
> mdadm: /dev/md/0_0 has been started with 4 drives.
> [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> in_sync,failfast
>
> How about we fix state_store to make sure it is always persistent?
>
I agree with you.

> Thanks,
> Song

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Xiao Ni]

Hi all

How about this patch? Now writemostly flag doesn't be stored in
superblock too. So this patch fix this problem too.
If this patch is ok, I'll send the patch.

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 6c0c3d0d905a..9e8a8c5c7758 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -2977,6 +2977,7 @@ state_store(struct md_rdev *rdev, const char
*buf, size_t len)
      *  {,-}failfast - set/clear FailFast
      */
     int err = -EINVAL;
+    int need_update_sb = 0;
     if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
         md_error(rdev->mddev, rdev);
         if (test_bit(Faulty, &rdev->flags))
@@ -2998,20 +2999,19 @@ state_store(struct md_rdev *rdev, const char
*buf, size_t len)

             if (err == 0) {
                 md_kick_rdev_from_array(rdev);
-                if (mddev->pers) {
-                    set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
-                    md_wakeup_thread(mddev->thread);
-                }
+                need_update_sb = 1;
                 md_new_event(mddev);
             }
         }
     } else if (cmd_match(buf, "writemostly")) {
         set_bit(WriteMostly, &rdev->flags);
         mddev_create_serial_pool(rdev->mddev, rdev, false);
+        need_update_sb = 1;
         err = 0;
     } else if (cmd_match(buf, "-writemostly")) {
         mddev_destroy_serial_pool(rdev->mddev, rdev, false);
         clear_bit(WriteMostly, &rdev->flags);
+        need_update_sb = 1;
         err = 0;
     } else if (cmd_match(buf, "blocked")) {
         set_bit(Blocked, &rdev->flags);
@@ -3037,9 +3037,11 @@ state_store(struct md_rdev *rdev, const char
*buf, size_t len)
         err = 0;
     } else if (cmd_match(buf, "failfast")) {
         set_bit(FailFast, &rdev->flags);
+        need_update_sb = 1;
         err = 0;
     } else if (cmd_match(buf, "-failfast")) {
         clear_bit(FailFast, &rdev->flags);
+        need_update_sb = 1;
         err = 0;
     } else if (cmd_match(buf, "-insync") && rdev->raid_disk >= 0 &&
            !test_bit(Journal, &rdev->flags)) {
@@ -3120,6 +3122,11 @@ state_store(struct md_rdev *rdev, const char
*buf, size_t len)
     }
     if (!err)
         sysfs_notify_dirent_safe(rdev->sysfs_state);
+    if (need_update_sb)
+        if (mddev->pers) {
+            set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
+            md_wakeup_thread(mddev->thread);
+        }
     return err ? err : len;
 }
 static struct rdev_sysfs_entry rdev_state =

On Tue, Oct 12, 2021 at 4:44 PM Li Feng <fengli@smartx.com> wrote:
>
> Song Liu <song@kernel.org> 于2021年10月12日周二 下午4:17写道：
> >
> > On Tue, Oct 12, 2021 at 1:07 AM Li Feng <fengli@smartx.com> wrote:
> > >
> > > Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午2:58写道：
> > > >
> > > > On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
> > > > >
> > > > > Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> > > > > >
> > > > > > Hi all
> > > > > >
> > > > > > Now the per device sysfs interface file state can change failfast. Do
> > > > > > we need a new file for failfast?
> > > > > >
> > > > > > I did a test. The steps are:
> > > > > >
> > > > > > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > > > > > cd /sys/block/md0/md/dev-sdb
> > > > > > echo failfast > state
> > > > > > cat state
> > > > > > in_sync,failfast
> > > > >
> > > > > This works,  will it be persisted to disk?
> > > > >
> > > >
> > > > mdadm --detail /dev/md0 can show the failfast information. So it
> > > > should be written in superblock.
> > > > But I don't find how md does this. I'm looking at this.
> > > >
> > > Yes, I have tested that it has been persisted, but don't understand who does it.
> >
> > I think this is not guaranteed to be persistent:
> >
> > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > in_sync,failfast
> > [root@eth50-1 ~]# echo -failfast >  /sys/block/md127/md/rd1/state
> > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > in_sync
> > [root@eth50-1 ~]# mdadm --stop /dev/md*
> > mdadm: /dev/md does not appear to be an md device
> > mdadm: stopped /dev/md127
> > [root@eth50-1 ~]# mdadm -As
> > mdadm: /dev/md/0_0 has been started with 4 drives.
> > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > in_sync,failfast
> >
> > How about we fix state_store to make sure it is always persistent?
> >
> I agree with you.
>
> > Thanks,
> > Song
>

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Li Feng]

Thanks,
Feng Li

Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午4:49写道：
>
> Hi all
>
> How about this patch? Now writemostly flag doesn't be stored in
> superblock too. So this patch fix this problem too.
> If this patch is ok, I'll send the patch.
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 6c0c3d0d905a..9e8a8c5c7758 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2977,6 +2977,7 @@ state_store(struct md_rdev *rdev, const char
> *buf, size_t len)
>       *  {,-}failfast - set/clear FailFast
>       */
>      int err = -EINVAL;
> +    int need_update_sb = 0;
>      if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
>          md_error(rdev->mddev, rdev);
>          if (test_bit(Faulty, &rdev->flags))
> @@ -2998,20 +2999,19 @@ state_store(struct md_rdev *rdev, const char
> *buf, size_t len)
>
>              if (err == 0) {
>                  md_kick_rdev_from_array(rdev);
> -                if (mddev->pers) {
> -                    set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> -                    md_wakeup_thread(mddev->thread);
> -                }
> +                need_update_sb = 1;
>                  md_new_event(mddev);
>              }
>          }
>      } else if (cmd_match(buf, "writemostly")) {
>          set_bit(WriteMostly, &rdev->flags);
>          mddev_create_serial_pool(rdev->mddev, rdev, false);
> +        need_update_sb = 1;
>          err = 0;
>      } else if (cmd_match(buf, "-writemostly")) {
>          mddev_destroy_serial_pool(rdev->mddev, rdev, false);
>          clear_bit(WriteMostly, &rdev->flags);
> +        need_update_sb = 1;
>          err = 0;
>      } else if (cmd_match(buf, "blocked")) {
>          set_bit(Blocked, &rdev->flags);
> @@ -3037,9 +3037,11 @@ state_store(struct md_rdev *rdev, const char
> *buf, size_t len)
>          err = 0;
>      } else if (cmd_match(buf, "failfast")) {
>          set_bit(FailFast, &rdev->flags);
> +        need_update_sb = 1;
>          err = 0;
>      } else if (cmd_match(buf, "-failfast")) {
>          clear_bit(FailFast, &rdev->flags);
> +        need_update_sb = 1;
>          err = 0;
>      } else if (cmd_match(buf, "-insync") && rdev->raid_disk >= 0 &&
>             !test_bit(Journal, &rdev->flags)) {
> @@ -3120,6 +3122,11 @@ state_store(struct md_rdev *rdev, const char
> *buf, size_t len)
>      }
>      if (!err)
>          sysfs_notify_dirent_safe(rdev->sysfs_state);
> +    if (need_update_sb)
> +        if (mddev->pers) {
> +            set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> +            md_wakeup_thread(mddev->thread);
> +        }
When will mddev->pers is NULL?
If it is NULL, this change will not on disk.

>      return err ? err : len;
>  }
>  static struct rdev_sysfs_entry rdev_state =
>
> On Tue, Oct 12, 2021 at 4:44 PM Li Feng <fengli@smartx.com> wrote:
> >
> > Song Liu <song@kernel.org> 于2021年10月12日周二 下午4:17写道：
> > >
> > > On Tue, Oct 12, 2021 at 1:07 AM Li Feng <fengli@smartx.com> wrote:
> > > >
> > > > Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午2:58写道：
> > > > >
> > > > > On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
> > > > > >
> > > > > > Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> > > > > > >
> > > > > > > Hi all
> > > > > > >
> > > > > > > Now the per device sysfs interface file state can change failfast. Do
> > > > > > > we need a new file for failfast?
> > > > > > >
> > > > > > > I did a test. The steps are:
> > > > > > >
> > > > > > > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > > > > > > cd /sys/block/md0/md/dev-sdb
> > > > > > > echo failfast > state
> > > > > > > cat state
> > > > > > > in_sync,failfast
> > > > > >
> > > > > > This works,  will it be persisted to disk?
> > > > > >
> > > > >
> > > > > mdadm --detail /dev/md0 can show the failfast information. So it
> > > > > should be written in superblock.
> > > > > But I don't find how md does this. I'm looking at this.
> > > > >
> > > > Yes, I have tested that it has been persisted, but don't understand who does it.
> > >
> > > I think this is not guaranteed to be persistent:
> > >
> > > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync,failfast
> > > [root@eth50-1 ~]# echo -failfast >  /sys/block/md127/md/rd1/state
> > > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync
> > > [root@eth50-1 ~]# mdadm --stop /dev/md*
> > > mdadm: /dev/md does not appear to be an md device
> > > mdadm: stopped /dev/md127
> > > [root@eth50-1 ~]# mdadm -As
> > > mdadm: /dev/md/0_0 has been started with 4 drives.
> > > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync,failfast
> > >
> > > How about we fix state_store to make sure it is always persistent?
> > >
> > I agree with you.
> >
> > > Thanks,
> > > Song
> >
>

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Song Liu]

On Tue, Oct 12, 2021 at 1:49 AM Xiao Ni <xni@redhat.com> wrote:
>
> Hi all
>
> How about this patch? Now writemostly flag doesn't be stored in
> superblock too. So this patch fix this problem too.
> If this patch is ok, I'll send the patch.
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 6c0c3d0d905a..9e8a8c5c7758 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -2977,6 +2977,7 @@ state_store(struct md_rdev *rdev, const char
> *buf, size_t len)
>       *  {,-}failfast - set/clear FailFast
>       */
>      int err = -EINVAL;
> +    int need_update_sb = 0;

Please use bool for need_update_sb.

>      if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
>          md_error(rdev->mddev, rdev);
>          if (test_bit(Faulty, &rdev->flags))

[...]

> @@ -3120,6 +3122,11 @@ state_store(struct md_rdev *rdev, const char
> *buf, size_t len)
>      }
>      if (!err)
>          sysfs_notify_dirent_safe(rdev->sysfs_state);
> +    if (need_update_sb)
> +        if (mddev->pers) {
We can merge the two conditions in in one line

if (need_update_sb && mddev->pers) {
...
}

> +            set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> +            md_wakeup_thread(mddev->thread);
> +        }
>      return err ? err : len;
>  }
>  static struct rdev_sysfs_entry rdev_state =
>
> On Tue, Oct 12, 2021 at 4:44 PM Li Feng <fengli@smartx.com> wrote:
> >
> > Song Liu <song@kernel.org> 于2021年10月12日周二 下午4:17写道：
> > >
> > > On Tue, Oct 12, 2021 at 1:07 AM Li Feng <fengli@smartx.com> wrote:
> > > >
> > > > Xiao Ni <xni@redhat.com> 于2021年10月12日周二 下午2:58写道：
> > > > >
> > > > > On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@smartx.com> wrote:
> > > > > >
> > > > > > Xiao Ni <xni@redhat.com> 于2021年10月11日周一 下午3:49写道：
> > > > > > >
> > > > > > > Hi all
> > > > > > >
> > > > > > > Now the per device sysfs interface file state can change failfast. Do
> > > > > > > we need a new file for failfast?
> > > > > > >
> > > > > > > I did a test. The steps are:
> > > > > > >
> > > > > > > mdadm -CR /dev/md0 -l1 -n2 /dev/sdb /dev/sdc --assume-clean
> > > > > > > cd /sys/block/md0/md/dev-sdb
> > > > > > > echo failfast > state
> > > > > > > cat state
> > > > > > > in_sync,failfast
> > > > > >
> > > > > > This works,  will it be persisted to disk?
> > > > > >
> > > > >
> > > > > mdadm --detail /dev/md0 can show the failfast information. So it
> > > > > should be written in superblock.
> > > > > But I don't find how md does this. I'm looking at this.
> > > > >
> > > > Yes, I have tested that it has been persisted, but don't understand who does it.
> > >
> > > I think this is not guaranteed to be persistent:
> > >
> > > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync,failfast
> > > [root@eth50-1 ~]# echo -failfast >  /sys/block/md127/md/rd1/state
> > > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync
> > > [root@eth50-1 ~]# mdadm --stop /dev/md*
> > > mdadm: /dev/md does not appear to be an md device
> > > mdadm: stopped /dev/md127
> > > [root@eth50-1 ~]# mdadm -As
> > > mdadm: /dev/md/0_0 has been started with 4 drives.
> > > [root@eth50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync,failfast
> > >
> > > How about we fix state_store to make sure it is always persistent?
> > >
> > I agree with you.
> >
> > > Thanks,
> > > Song
> >
>

Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

[Xiao Ni]

On Tue, Oct 12, 2021 at 5:14 PM Li Feng <fengli@smartx.com> wrote:
> >          sysfs_notify_dirent_safe(rdev->sysfs_state);
> > +    if (need_update_sb)
> > +        if (mddev->pers) {
> > +            set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> > +            md_wakeup_thread(mddev->thread);
> > +        }
> When will mddev->pers is NULL?

The process of creating a raid device is:
1. md_ioctl->md_add_new_disk
2. md_ioctl->do_md_run->md_run->(pers->run)->(mddev->pers = pers)
In md_add_new_disk it creates the per device sysfs files. It can
read/write these
files before setting mddev->pers.

> If it is NULL, this change will not on disk.

I did a test. You are right. Someone can change the per device state
before ADD_NEW_DISK
and RUN_ARRAY ioctl. Please note, the mdadm --create command doesn't
return until RUN_ARRAY
ioctl finishes. Even though the are small, we can try to set MD_SB_CHANGE_DEVS.

Best regards
Xiao