2023-08-15 08:11:35

by Yu Kuai

[permalink] [raw]
Subject: [PATCH -next v2 0/7] md: make rdev addition and removal independent from daemon thread

From: Yu Kuai <[email protected]>

Changes in v2:
- remove patch 1 from v1 and some related patches, those patches will
be sent later when rcu protection for rdev is removed.
- add patch 2.

This is the third patchset to do some preparatory work to synchronize
io with array reconfiguration.

1) The first patchset refactor 'active_io', make sure that mddev_suspend()
will wait for io to be done. [1]

2) The second patchset remove 'quiesce' callback from mddev_suspend(), so
that mddev_suspend() doesn't rely on 'quiesce' callback is registered,
and can be used for all personalites; [2]

3) This patchset make array reconfiguration independent from daemon thread,
and synchronize it with io will be much easier because io may rely on
daemon thread to be done.

More patchset on the way!

Yu Kuai (7):
md: use separate work_struct for md_start_sync()
md: factor out a helper to choose sync direction from
md_check_recovery()
md: delay choosing sync direction to md_start_sync()
md: factor out a helper rdev_removeable() from remove_and_add_spares()
md: factor out a helper rdev_is_spare() from remove_and_add_spares()
md: factor out a helper rdev_addable() from remove_and_add_spares()
md: delay remove_and_add_spares() for read only array to
md_start_sync()

drivers/md/md.c | 257 +++++++++++++++++++++++++++++-------------------
drivers/md/md.h | 5 +-
2 files changed, 160 insertions(+), 102 deletions(-)

--
2.39.2



2023-08-15 08:57:59

by Yu Kuai

[permalink] [raw]
Subject: [PATCH -next v2 2/7] md: factor out a helper to choose sync direction from md_check_recovery()

From: Yu Kuai <[email protected]>

There are no functional changes, on the one hand make the code cleaner,
on the other hand prevent following checkpatch error in the next patch to
delay choosing sync direction to md_start_sync().

ERROR: do not use assignment in if condition
+ } else if ((spares = remove_and_add_spares(mddev, NULL))) {

Signed-off-by: Yu Kuai <[email protected]>
---
drivers/md/md.c | 68 +++++++++++++++++++++++++++++++------------------
1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 90815be1e80f..4846ff6d25b0 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9246,6 +9246,48 @@ static int remove_and_add_spares(struct mddev *mddev,
return spares;
}

+static bool md_choose_sync_direction(struct mddev *mddev, int *spares)
+{
+ /* check reshape first */
+ if (mddev->reshape_position != MaxSector) {
+ if (mddev->pers->check_reshape == NULL ||
+ mddev->pers->check_reshape(mddev) != 0)
+ return false;
+
+ set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
+ clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
+ return true;
+ }
+
+ /*
+ * remove any failed drives, then add spares if possible. Spares are
+ * also removed and re-added, to allow the personality to fail the
+ * re-add.
+ */
+ *spares = remove_and_add_spares(mddev, NULL);
+ if (*spares) {
+ clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
+ clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
+ clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
+ set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
+ return true;
+ }
+
+ /* check recovery */
+ if (mddev->recovery_cp < MaxSector) {
+ set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
+ clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
+ return true;
+ }
+
+ /* check resync */
+ if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
+ return true;
+
+ /* nothing to be done */
+ return false;
+}
+
static void md_start_sync(struct work_struct *ws)
{
struct mddev *mddev = container_of(ws, struct mddev, sync_work);
@@ -9427,32 +9469,8 @@ void md_check_recovery(struct mddev *mddev)
if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
goto not_running;
- /* no recovery is running.
- * remove any failed drives, then
- * add spares if possible.
- * Spares are also removed and re-added, to allow
- * the personality to fail the re-add.
- */
-
- if (mddev->reshape_position != MaxSector) {
- if (mddev->pers->check_reshape == NULL ||
- mddev->pers->check_reshape(mddev) != 0)
- /* Cannot proceed */
- goto not_running;
- set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
- clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
- } else if ((spares = remove_and_add_spares(mddev, NULL))) {
- clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
- clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
- clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
- set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
- } else if (mddev->recovery_cp < MaxSector) {
- set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
- clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
- } else if (!test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
- /* nothing to be done ... */
+ if (!md_choose_sync_direction(mddev, &spares))
goto not_running;
-
if (mddev->pers->sync_request) {
if (spares) {
/* We are adding a device or devices to an array
--
2.39.2


2023-08-15 15:29:34

by Yu Kuai

[permalink] [raw]
Subject: [PATCH -next v2 6/7] md: factor out a helper rdev_addable() from remove_and_add_spares()

From: Yu Kuai <[email protected]>

There are no functional changes, just to make the code simpler and
prepare to delay remove_and_add_spares() to md_start_sync().

Signed-off-by: Yu Kuai <[email protected]>
---
drivers/md/md.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 6baaa4d314b3..d26d2c35f9af 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9177,6 +9177,20 @@ static bool rdev_is_spare(struct md_rdev *rdev)
!test_bit(Faulty, &rdev->flags);
}

+static bool rdev_addable(struct md_rdev *rdev)
+{
+ if (test_bit(Candidate, &rdev->flags) || rdev->raid_disk >= 0 ||
+ test_bit(Faulty, &rdev->flags))
+ return false;
+
+ if (!test_bit(Journal, &rdev->flags) && !md_is_rdwr(rdev->mddev) &&
+ !(rdev->saved_raid_disk >= 0 &&
+ !test_bit(Bitmap_sync, &rdev->flags)))
+ return false;
+
+ return true;
+}
+
static int remove_and_add_spares(struct mddev *mddev,
struct md_rdev *this)
{
@@ -9227,20 +9241,10 @@ static int remove_and_add_spares(struct mddev *mddev,
continue;
if (rdev_is_spare(rdev))
spares++;
- if (test_bit(Candidate, &rdev->flags))
+ if (!rdev_addable(rdev))
continue;
- if (rdev->raid_disk >= 0)
- continue;
- if (test_bit(Faulty, &rdev->flags))
- continue;
- if (!test_bit(Journal, &rdev->flags)) {
- if (!md_is_rdwr(mddev) &&
- !(rdev->saved_raid_disk >= 0 &&
- !test_bit(Bitmap_sync, &rdev->flags)))
- continue;
-
+ if (!test_bit(Journal, &rdev->flags))
rdev->recovery_offset = 0;
- }
if (mddev->pers->hot_add_disk(mddev, rdev) == 0) {
/* failure here is OK */
sysfs_link_rdev(mddev, rdev);
--
2.39.2


2023-08-18 11:05:53

by Yu Kuai

[permalink] [raw]
Subject: [PATCH -next v2 5/7] md: factor out a helper rdev_is_spare() from remove_and_add_spares()

From: Yu Kuai <[email protected]>

There are no functional changes, just to make the code simpler and
prepare to delay remove_and_add_spares() to md_start_sync().

Signed-off-by: Yu Kuai <[email protected]>
---
drivers/md/md.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index ea091eef23d1..6baaa4d314b3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9169,6 +9169,14 @@ static bool rdev_removeable(struct md_rdev *rdev)
return true;
}

+static bool rdev_is_spare(struct md_rdev *rdev)
+{
+ return !test_bit(Candidate, &rdev->flags) && rdev->raid_disk >= 0 &&
+ !test_bit(In_sync, &rdev->flags) &&
+ !test_bit(Journal, &rdev->flags) &&
+ !test_bit(Faulty, &rdev->flags);
+}
+
static int remove_and_add_spares(struct mddev *mddev,
struct md_rdev *this)
{
@@ -9217,13 +9225,10 @@ static int remove_and_add_spares(struct mddev *mddev,
rdev_for_each(rdev, mddev) {
if (this && this != rdev)
continue;
+ if (rdev_is_spare(rdev))
+ spares++;
if (test_bit(Candidate, &rdev->flags))
continue;
- if (rdev->raid_disk >= 0 &&
- !test_bit(In_sync, &rdev->flags) &&
- !test_bit(Journal, &rdev->flags) &&
- !test_bit(Faulty, &rdev->flags))
- spares++;
if (rdev->raid_disk >= 0)
continue;
if (test_bit(Faulty, &rdev->flags))
--
2.39.2


2023-08-19 13:13:46

by Song Liu

[permalink] [raw]
Subject: Re: [PATCH -next v2 2/7] md: factor out a helper to choose sync direction from md_check_recovery()

On Thu, Aug 17, 2023 at 12:58 AM Mariusz Tkaczyk
<[email protected]> wrote:
>
> On Tue, 15 Aug 2023 11:09:52 +0800
> Yu Kuai <[email protected]> wrote:
>
> > From: Yu Kuai <[email protected]>
> >
> > There are no functional changes, on the one hand make the code cleaner,
> > on the other hand prevent following checkpatch error in the next patch to
> > delay choosing sync direction to md_start_sync().
> >
> > ERROR: do not use assignment in if condition
> > + } else if ((spares = remove_and_add_spares(mddev, NULL))) {
> >
> > Signed-off-by: Yu Kuai <[email protected]>
> > ---
> > drivers/md/md.c | 68 +++++++++++++++++++++++++++++++------------------
> > 1 file changed, 43 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 90815be1e80f..4846ff6d25b0 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -9246,6 +9246,48 @@ static int remove_and_add_spares(struct mddev *mddev,
> > return spares;
> > }
> >
> > +static bool md_choose_sync_direction(struct mddev *mddev, int *spares)
>
> The naming is little confusing because as a direction I would expect forward or
> backward - from end to start or or from start to end. In this case you are
> determining the type of the background operation needed. Assuming that reshape
> is a kind of "sync" operation I would say "md_choose_sync_action".

Yeah, md_choose_sync_direction is indeed confusing.

Thanks,
Song

2023-08-19 17:08:30

by Yu Kuai

[permalink] [raw]
Subject: [PATCH -next v2 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync()

From: Yu Kuai <[email protected]>

Before this patch, for read-only array:

md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, then it will
call remove_and_add_spares() directly to try to remove and add rdevs
from array.

After this patch:

1) md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, and the
worker 'sync_work' is not pending, and there are rdevs can be added
or removed, then it will queue new work md_start_sync();
2) md_start_sync() will call remove_and_add_spares() and exist;

This change make sure that array reconfiguration is independent from
daemon, and it'll be much easier to synchronize it with io, consier
that io may rely on daemon thread to be done.

Signed-off-by: Yu Kuai <[email protected]>
---
drivers/md/md.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d26d2c35f9af..74d529479fcf 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9191,6 +9191,16 @@ static bool rdev_addable(struct md_rdev *rdev)
return true;
}

+static bool md_spares_need_change(struct mddev *mddev)
+{
+ struct md_rdev *rdev;
+
+ rdev_for_each(rdev, mddev)
+ if (rdev_removeable(rdev) || rdev_addable(rdev))
+ return true;
+ return false;
+}
+
static int remove_and_add_spares(struct mddev *mddev,
struct md_rdev *this)
{
@@ -9309,6 +9319,12 @@ static void md_start_sync(struct work_struct *ws)

mddev_lock_nointr(mddev);

+ if (!md_is_rdwr(mddev)) {
+ remove_and_add_spares(mddev, NULL);
+ mddev_unlock(mddev);
+ return;
+ }
+
if (!md_choose_sync_direction(mddev, &spares))
goto not_running;

@@ -9403,7 +9419,8 @@ void md_check_recovery(struct mddev *mddev)
}

if (!md_is_rdwr(mddev) &&
- !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery))
+ (!test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
+ work_pending(&mddev->sync_work)))
return;
if ( ! (
(mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
@@ -9431,15 +9448,8 @@ void md_check_recovery(struct mddev *mddev)
*/
rdev_for_each(rdev, mddev)
clear_bit(Blocked, &rdev->flags);
- /* On a read-only array we can:
- * - remove failed devices
- * - add already-in_sync devices if the array itself
- * is in-sync.
- * As we only add devices that are already in-sync,
- * we can activate the spares immediately.
- */
- remove_and_add_spares(mddev, NULL);
- /* There is no thread, but we need to call
+ /*
+ * There is no thread, but we need to call
* ->spare_active and clear saved_raid_disk
*/
set_bit(MD_RECOVERY_INTR, &mddev->recovery);
@@ -9447,6 +9457,13 @@ void md_check_recovery(struct mddev *mddev)
clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);
+
+ /*
+ * Let md_start_sync() to remove and add rdevs to the
+ * array.
+ */
+ if (md_spares_need_change(mddev))
+ queue_work(md_misc_wq, &mddev->sync_work);
goto unlock;
}

--
2.39.2


2023-08-19 23:38:48

by Xiao Ni

[permalink] [raw]
Subject: Re: [PATCH -next v2 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync()

On Tue, Aug 15, 2023 at 11:13 AM Yu Kuai <[email protected]> wrote:
>
> From: Yu Kuai <[email protected]>
>
> Before this patch, for read-only array:
>
> md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, then it will
> call remove_and_add_spares() directly to try to remove and add rdevs
> from array.
>
> After this patch:
>
> 1) md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, and the
> worker 'sync_work' is not pending, and there are rdevs can be added
> or removed, then it will queue new work md_start_sync();
> 2) md_start_sync() will call remove_and_add_spares() and exist;

Hi Kuai

If md_check_recovery returns and md_starts_sync doesn't start, during
the window the raid changes to read-write, the sync thread can be run
but MD_RECOVERY_RUNNING isn't set. Is there such a problem?

Regards
Xiao

>
> This change make sure that array reconfiguration is independent from
> daemon, and it'll be much easier to synchronize it with io, consier
> that io may rely on daemon thread to be done.
>
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> drivers/md/md.c | 37 +++++++++++++++++++++++++++----------
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d26d2c35f9af..74d529479fcf 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9191,6 +9191,16 @@ static bool rdev_addable(struct md_rdev *rdev)
> return true;
> }
>
> +static bool md_spares_need_change(struct mddev *mddev)
> +{
> + struct md_rdev *rdev;
> +
> + rdev_for_each(rdev, mddev)
> + if (rdev_removeable(rdev) || rdev_addable(rdev))
> + return true;
> + return false;
> +}
> +
> static int remove_and_add_spares(struct mddev *mddev,
> struct md_rdev *this)
> {
> @@ -9309,6 +9319,12 @@ static void md_start_sync(struct work_struct *ws)
>
> mddev_lock_nointr(mddev);
>
> + if (!md_is_rdwr(mddev)) {
> + remove_and_add_spares(mddev, NULL);
> + mddev_unlock(mddev);
> + return;
> + }
> +
> if (!md_choose_sync_direction(mddev, &spares))
> goto not_running;
>
> @@ -9403,7 +9419,8 @@ void md_check_recovery(struct mddev *mddev)
> }
>
> if (!md_is_rdwr(mddev) &&
> - !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery))
> + (!test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> + work_pending(&mddev->sync_work)))
> return;
> if ( ! (
> (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
> @@ -9431,15 +9448,8 @@ void md_check_recovery(struct mddev *mddev)
> */
> rdev_for_each(rdev, mddev)
> clear_bit(Blocked, &rdev->flags);
> - /* On a read-only array we can:
> - * - remove failed devices
> - * - add already-in_sync devices if the array itself
> - * is in-sync.
> - * As we only add devices that are already in-sync,
> - * we can activate the spares immediately.
> - */
> - remove_and_add_spares(mddev, NULL);
> - /* There is no thread, but we need to call
> + /*
> + * There is no thread, but we need to call
> * ->spare_active and clear saved_raid_disk
> */
> set_bit(MD_RECOVERY_INTR, &mddev->recovery);
> @@ -9447,6 +9457,13 @@ void md_check_recovery(struct mddev *mddev)
> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);
> +
> + /*
> + * Let md_start_sync() to remove and add rdevs to the
> + * array.
> + */
> + if (md_spares_need_change(mddev))
> + queue_work(md_misc_wq, &mddev->sync_work);
> goto unlock;
> }
>
> --
> 2.39.2
>


2023-08-20 05:59:24

by Yu Kuai

[permalink] [raw]
Subject: Re: [PATCH -next v2 7/7] md: delay remove_and_add_spares() for read only array to md_start_sync()

Hi,

在 2023/08/16 15:18, Xiao Ni 写道:
> On Tue, Aug 15, 2023 at 11:13 AM Yu Kuai <[email protected]> wrote:
>>
>> From: Yu Kuai <[email protected]>
>>
>> Before this patch, for read-only array:
>>
>> md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, then it will
>> call remove_and_add_spares() directly to try to remove and add rdevs
>> from array.
>>
>> After this patch:
>>
>> 1) md_check_recovery() check that 'MD_RECOVERY_NEEDED' is set, and the
>> worker 'sync_work' is not pending, and there are rdevs can be added
>> or removed, then it will queue new work md_start_sync();
>> 2) md_start_sync() will call remove_and_add_spares() and exist;
>
> Hi Kuai
>
> If md_check_recovery returns and md_starts_sync doesn't start, during
> the window the raid changes to read-write, the sync thread can be run
> but MD_RECOVERY_RUNNING isn't set. Is there such a problem?

That's a good question, looks like this is possible. Read-only array
doesn't test or set MD_RECOVERY_RUNNING at all in md_check_recovery().
I'll use MD_RECOVERY_RUNNING to prevent such concurrent scenario in
the new version.

Thanks,
Kuai

>
> Regards
> Xiao
>
>>
>> This change make sure that array reconfiguration is independent from
>> daemon, and it'll be much easier to synchronize it with io, consier
>> that io may rely on daemon thread to be done.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> drivers/md/md.c | 37 +++++++++++++++++++++++++++----------
>> 1 file changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index d26d2c35f9af..74d529479fcf 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9191,6 +9191,16 @@ static bool rdev_addable(struct md_rdev *rdev)
>> return true;
>> }
>>
>> +static bool md_spares_need_change(struct mddev *mddev)
>> +{
>> + struct md_rdev *rdev;
>> +
>> + rdev_for_each(rdev, mddev)
>> + if (rdev_removeable(rdev) || rdev_addable(rdev))
>> + return true;
>> + return false;
>> +}
>> +
>> static int remove_and_add_spares(struct mddev *mddev,
>> struct md_rdev *this)
>> {
>> @@ -9309,6 +9319,12 @@ static void md_start_sync(struct work_struct *ws)
>>
>> mddev_lock_nointr(mddev);
>>
>> + if (!md_is_rdwr(mddev)) {
>> + remove_and_add_spares(mddev, NULL);
>> + mddev_unlock(mddev);
>> + return;
>> + }
>> +
>> if (!md_choose_sync_direction(mddev, &spares))
>> goto not_running;
>>
>> @@ -9403,7 +9419,8 @@ void md_check_recovery(struct mddev *mddev)
>> }
>>
>> if (!md_is_rdwr(mddev) &&
>> - !test_bit(MD_RECOVERY_NEEDED, &mddev->recovery))
>> + (!test_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>> + work_pending(&mddev->sync_work)))
>> return;
>> if ( ! (
>> (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING)) ||
>> @@ -9431,15 +9448,8 @@ void md_check_recovery(struct mddev *mddev)
>> */
>> rdev_for_each(rdev, mddev)
>> clear_bit(Blocked, &rdev->flags);
>> - /* On a read-only array we can:
>> - * - remove failed devices
>> - * - add already-in_sync devices if the array itself
>> - * is in-sync.
>> - * As we only add devices that are already in-sync,
>> - * we can activate the spares immediately.
>> - */
>> - remove_and_add_spares(mddev, NULL);
>> - /* There is no thread, but we need to call
>> + /*
>> + * There is no thread, but we need to call
>> * ->spare_active and clear saved_raid_disk
>> */
>> set_bit(MD_RECOVERY_INTR, &mddev->recovery);
>> @@ -9447,6 +9457,13 @@ void md_check_recovery(struct mddev *mddev)
>> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
>> clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>> clear_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags);
>> +
>> + /*
>> + * Let md_start_sync() to remove and add rdevs to the
>> + * array.
>> + */
>> + if (md_spares_need_change(mddev))
>> + queue_work(md_misc_wq, &mddev->sync_work);
>> goto unlock;
>> }
>>
>> --
>> 2.39.2
>>
>
> .
>


2023-08-21 04:14:10

by Yu Kuai

[permalink] [raw]
Subject: Re: [PATCH -next v2 2/7] md: factor out a helper to choose sync direction from md_check_recovery()

Hi,

在 2023/08/18 5:49, Song Liu 写道:
> On Thu, Aug 17, 2023 at 12:58 AM Mariusz Tkaczyk
> <[email protected]> wrote:
>>
>> On Tue, 15 Aug 2023 11:09:52 +0800
>> Yu Kuai <[email protected]> wrote:
>>
>>> From: Yu Kuai <[email protected]>
>>>
>>> There are no functional changes, on the one hand make the code cleaner,
>>> on the other hand prevent following checkpatch error in the next patch to
>>> delay choosing sync direction to md_start_sync().
>>>
>>> ERROR: do not use assignment in if condition
>>> + } else if ((spares = remove_and_add_spares(mddev, NULL))) {
>>>
>>> Signed-off-by: Yu Kuai <[email protected]>
>>> ---
>>> drivers/md/md.c | 68 +++++++++++++++++++++++++++++++------------------
>>> 1 file changed, 43 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>> index 90815be1e80f..4846ff6d25b0 100644
>>> --- a/drivers/md/md.c
>>> +++ b/drivers/md/md.c
>>> @@ -9246,6 +9246,48 @@ static int remove_and_add_spares(struct mddev *mddev,
>>> return spares;
>>> }
>>>
>>> +static bool md_choose_sync_direction(struct mddev *mddev, int *spares)
>>
>> The naming is little confusing because as a direction I would expect forward or
>> backward - from end to start or or from start to end. In this case you are
>> determining the type of the background operation needed. Assuming that reshape
>> is a kind of "sync" operation I would say "md_choose_sync_action".
>
> Yeah, md_choose_sync_direction is indeed confusing.
>

Thanks for the suggestion, I'll update this in the new version.

Kuai,

> Thanks,
> Song
> .
>