linux-kernel - Re: [PATCH RESEND] md: allow to set the fail

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW6F6kF8CWAFk8Af+-WrELKVz8BYuYPFF1Fy_fAq8us84Q@mail.gmail.com>
Date:   Tue, 12 Oct 2021 10:20:44 -0700
From:   Song Liu <song@...nel.org>
To:     Xiao Ni <xni@...hat.com>
Cc:     Li Feng <fengli@...rtx.com>,
        "open list:SOFTWARE RAID (Multiple Disks) SUPPORT" 
        <linux-raid@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND] md: allow to set the fail_fast on RAID1/RAID10

On Tue, Oct 12, 2021 at 1:49 AM Xiao Ni <xni@...hat.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@...rtx.com> wrote:
> >
> > Song Liu <song@...nel.org> 于2021年10月12日周二 下午4:17写道：
> > >
> > > On Tue, Oct 12, 2021 at 1:07 AM Li Feng <fengli@...rtx.com> wrote:
> > > >
> > > > Xiao Ni <xni@...hat.com> 于2021年10月12日周二 下午2:58写道：
> > > > >
> > > > > On Mon, Oct 11, 2021 at 5:42 PM Li Feng <fengli@...rtx.com> wrote:
> > > > > >
> > > > > > Xiao Ni <xni@...hat.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@...50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync,failfast
> > > [root@...50-1 ~]# echo -failfast >  /sys/block/md127/md/rd1/state
> > > [root@...50-1 ~]# cat /sys/block/md127/md/rd1/state
> > > in_sync
> > > [root@...50-1 ~]# mdadm --stop /dev/md*
> > > mdadm: /dev/md does not appear to be an md device
> > > mdadm: stopped /dev/md127
> > > [root@...50-1 ~]# mdadm -As
> > > mdadm: /dev/md/0_0 has been started with 4 drives.
> > > [root@...50-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
> >
>