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

[Li Feng <fengli@smartx.com>]

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