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

[Xiao Ni <xni@redhat.com>]

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
>