From: Yu Kuai <[email protected]>
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 <[email protected]>
---
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 *mddev, struct md_rdev *rdev)
goto abort;
}
p->rdev = NULL;
- if (!test_bit(RemoveSynchronized, &rdev->flags)) {
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- p->rdev = rdev;
- goto abort;
- }
+
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ p->rdev = rdev;
+ goto abort;
}
err = 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 *mddev,
struct md_rdev *rdev;
int spares = 0;
int removed = 0;
- bool remove_some = 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 *mddev,
if ((this == NULL || rdev == this) &&
rdev->raid_disk >= 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)==0) {
- /* Faulty non-Blocked devices with nr_pending == 0
- * never get nr_pending incremented,
- * never get Faulty cleared, and never get Blocked set.
- * So we can synchronize_rcu now rather than once per device
- */
- remove_some = true;
- set_bit(RemoveSynchronized, &rdev->flags);
- }
- }
-
- if (remove_some)
- synchronize_rcu();
- rdev_for_each(rdev, mddev) {
- if ((this == NULL || rdev == this) &&
- rdev->raid_disk >= 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)==0)) {
if (mddev->pers->hot_remove_disk(
mddev, rdev) == 0) {
sysfs_unlink_rdev(mddev, rdev);
@@ -9195,8 +9175,6 @@ static int remove_and_add_spares(struct mddev *mddev,
removed++;
}
}
- if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
- 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 = NULL;
- if (!test_bit(RemoveSynchronized, &rdev->flags)) {
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- p->rdev = rdev;
- goto abort;
- }
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ p->rdev = rdev;
+ goto abort;
}
+
if (conf->mirrors[conf->raid_disks + number].rdev) {
/* We just removed a device that is being replaced.
* Move down the replacement. We drain all IO before
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 = NULL;
- if (!test_bit(RemoveSynchronized, &rdev->flags)) {
- synchronize_rcu();
- if (atomic_read(&rdev->nr_pending)) {
- /* lost the race, try later */
- err = -EBUSY;
- *rdevp = rdev;
- goto abort;
- }
+ synchronize_rcu();
+ if (atomic_read(&rdev->nr_pending)) {
+ /* lost the race, try later */
+ err = -EBUSY;
+ *rdevp = rdev;
+ goto abort;
}
+
if (p->replacement) {
/* We must have just cleared 'rdev' */
p->rdev = 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 = 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 = -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 = -EBUSY;
+ rcu_assign_pointer(*rdevp, rdev);
}
+
if (!err) {
err = log_modify(conf, rdev, false);
if (err)
--
2.39.2
+ 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 PM Yu Kuai <[email protected]> wrote:
>
> From: Yu Kuai <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 *mddev, struct md_rdev *rdev)
> goto abort;
> }
> p->rdev = NULL;
> - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> - synchronize_rcu();
> - if (atomic_read(&rdev->nr_pending)) {
> - /* lost the race, try later */
> - err = -EBUSY;
> - p->rdev = rdev;
> - goto abort;
> - }
> +
> + synchronize_rcu();
> + if (atomic_read(&rdev->nr_pending)) {
> + /* lost the race, try later */
> + err = -EBUSY;
> + p->rdev = rdev;
> + goto abort;
> }
> err = 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 *mddev,
> struct md_rdev *rdev;
> int spares = 0;
> int removed = 0;
> - bool remove_some = 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 *mddev,
> if ((this == NULL || rdev == this) &&
> rdev->raid_disk >= 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)==0) {
> - /* Faulty non-Blocked devices with nr_pending == 0
> - * never get nr_pending incremented,
> - * never get Faulty cleared, and never get Blocked set.
> - * So we can synchronize_rcu now rather than once per device
> - */
> - remove_some = true;
> - set_bit(RemoveSynchronized, &rdev->flags);
> - }
> - }
> -
> - if (remove_some)
> - synchronize_rcu();
> - rdev_for_each(rdev, mddev) {
> - if ((this == NULL || rdev == this) &&
> - rdev->raid_disk >= 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)==0)) {
> if (mddev->pers->hot_remove_disk(
> mddev, rdev) == 0) {
> sysfs_unlink_rdev(mddev, rdev);
> @@ -9195,8 +9175,6 @@ static int remove_and_add_spares(struct mddev *mddev,
> removed++;
> }
> }
> - if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
> - 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 = NULL;
> - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> - synchronize_rcu();
> - if (atomic_read(&rdev->nr_pending)) {
> - /* lost the race, try later */
> - err = -EBUSY;
> - p->rdev = rdev;
> - goto abort;
> - }
> + synchronize_rcu();
> + if (atomic_read(&rdev->nr_pending)) {
> + /* lost the race, try later */
> + err = -EBUSY;
> + p->rdev = rdev;
> + goto abort;
> }
> +
> if (conf->mirrors[conf->raid_disks + number].rdev) {
> /* We just removed a device that is being replaced.
> * Move down the replacement. We drain all IO before
> 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 = NULL;
> - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> - synchronize_rcu();
> - if (atomic_read(&rdev->nr_pending)) {
> - /* lost the race, try later */
> - err = -EBUSY;
> - *rdevp = rdev;
> - goto abort;
> - }
> + synchronize_rcu();
> + if (atomic_read(&rdev->nr_pending)) {
> + /* lost the race, try later */
> + err = -EBUSY;
> + *rdevp = rdev;
> + goto abort;
> }
> +
> if (p->replacement) {
> /* We must have just cleared 'rdev' */
> p->rdev = 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 = 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 = -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 = -EBUSY;
> + rcu_assign_pointer(*rdevp, rdev);
> }
> +
> if (!err) {
> err = log_modify(conf, rdev, false);
> if (err)
> --
> 2.39.2
>
Hi,
在 2023/08/15 5:36, NeilBrown 写道:
> On Mon, 14 Aug 2023, Song Liu wrote:
>> + 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).
>
> We have customers with thousands of members in a raid10. They do
> sometimes need to remove half of them. Dropping RemoveSynchronized
> would significantly increase the time it takes to do that.
>
> Removing an rdev is not often a hot path. But sometimes it can be.
>
Thanks for the explanation, and soory that I didn't mention that in this
commit message, the way that rdev is protected through rcu is not safe,
and refer to the implementation of 'q_usage_counter' in block layer, I'm
planing to refactor the way that rdev is protected and rcu is not needed
anymore:
1. refactor mddev_suspend(), and make sure it's called by all the caller
of remove_and_add_spares().
2. factor out a helper md_array_enter() from md_handle_request, this
helper will grab 'active_io' and wait for mddev_suspend() to be done.
3. in the fastpath, before accessing rdev, make sure md_array_enter() is
called.
Thanks,
Kuai
> NeilBrown
>
>
>>
>> Thanks,
>> Song
>>
>>
>> On Thu, Aug 3, 2023 at 9:27 PM Yu Kuai <[email protected]> wrote:
>>>
>>> From: Yu Kuai <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 *mddev, struct md_rdev *rdev)
>>> goto abort;
>>> }
>>> p->rdev = NULL;
>>> - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>>> - synchronize_rcu();
>>> - if (atomic_read(&rdev->nr_pending)) {
>>> - /* lost the race, try later */
>>> - err = -EBUSY;
>>> - p->rdev = rdev;
>>> - goto abort;
>>> - }
>>> +
>>> + synchronize_rcu();
>>> + if (atomic_read(&rdev->nr_pending)) {
>>> + /* lost the race, try later */
>>> + err = -EBUSY;
>>> + p->rdev = rdev;
>>> + goto abort;
>>> }
>>> err = 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 *mddev,
>>> struct md_rdev *rdev;
>>> int spares = 0;
>>> int removed = 0;
>>> - bool remove_some = 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 *mddev,
>>> if ((this == NULL || rdev == this) &&
>>> rdev->raid_disk >= 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)==0) {
>>> - /* Faulty non-Blocked devices with nr_pending == 0
>>> - * never get nr_pending incremented,
>>> - * never get Faulty cleared, and never get Blocked set.
>>> - * So we can synchronize_rcu now rather than once per device
>>> - */
>>> - remove_some = true;
>>> - set_bit(RemoveSynchronized, &rdev->flags);
>>> - }
>>> - }
>>> -
>>> - if (remove_some)
>>> - synchronize_rcu();
>>> - rdev_for_each(rdev, mddev) {
>>> - if ((this == NULL || rdev == this) &&
>>> - rdev->raid_disk >= 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)==0)) {
>>> if (mddev->pers->hot_remove_disk(
>>> mddev, rdev) == 0) {
>>> sysfs_unlink_rdev(mddev, rdev);
>>> @@ -9195,8 +9175,6 @@ static int remove_and_add_spares(struct mddev *mddev,
>>> removed++;
>>> }
>>> }
>>> - if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
>>> - 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 = NULL;
>>> - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>>> - synchronize_rcu();
>>> - if (atomic_read(&rdev->nr_pending)) {
>>> - /* lost the race, try later */
>>> - err = -EBUSY;
>>> - p->rdev = rdev;
>>> - goto abort;
>>> - }
>>> + synchronize_rcu();
>>> + if (atomic_read(&rdev->nr_pending)) {
>>> + /* lost the race, try later */
>>> + err = -EBUSY;
>>> + p->rdev = rdev;
>>> + goto abort;
>>> }
>>> +
>>> if (conf->mirrors[conf->raid_disks + number].rdev) {
>>> /* We just removed a device that is being replaced.
>>> * Move down the replacement. We drain all IO before
>>> 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 = NULL;
>>> - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
>>> - synchronize_rcu();
>>> - if (atomic_read(&rdev->nr_pending)) {
>>> - /* lost the race, try later */
>>> - err = -EBUSY;
>>> - *rdevp = rdev;
>>> - goto abort;
>>> - }
>>> + synchronize_rcu();
>>> + if (atomic_read(&rdev->nr_pending)) {
>>> + /* lost the race, try later */
>>> + err = -EBUSY;
>>> + *rdevp = rdev;
>>> + goto abort;
>>> }
>>> +
>>> if (p->replacement) {
>>> /* We must have just cleared 'rdev' */
>>> p->rdev = 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 = 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 = -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 = -EBUSY;
>>> + rcu_assign_pointer(*rdevp, rdev);
>>> }
>>> +
>>> if (!err) {
>>> err = log_modify(conf, rdev, false);
>>> if (err)
>>> --
>>> 2.39.2
>>>
>>
>
>
> .
>
On Mon, 14 Aug 2023, Song Liu wrote:
> + 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).
We have customers with thousands of members in a raid10. They do
sometimes need to remove half of them. Dropping RemoveSynchronized
would significantly increase the time it takes to do that.
Removing an rdev is not often a hot path. But sometimes it can be.
NeilBrown
>
> Thanks,
> Song
>
>
> On Thu, Aug 3, 2023 at 9:27 PM Yu Kuai <[email protected]> wrote:
> >
> > From: Yu Kuai <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 *mddev, struct md_rdev *rdev)
> > goto abort;
> > }
> > p->rdev = NULL;
> > - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> > - synchronize_rcu();
> > - if (atomic_read(&rdev->nr_pending)) {
> > - /* lost the race, try later */
> > - err = -EBUSY;
> > - p->rdev = rdev;
> > - goto abort;
> > - }
> > +
> > + synchronize_rcu();
> > + if (atomic_read(&rdev->nr_pending)) {
> > + /* lost the race, try later */
> > + err = -EBUSY;
> > + p->rdev = rdev;
> > + goto abort;
> > }
> > err = 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 *mddev,
> > struct md_rdev *rdev;
> > int spares = 0;
> > int removed = 0;
> > - bool remove_some = 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 *mddev,
> > if ((this == NULL || rdev == this) &&
> > rdev->raid_disk >= 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)==0) {
> > - /* Faulty non-Blocked devices with nr_pending == 0
> > - * never get nr_pending incremented,
> > - * never get Faulty cleared, and never get Blocked set.
> > - * So we can synchronize_rcu now rather than once per device
> > - */
> > - remove_some = true;
> > - set_bit(RemoveSynchronized, &rdev->flags);
> > - }
> > - }
> > -
> > - if (remove_some)
> > - synchronize_rcu();
> > - rdev_for_each(rdev, mddev) {
> > - if ((this == NULL || rdev == this) &&
> > - rdev->raid_disk >= 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)==0)) {
> > if (mddev->pers->hot_remove_disk(
> > mddev, rdev) == 0) {
> > sysfs_unlink_rdev(mddev, rdev);
> > @@ -9195,8 +9175,6 @@ static int remove_and_add_spares(struct mddev *mddev,
> > removed++;
> > }
> > }
> > - if (remove_some && test_bit(RemoveSynchronized, &rdev->flags))
> > - 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 = NULL;
> > - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> > - synchronize_rcu();
> > - if (atomic_read(&rdev->nr_pending)) {
> > - /* lost the race, try later */
> > - err = -EBUSY;
> > - p->rdev = rdev;
> > - goto abort;
> > - }
> > + synchronize_rcu();
> > + if (atomic_read(&rdev->nr_pending)) {
> > + /* lost the race, try later */
> > + err = -EBUSY;
> > + p->rdev = rdev;
> > + goto abort;
> > }
> > +
> > if (conf->mirrors[conf->raid_disks + number].rdev) {
> > /* We just removed a device that is being replaced.
> > * Move down the replacement. We drain all IO before
> > 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 = NULL;
> > - if (!test_bit(RemoveSynchronized, &rdev->flags)) {
> > - synchronize_rcu();
> > - if (atomic_read(&rdev->nr_pending)) {
> > - /* lost the race, try later */
> > - err = -EBUSY;
> > - *rdevp = rdev;
> > - goto abort;
> > - }
> > + synchronize_rcu();
> > + if (atomic_read(&rdev->nr_pending)) {
> > + /* lost the race, try later */
> > + err = -EBUSY;
> > + *rdevp = rdev;
> > + goto abort;
> > }
> > +
> > if (p->replacement) {
> > /* We must have just cleared 'rdev' */
> > p->rdev = 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 = 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 = -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 = -EBUSY;
> > + rcu_assign_pointer(*rdevp, rdev);
> > }
> > +
> > if (!err) {
> > err = log_modify(conf, rdev, false);
> > if (err)
> > --
> > 2.39.2
> >
>