Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230803132426.2688608-1-yukuai1@huaweicloud.com> <20230803132426.2688608-2-yukuai1@huaweicloud.com>
In-Reply-To: <20230803132426.2688608-2-yukuai1@huaweicloud.com>
From:   Song Liu <song@kernel.org>
Date:   Mon, 14 Aug 2023 18:02:46 +0800
Message-ID: <CAPhsuW5m0bzO_FKw4VxE9S4KK6p7EZaffX+ZoYJZ6VGZQrf1hQ@mail.gmail.com>
Subject: Re: [PATCH -next 01/13] md: remove rdev flag 'RemoveSynchronized'
To:     Yu Kuai <yukuai1@huaweicloud.com>, NeilBrown <neil@brown.name>,
        NeilBrown <neilb@suse.de>
Cc:     xni@redhat.com, linux-raid@vger.kernel.org,
        linux-kernel@vger.kernel.org, yukuai3@huawei.com,
        yi.zhang@huawei.com, yangerkun@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

+ Neil

RemoveSynchronized was added by Neil not too long ago. I wonder whether
we still need it in some cases (large raid10 losing a large set of disks).

Thanks,
Song


On Thu, Aug 3, 2023 at 9:27=E2=80=AFPM Yu Kuai <yukuai1@huaweicloud.com> wr=
ote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> When multiple rdevs are removed from array in remove_and_add_spares(),
> which is only possible from daemon thread(user can only remove one rdev
> through ioctl/sysfs at one time), the flag is used to avoid calling
> synchronize_rcu() multiple times for each rdev.
>
> However, remove rdev from daemon thread really is super cold path, while
> removing n rdevs, it doesn't matter sync rcu n times or just one time.
> Hence remove this flag and make code simpler.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md-multipath.c | 15 +++++++--------
>  drivers/md/md.c           | 26 ++------------------------
>  drivers/md/md.h           |  5 -----
>  drivers/md/raid1.c        | 15 +++++++--------
>  drivers/md/raid10.c       | 15 +++++++--------
>  drivers/md/raid5.c        | 15 +++++++--------
>  6 files changed, 30 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/md/md-multipath.c b/drivers/md/md-multipath.c
> index d22276870283..9e4ddd5240cd 100644
> --- a/drivers/md/md-multipath.c
> +++ b/drivers/md/md-multipath.c
> @@ -258,14 +258,13 @@ static int multipath_remove_disk(struct mddev *mdde=
v, struct md_rdev *rdev)
>                         goto abort;
>                 }
>                 p->rdev =3D NULL;
> -               if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> -                       synchronize_rcu();
> -                       if (atomic_read(&rdev->nr_pending)) {
> -                               /* lost the race, try later */
> -                               err =3D -EBUSY;
> -                               p->rdev =3D rdev;
> -                               goto abort;
> -                       }
> +
> +               synchronize_rcu();
> +               if (atomic_read(&rdev->nr_pending)) {
> +                       /* lost the race, try later */
> +                       err =3D -EBUSY;
> +                       p->rdev =3D rdev;
> +                       goto abort;
>                 }
>                 err =3D md_integrity_register(mddev);
>         }
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index a3d98273b295..cd7ac1dee3b8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9155,7 +9155,6 @@ static int remove_and_add_spares(struct mddev *mdde=
v,
>         struct md_rdev *rdev;
>         int spares =3D 0;
>         int removed =3D 0;
> -       bool remove_some =3D false;
>
>         if (this && test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
>                 /* Mustn't remove devices when resync thread is running *=
/
> @@ -9165,28 +9164,9 @@ static int remove_and_add_spares(struct mddev *mdd=
ev,
>                 if ((this =3D=3D NULL || rdev =3D=3D this) &&
>                     rdev->raid_disk >=3D 0 &&
>                     !test_bit(Blocked, &rdev->flags) &&
> -                   test_bit(Faulty, &rdev->flags) &&
> +                   !test_bit(In_sync, &rdev->flags) &&
> +                   !test_bit(Journal, &rdev->flags) &&
>                     atomic_read(&rdev->nr_pending)=3D=3D0) {
> -                       /* Faulty non-Blocked devices with nr_pending =3D=
=3D 0
> -                        * never get nr_pending incremented,
> -                        * never get Faulty cleared, and never get Blocke=
d set.
> -                        * So we can synchronize_rcu now rather than once=
 per device
> -                        */
> -                       remove_some =3D true;
> -                       set_bit(RemoveSynchronized, &rdev->flags);
> -               }
> -       }
> -
> -       if (remove_some)
> -               synchronize_rcu();
> -       rdev_for_each(rdev, mddev) {
> -               if ((this =3D=3D NULL || rdev =3D=3D this) &&
> -                   rdev->raid_disk >=3D 0 &&
> -                   !test_bit(Blocked, &rdev->flags) &&
> -                   ((test_bit(RemoveSynchronized, &rdev->flags) ||
> -                    (!test_bit(In_sync, &rdev->flags) &&
> -                     !test_bit(Journal, &rdev->flags))) &&
> -                   atomic_read(&rdev->nr_pending)=3D=3D0)) {
>                         if (mddev->pers->hot_remove_disk(
>                                     mddev, rdev) =3D=3D 0) {
>                                 sysfs_unlink_rdev(mddev, rdev);
> @@ -9195,8 +9175,6 @@ static int remove_and_add_spares(struct mddev *mdde=
v,
>                                 removed++;
>                         }
>                 }
> -               if (remove_some && test_bit(RemoveSynchronized, &rdev->fl=
ags))
> -                       clear_bit(RemoveSynchronized, &rdev->flags);
>         }
>
>         if (removed && mddev->kobj.sd)
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 8ae957480976..b25b6d061372 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -189,11 +189,6 @@ enum flag_bits {
>                                  * than other devices in the array
>                                  */
>         ClusterRemove,
> -       RemoveSynchronized,     /* synchronize_rcu() was called after
> -                                * this device was known to be faulty,
> -                                * so it is safe to remove without
> -                                * another synchronize_rcu() call.
> -                                */
>         ExternalBbl,            /* External metadata provides bad
>                                  * block management for a disk
>                                  */
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 23d211969565..acb6d6542619 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1859,15 +1859,14 @@ static int raid1_remove_disk(struct mddev *mddev,=
 struct md_rdev *rdev)
>                         goto abort;
>                 }
>                 p->rdev =3D NULL;
> -               if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> -                       synchronize_rcu();
> -                       if (atomic_read(&rdev->nr_pending)) {
> -                               /* lost the race, try later */
> -                               err =3D -EBUSY;
> -                               p->rdev =3D rdev;
> -                               goto abort;
> -                       }
> +               synchronize_rcu();
> +               if (atomic_read(&rdev->nr_pending)) {
> +                       /* lost the race, try later */
> +                       err =3D -EBUSY;
> +                       p->rdev =3D rdev;
> +                       goto abort;
>                 }
> +
>                 if (conf->mirrors[conf->raid_disks + number].rdev) {
>                         /* We just removed a device that is being replace=
d.
>                          * Move down the replacement.  We drain all IO be=
fore
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 7704a4c7f469..64dd5cb6133e 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -2247,15 +2247,14 @@ static int raid10_remove_disk(struct mddev *mddev=
, struct md_rdev *rdev)
>                 goto abort;
>         }
>         *rdevp =3D NULL;
> -       if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> -               synchronize_rcu();
> -               if (atomic_read(&rdev->nr_pending)) {
> -                       /* lost the race, try later */
> -                       err =3D -EBUSY;
> -                       *rdevp =3D rdev;
> -                       goto abort;
> -               }
> +       synchronize_rcu();
> +       if (atomic_read(&rdev->nr_pending)) {
> +               /* lost the race, try later */
> +               err =3D -EBUSY;
> +               *rdevp =3D rdev;
> +               goto abort;
>         }
> +
>         if (p->replacement) {
>                 /* We must have just cleared 'rdev' */
>                 p->rdev =3D p->replacement;
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 4cdb35e54251..37d9865b180a 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -8267,15 +8267,14 @@ static int raid5_remove_disk(struct mddev *mddev,=
 struct md_rdev *rdev)
>                 goto abort;
>         }
>         *rdevp =3D NULL;
> -       if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> -               lockdep_assert_held(&mddev->reconfig_mutex);
> -               synchronize_rcu();
> -               if (atomic_read(&rdev->nr_pending)) {
> -                       /* lost the race, try later */
> -                       err =3D -EBUSY;
> -                       rcu_assign_pointer(*rdevp, rdev);
> -               }
> +       lockdep_assert_held(&mddev->reconfig_mutex);
> +       synchronize_rcu();
> +       if (atomic_read(&rdev->nr_pending)) {
> +               /* lost the race, try later */
> +               err =3D -EBUSY;
> +               rcu_assign_pointer(*rdevp, rdev);
>         }
> +
>         if (!err) {
>                 err =3D log_modify(conf, rdev, false);
>                 if (err)
> --
> 2.39.2
>