diff mbox series

[md-6.10,3/9] md: add new helpers for sync_action

Message ID 20240509011900.2694291-4-yukuai1@huaweicloud.com (mailing list archive)
State Not Applicable, archived
Delegated to: Mike Snitzer
Headers show
Series md: refactor and cleanup for sync action | expand

Commit Message

Yu Kuai May 9, 2024, 1:18 a.m. UTC
From: Yu Kuai <yukuai3@huawei.com>

The new helpers will get current sync_action of the array, will be used
in later patches to make code cleaner.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/md/md.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/md/md.h |  3 +++
 2 files changed, 67 insertions(+)

Comments

Xiao Ni May 14, 2024, 6:52 a.m. UTC | #1
On Mon, May 13, 2024 at 5:31 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> The new helpers will get current sync_action of the array, will be used
> in later patches to make code cleaner.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/md/md.h |  3 +++
>  2 files changed, 67 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 00bbafcd27bb..48ec35342d1b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -69,6 +69,16 @@
>  #include "md-bitmap.h"
>  #include "md-cluster.h"
>
> +static char *action_name[NR_SYNC_ACTIONS] = {
> +       [ACTION_RESYNC]         = "resync",
> +       [ACTION_RECOVER]        = "recover",
> +       [ACTION_CHECK]          = "check",
> +       [ACTION_REPAIR]         = "repair",
> +       [ACTION_RESHAPE]        = "reshape",
> +       [ACTION_FROZEN]         = "frozen",
> +       [ACTION_IDLE]           = "idle",
> +};
> +
>  /* pers_list is a list of registered personalities protected by pers_lock. */
>  static LIST_HEAD(pers_list);
>  static DEFINE_SPINLOCK(pers_lock);
> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
>  static struct md_sysfs_entry md_metadata =
>  __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, metadata_show, metadata_store);
>
> +enum sync_action md_sync_action(struct mddev *mddev)
> +{
> +       unsigned long recovery = mddev->recovery;
> +
> +       /*
> +        * frozen has the highest priority, means running sync_thread will be
> +        * stopped immediately, and no new sync_thread can start.
> +        */
> +       if (test_bit(MD_RECOVERY_FROZEN, &recovery))
> +               return ACTION_FROZEN;
> +
> +       /*
> +        * idle means no sync_thread is running, and no new sync_thread is
> +        * requested.
> +        */
> +       if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
> +           (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, &recovery)))
> +               return ACTION_IDLE;

Hi Kuai

Can I think the above judgement cover these two situations:
1. The array is readonly / readauto and it doesn't have
MD_RECOVERY_RUNNING. Now maybe it has MD_RECOVERY_NEEDED, it means one
array may want to do some sync action, but the array state is not
readwrite and it can't start.
2. The array doesn't have MD_RECOVERY_RUNNING and MD_RECOVERY_NEEDED

> +
> +       if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
> +           mddev->reshape_position != MaxSector)
> +               return ACTION_RESHAPE;
> +
> +       if (test_bit(MD_RECOVERY_RECOVER, &recovery))
> +               return ACTION_RECOVER;
> +
> +       if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
> +               if (test_bit(MD_RECOVERY_CHECK, &recovery))
> +                       return ACTION_CHECK;
> +               if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
> +                       return ACTION_REPAIR;
> +               return ACTION_RESYNC;
> +       }
> +
> +       return ACTION_IDLE;

Does it need this? I guess it's the reason in case there are other
situations, right?

Regards
Xiao

> +}
> +
> +enum sync_action md_sync_action_by_name(char *page)
> +{
> +       enum sync_action action;
> +
> +       for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
> +               if (cmd_match(page, action_name[action]))
> +                       return action;
> +       }
> +
> +       return NR_SYNC_ACTIONS;
> +}
> +
> +char *md_sync_action_name(enum sync_action action)
> +{
> +       return action_name[action];
> +}
> +
>  static ssize_t
>  action_show(struct mddev *mddev, char *page)
>  {
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 2edad966f90a..72ca7a796df5 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **t
>  extern void md_wakeup_thread(struct md_thread __rcu *thread);
>  extern void md_check_recovery(struct mddev *mddev);
>  extern void md_reap_sync_thread(struct mddev *mddev);
> +extern enum sync_action md_sync_action(struct mddev *mddev);
> +extern enum sync_action md_sync_action_by_name(char *page);
> +extern char *md_sync_action_name(enum sync_action action);
>  extern bool md_write_start(struct mddev *mddev, struct bio *bi);
>  extern void md_write_inc(struct mddev *mddev, struct bio *bi);
>  extern void md_write_end(struct mddev *mddev);
> --
> 2.39.2
>
Yu Kuai May 14, 2024, 7:39 a.m. UTC | #2
Hi,

在 2024/05/14 14:52, Xiao Ni 写道:
> On Mon, May 13, 2024 at 5:31 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> The new helpers will get current sync_action of the array, will be used
>> in later patches to make code cleaner.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>   drivers/md/md.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/md/md.h |  3 +++
>>   2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 00bbafcd27bb..48ec35342d1b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -69,6 +69,16 @@
>>   #include "md-bitmap.h"
>>   #include "md-cluster.h"
>>
>> +static char *action_name[NR_SYNC_ACTIONS] = {
>> +       [ACTION_RESYNC]         = "resync",
>> +       [ACTION_RECOVER]        = "recover",
>> +       [ACTION_CHECK]          = "check",
>> +       [ACTION_REPAIR]         = "repair",
>> +       [ACTION_RESHAPE]        = "reshape",
>> +       [ACTION_FROZEN]         = "frozen",
>> +       [ACTION_IDLE]           = "idle",
>> +};
>> +
>>   /* pers_list is a list of registered personalities protected by pers_lock. */
>>   static LIST_HEAD(pers_list);
>>   static DEFINE_SPINLOCK(pers_lock);
>> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
>>   static struct md_sysfs_entry md_metadata =
>>   __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, metadata_show, metadata_store);
>>
>> +enum sync_action md_sync_action(struct mddev *mddev)
>> +{
>> +       unsigned long recovery = mddev->recovery;
>> +
>> +       /*
>> +        * frozen has the highest priority, means running sync_thread will be
>> +        * stopped immediately, and no new sync_thread can start.
>> +        */
>> +       if (test_bit(MD_RECOVERY_FROZEN, &recovery))
>> +               return ACTION_FROZEN;
>> +
>> +       /*
>> +        * idle means no sync_thread is running, and no new sync_thread is
>> +        * requested.
>> +        */
>> +       if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
>> +           (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, &recovery)))
>> +               return ACTION_IDLE;
> 
> Hi Kuai
> 
> Can I think the above judgement cover these two situations:
> 1. The array is readonly / readauto and it doesn't have
> MD_RECOVERY_RUNNING. Now maybe it has MD_RECOVERY_NEEDED, it means one
> array may want to do some sync action, but the array state is not
> readwrite and it can't start.
> 2. The array doesn't have MD_RECOVERY_RUNNING and MD_RECOVERY_NEEDED
> 
>> +
>> +       if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
>> +           mddev->reshape_position != MaxSector)
>> +               return ACTION_RESHAPE;
>> +
>> +       if (test_bit(MD_RECOVERY_RECOVER, &recovery))
>> +               return ACTION_RECOVER;
>> +
>> +       if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
>> +               if (test_bit(MD_RECOVERY_CHECK, &recovery))
>> +                       return ACTION_CHECK;
>> +               if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
>> +                       return ACTION_REPAIR;
>> +               return ACTION_RESYNC;
>> +       }
>> +
>> +       return ACTION_IDLE;
> 
> Does it need this? I guess it's the reason in case there are other
> situations, right?

Yes, we need this, because they are many places to set
MD_RECOVERY_NEEDED, while there are no sync action actually, this case
is 'idle'.

Thanks,
Kuai

> 
> Regards
> Xiao
> 
>> +}
>> +
>> +enum sync_action md_sync_action_by_name(char *page)
>> +{
>> +       enum sync_action action;
>> +
>> +       for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
>> +               if (cmd_match(page, action_name[action]))
>> +                       return action;
>> +       }
>> +
>> +       return NR_SYNC_ACTIONS;
>> +}
>> +
>> +char *md_sync_action_name(enum sync_action action)
>> +{
>> +       return action_name[action];
>> +}
>> +
>>   static ssize_t
>>   action_show(struct mddev *mddev, char *page)
>>   {
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index 2edad966f90a..72ca7a796df5 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **t
>>   extern void md_wakeup_thread(struct md_thread __rcu *thread);
>>   extern void md_check_recovery(struct mddev *mddev);
>>   extern void md_reap_sync_thread(struct mddev *mddev);
>> +extern enum sync_action md_sync_action(struct mddev *mddev);
>> +extern enum sync_action md_sync_action_by_name(char *page);
>> +extern char *md_sync_action_name(enum sync_action action);
>>   extern bool md_write_start(struct mddev *mddev, struct bio *bi);
>>   extern void md_write_inc(struct mddev *mddev, struct bio *bi);
>>   extern void md_write_end(struct mddev *mddev);
>> --
>> 2.39.2
>>
> 
> .
>
Xiao Ni May 14, 2024, 8:40 a.m. UTC | #3
On Tue, May 14, 2024 at 3:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> Hi,
>
> 在 2024/05/14 14:52, Xiao Ni 写道:
> > On Mon, May 13, 2024 at 5:31 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
> >>
> >> From: Yu Kuai <yukuai3@huawei.com>
> >>
> >> The new helpers will get current sync_action of the array, will be used
> >> in later patches to make code cleaner.
> >>
> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> >> ---
> >>   drivers/md/md.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>   drivers/md/md.h |  3 +++
> >>   2 files changed, 67 insertions(+)
> >>
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 00bbafcd27bb..48ec35342d1b 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -69,6 +69,16 @@
> >>   #include "md-bitmap.h"
> >>   #include "md-cluster.h"
> >>
> >> +static char *action_name[NR_SYNC_ACTIONS] = {
> >> +       [ACTION_RESYNC]         = "resync",
> >> +       [ACTION_RECOVER]        = "recover",
> >> +       [ACTION_CHECK]          = "check",
> >> +       [ACTION_REPAIR]         = "repair",
> >> +       [ACTION_RESHAPE]        = "reshape",
> >> +       [ACTION_FROZEN]         = "frozen",
> >> +       [ACTION_IDLE]           = "idle",
> >> +};
> >> +
> >>   /* pers_list is a list of registered personalities protected by pers_lock. */
> >>   static LIST_HEAD(pers_list);
> >>   static DEFINE_SPINLOCK(pers_lock);
> >> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
> >>   static struct md_sysfs_entry md_metadata =
> >>   __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, metadata_show, metadata_store);
> >>
> >> +enum sync_action md_sync_action(struct mddev *mddev)
> >> +{
> >> +       unsigned long recovery = mddev->recovery;
> >> +
> >> +       /*
> >> +        * frozen has the highest priority, means running sync_thread will be
> >> +        * stopped immediately, and no new sync_thread can start.
> >> +        */
> >> +       if (test_bit(MD_RECOVERY_FROZEN, &recovery))
> >> +               return ACTION_FROZEN;
> >> +
> >> +       /*
> >> +        * idle means no sync_thread is running, and no new sync_thread is
> >> +        * requested.
> >> +        */
> >> +       if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
> >> +           (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, &recovery)))
> >> +               return ACTION_IDLE;
> >
> > Hi Kuai
> >
> > Can I think the above judgement cover these two situations:
> > 1. The array is readonly / readauto and it doesn't have
> > MD_RECOVERY_RUNNING. Now maybe it has MD_RECOVERY_NEEDED, it means one
> > array may want to do some sync action, but the array state is not
> > readwrite and it can't start.
> > 2. The array doesn't have MD_RECOVERY_RUNNING and MD_RECOVERY_NEEDED
> >
> >> +
> >> +       if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
> >> +           mddev->reshape_position != MaxSector)
> >> +               return ACTION_RESHAPE;
> >> +
> >> +       if (test_bit(MD_RECOVERY_RECOVER, &recovery))
> >> +               return ACTION_RECOVER;
> >> +
> >> +       if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
> >> +               if (test_bit(MD_RECOVERY_CHECK, &recovery))
> >> +                       return ACTION_CHECK;
> >> +               if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
> >> +                       return ACTION_REPAIR;
> >> +               return ACTION_RESYNC;
> >> +       }
> >> +
> >> +       return ACTION_IDLE;
> >
> > Does it need this? I guess it's the reason in case there are other
> > situations, right?
>
> Yes, we need this, because they are many places to set
> MD_RECOVERY_NEEDED, while there are no sync action actually, this case
> is 'idle'.

To be frank, the logic in action_show is easier to understand than the
logic above. I have taken more than half an hour to think if the logic
here is right or not. In action_show, it only needs to think when it's
not idle and it's easy.

Now this patch logic needs to think in the opposite direction: when
it's idle. And it returns ACTION_IDLE at two places which means it
still needs to think about when it has sync action. So it's better to
keep the original logic in action_show now. It's just my 2 cents
point.

Best Regards
Xiao

>
> Thanks,
> Kuai
>
> >
> > Regards
> > Xiao
> >
> >> +}
> >> +
> >> +enum sync_action md_sync_action_by_name(char *page)
> >> +{
> >> +       enum sync_action action;
> >> +
> >> +       for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
> >> +               if (cmd_match(page, action_name[action]))
> >> +                       return action;
> >> +       }
> >> +
> >> +       return NR_SYNC_ACTIONS;
> >> +}
> >> +
> >> +char *md_sync_action_name(enum sync_action action)
> >> +{
> >> +       return action_name[action];
> >> +}
> >> +
> >>   static ssize_t
> >>   action_show(struct mddev *mddev, char *page)
> >>   {
> >> diff --git a/drivers/md/md.h b/drivers/md/md.h
> >> index 2edad966f90a..72ca7a796df5 100644
> >> --- a/drivers/md/md.h
> >> +++ b/drivers/md/md.h
> >> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **t
> >>   extern void md_wakeup_thread(struct md_thread __rcu *thread);
> >>   extern void md_check_recovery(struct mddev *mddev);
> >>   extern void md_reap_sync_thread(struct mddev *mddev);
> >> +extern enum sync_action md_sync_action(struct mddev *mddev);
> >> +extern enum sync_action md_sync_action_by_name(char *page);
> >> +extern char *md_sync_action_name(enum sync_action action);
> >>   extern bool md_write_start(struct mddev *mddev, struct bio *bi);
> >>   extern void md_write_inc(struct mddev *mddev, struct bio *bi);
> >>   extern void md_write_end(struct mddev *mddev);
> >> --
> >> 2.39.2
> >>
> >
> > .
> >
>
>
Yu Kuai May 14, 2024, 8:52 a.m. UTC | #4
在 2024/05/14 16:40, Xiao Ni 写道:
> On Tue, May 14, 2024 at 3:39 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>
>> Hi,
>>
>> 在 2024/05/14 14:52, Xiao Ni 写道:
>>> On Mon, May 13, 2024 at 5:31 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>>>>
>>>> From: Yu Kuai <yukuai3@huawei.com>
>>>>
>>>> The new helpers will get current sync_action of the array, will be used
>>>> in later patches to make code cleaner.
>>>>
>>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>>>> ---
>>>>    drivers/md/md.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/md/md.h |  3 +++
>>>>    2 files changed, 67 insertions(+)
>>>>
>>>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>>>> index 00bbafcd27bb..48ec35342d1b 100644
>>>> --- a/drivers/md/md.c
>>>> +++ b/drivers/md/md.c
>>>> @@ -69,6 +69,16 @@
>>>>    #include "md-bitmap.h"
>>>>    #include "md-cluster.h"
>>>>
>>>> +static char *action_name[NR_SYNC_ACTIONS] = {
>>>> +       [ACTION_RESYNC]         = "resync",
>>>> +       [ACTION_RECOVER]        = "recover",
>>>> +       [ACTION_CHECK]          = "check",
>>>> +       [ACTION_REPAIR]         = "repair",
>>>> +       [ACTION_RESHAPE]        = "reshape",
>>>> +       [ACTION_FROZEN]         = "frozen",
>>>> +       [ACTION_IDLE]           = "idle",
>>>> +};
>>>> +
>>>>    /* pers_list is a list of registered personalities protected by pers_lock. */
>>>>    static LIST_HEAD(pers_list);
>>>>    static DEFINE_SPINLOCK(pers_lock);
>>>> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const char *buf, size_t len)
>>>>    static struct md_sysfs_entry md_metadata =
>>>>    __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, metadata_show, metadata_store);
>>>>
>>>> +enum sync_action md_sync_action(struct mddev *mddev)
>>>> +{
>>>> +       unsigned long recovery = mddev->recovery;
>>>> +
>>>> +       /*
>>>> +        * frozen has the highest priority, means running sync_thread will be
>>>> +        * stopped immediately, and no new sync_thread can start.
>>>> +        */
>>>> +       if (test_bit(MD_RECOVERY_FROZEN, &recovery))
>>>> +               return ACTION_FROZEN;
>>>> +
>>>> +       /*
>>>> +        * idle means no sync_thread is running, and no new sync_thread is
>>>> +        * requested.
>>>> +        */
>>>> +       if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
>>>> +           (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, &recovery)))
>>>> +               return ACTION_IDLE;
>>>
>>> Hi Kuai
>>>
>>> Can I think the above judgement cover these two situations:
>>> 1. The array is readonly / readauto and it doesn't have
>>> MD_RECOVERY_RUNNING. Now maybe it has MD_RECOVERY_NEEDED, it means one
>>> array may want to do some sync action, but the array state is not
>>> readwrite and it can't start.
>>> 2. The array doesn't have MD_RECOVERY_RUNNING and MD_RECOVERY_NEEDED
>>>
>>>> +
>>>> +       if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
>>>> +           mddev->reshape_position != MaxSector)
>>>> +               return ACTION_RESHAPE;
>>>> +
>>>> +       if (test_bit(MD_RECOVERY_RECOVER, &recovery))
>>>> +               return ACTION_RECOVER;
>>>> +
>>>> +       if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
>>>> +               if (test_bit(MD_RECOVERY_CHECK, &recovery))
>>>> +                       return ACTION_CHECK;
>>>> +               if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
>>>> +                       return ACTION_REPAIR;
>>>> +               return ACTION_RESYNC;
>>>> +       }
>>>> +
>>>> +       return ACTION_IDLE;
>>>
>>> Does it need this? I guess it's the reason in case there are other
>>> situations, right?
>>
>> Yes, we need this, because they are many places to set
>> MD_RECOVERY_NEEDED, while there are no sync action actually, this case
>> is 'idle'.
> 
> To be frank, the logic in action_show is easier to understand than the
> logic above. I have taken more than half an hour to think if the logic
> here is right or not. In action_show, it only needs to think when it's
> not idle and it's easy.
> 
> Now this patch logic needs to think in the opposite direction: when
> it's idle. And it returns ACTION_IDLE at two places which means it
> still needs to think about when it has sync action. So it's better to
> keep the original logic in action_show now. It's just my 2 cents
> point.

Hi,

but the logical is exactlly the same as action_show(), and there are
no functional changes. I just remove the local variable and return
early, because I think code is cleaner this way...

action_show:

char *type = "idle"

if (test_bit() || xxx) {

  if (xxx)
   type ="reshape"
  else if(xxx)
   type ="resync/check/repair"
  else if(xxx)
   type = "recover"
  else if (xxx)
   type = "reshape"
  -> else is idle
}
-> else is idle

The above two place are corresponding to the new code to return
ACTION_IDLE.

Thanks,
Kuai

> 
> Best Regards
> Xiao
> 
>>
>> Thanks,
>> Kuai
>>
>>>
>>> Regards
>>> Xiao
>>>
>>>> +}
>>>> +
>>>> +enum sync_action md_sync_action_by_name(char *page)
>>>> +{
>>>> +       enum sync_action action;
>>>> +
>>>> +       for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
>>>> +               if (cmd_match(page, action_name[action]))
>>>> +                       return action;
>>>> +       }
>>>> +
>>>> +       return NR_SYNC_ACTIONS;
>>>> +}
>>>> +
>>>> +char *md_sync_action_name(enum sync_action action)
>>>> +{
>>>> +       return action_name[action];
>>>> +}
>>>> +
>>>>    static ssize_t
>>>>    action_show(struct mddev *mddev, char *page)
>>>>    {
>>>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>>>> index 2edad966f90a..72ca7a796df5 100644
>>>> --- a/drivers/md/md.h
>>>> +++ b/drivers/md/md.h
>>>> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **t
>>>>    extern void md_wakeup_thread(struct md_thread __rcu *thread);
>>>>    extern void md_check_recovery(struct mddev *mddev);
>>>>    extern void md_reap_sync_thread(struct mddev *mddev);
>>>> +extern enum sync_action md_sync_action(struct mddev *mddev);
>>>> +extern enum sync_action md_sync_action_by_name(char *page);
>>>> +extern char *md_sync_action_name(enum sync_action action);
>>>>    extern bool md_write_start(struct mddev *mddev, struct bio *bi);
>>>>    extern void md_write_inc(struct mddev *mddev, struct bio *bi);
>>>>    extern void md_write_end(struct mddev *mddev);
>>>> --
>>>> 2.39.2
>>>>
>>>
>>> .
>>>
>>
>>
> 
> .
>
Su Yue May 20, 2024, 11:51 a.m. UTC | #5
On Thu 09 May 2024 at 09:18, Yu Kuai <yukuai1@huaweicloud.com> 
wrote:

> From: Yu Kuai <yukuai3@huawei.com>
>
> The new helpers will get current sync_action of the array, will 
> be used
> in later patches to make code cleaner.
>
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 64 
>  +++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/md/md.h |  3 +++
>  2 files changed, 67 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 00bbafcd27bb..48ec35342d1b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -69,6 +69,16 @@
>  #include "md-bitmap.h"
>  #include "md-cluster.h"
>
> +static char *action_name[NR_SYNC_ACTIONS] = {
>

Th array will not be modified, so:

static const char * const action_names[NR_SYNC_ACTIONS]

> +	[ACTION_RESYNC]		= "resync",
> +	[ACTION_RECOVER]	= "recover",
> +	[ACTION_CHECK]		= "check",
> +	[ACTION_REPAIR]		= "repair",
> +	[ACTION_RESHAPE]	= "reshape",
> +	[ACTION_FROZEN]		= "frozen",
> +	[ACTION_IDLE]		= "idle",
> +};
> +
>  /* pers_list is a list of registered personalities protected by 
>  pers_lock. */
>  static LIST_HEAD(pers_list);
>  static DEFINE_SPINLOCK(pers_lock);
> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const 
> char *buf, size_t len)
>  static struct md_sysfs_entry md_metadata =
>  __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, 
>  metadata_show, metadata_store);
>
> +enum sync_action md_sync_action(struct mddev *mddev)
> +{
> +	unsigned long recovery = mddev->recovery;
> +
> +	/*
> +	 * frozen has the highest priority, means running sync_thread 
> will be
> +	 * stopped immediately, and no new sync_thread can start.
> +	 */
> +	if (test_bit(MD_RECOVERY_FROZEN, &recovery))
> +		return ACTION_FROZEN;
> +
> +	/*
> +	 * idle means no sync_thread is running, and no new 
> sync_thread is
> +	 * requested.
> +	 */
> +	if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
> +	    (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, 
> &recovery)))
> +		return ACTION_IDLE;
My brain was lost sometimes looking into nested conditions of md 
code...
I agree with Xiao Ni's suggestion that more comments about the 
array
state should be added.

> +	if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
> +	    mddev->reshape_position != MaxSector)
> +		return ACTION_RESHAPE;
> +
> +	if (test_bit(MD_RECOVERY_RECOVER, &recovery))
> +		return ACTION_RECOVER;
> +
>
In action_show, MD_RECOVERY_SYNC is tested first then 
MD_RECOVERY_RECOVER.
After looking through the logic of MD_RECOVERY_RECOVER 
clear/set_bit, the
change is fine to me. However, better to follow old pattern unless 
there
have resons.


> +	if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
> +		if (test_bit(MD_RECOVERY_CHECK, &recovery))
> +			return ACTION_CHECK;
> +		if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
> +			return ACTION_REPAIR;
> +		return ACTION_RESYNC;
> +	}
> +
> +	return ACTION_IDLE;
> +}
> +
> +enum sync_action md_sync_action_by_name(char *page)
> +{
> +	enum sync_action action;
> +
> +	for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
> +		if (cmd_match(page, action_name[action]))
> +			return action;
> +	}
> +
> +	return NR_SYNC_ACTIONS;
> +}
> +
> +char *md_sync_action_name(enum sync_action action)
>

And 'const char *'

--
Su

> +{
> +	return action_name[action];
> +}
> +
>  static ssize_t
>  action_show(struct mddev *mddev, char *page)
>  {
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 2edad966f90a..72ca7a796df5 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct 
> mddev *mddev, struct md_thread __rcu **t
>  extern void md_wakeup_thread(struct md_thread __rcu *thread);
>  extern void md_check_recovery(struct mddev *mddev);
>  extern void md_reap_sync_thread(struct mddev *mddev);
> +extern enum sync_action md_sync_action(struct mddev *mddev);
> +extern enum sync_action md_sync_action_by_name(char *page);
> +extern char *md_sync_action_name(enum sync_action action);
>  extern bool md_write_start(struct mddev *mddev, struct bio 
>  *bi);
>  extern void md_write_inc(struct mddev *mddev, struct bio *bi);
>  extern void md_write_end(struct mddev *mddev);
Yu Kuai May 21, 2024, 2:30 a.m. UTC | #6
Hi,

在 2024/05/20 19:51, Su Yue 写道:
> 
> On Thu 09 May 2024 at 09:18, Yu Kuai <yukuai1@huaweicloud.com> wrote:
> 
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> The new helpers will get current sync_action of the array, will be used
>> in later patches to make code cleaner.
>>
>> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> ---
>>  drivers/md/md.c | 64  +++++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/md/md.h |  3 +++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 00bbafcd27bb..48ec35342d1b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -69,6 +69,16 @@
>>  #include "md-bitmap.h"
>>  #include "md-cluster.h"
>>
>> +static char *action_name[NR_SYNC_ACTIONS] = {
>>
> 
> Th array will not be modified, so:
> 
> static const char * const action_names[NR_SYNC_ACTIONS]

Yes, this make sense.
> 
>> +    [ACTION_RESYNC]        = "resync",
>> +    [ACTION_RECOVER]    = "recover",
>> +    [ACTION_CHECK]        = "check",
>> +    [ACTION_REPAIR]        = "repair",
>> +    [ACTION_RESHAPE]    = "reshape",
>> +    [ACTION_FROZEN]        = "frozen",
>> +    [ACTION_IDLE]        = "idle",
>> +};
>> +
>>  /* pers_list is a list of registered personalities protected by 
>>  pers_lock. */
>>  static LIST_HEAD(pers_list);
>>  static DEFINE_SPINLOCK(pers_lock);
>> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const char 
>> *buf, size_t len)
>>  static struct md_sysfs_entry md_metadata =
>>  __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR,  metadata_show, 
>> metadata_store);
>>
>> +enum sync_action md_sync_action(struct mddev *mddev)
>> +{
>> +    unsigned long recovery = mddev->recovery;
>> +
>> +    /*
>> +     * frozen has the highest priority, means running sync_thread 
>> will be
>> +     * stopped immediately, and no new sync_thread can start.
>> +     */
>> +    if (test_bit(MD_RECOVERY_FROZEN, &recovery))
>> +        return ACTION_FROZEN;
>> +
>> +    /*
>> +     * idle means no sync_thread is running, and no new sync_thread is
>> +     * requested.
>> +     */
>> +    if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
>> +        (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, 
>> &recovery)))
>> +        return ACTION_IDLE;
> My brain was lost sometimes looking into nested conditions of md code...
> I agree with Xiao Ni's suggestion that more comments about the array
> state should be added.

Okay, perhaps you're refering md_is_rdwr()? which is supposed to be
related to "no new sync_thread is requestd", perhaps following is
better:

/* only read-write array can start sync_thread */
if (!(md_is_rdwr(mddev))
	return ACTION_IDLE;

/* sync_thread is not running, and no new sync_thread is requested */
if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
     !test_bit(MD_RECOVERY_NEEDED, &recovery)
	return ACTION_IDLE;

> 
>> +    if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
>> +        mddev->reshape_position != MaxSector)
>> +        return ACTION_RESHAPE;
>> +
>> +    if (test_bit(MD_RECOVERY_RECOVER, &recovery))
>> +        return ACTION_RECOVER;
>> +
>>
> In action_show, MD_RECOVERY_SYNC is tested first then MD_RECOVERY_RECOVER.
> After looking through the logic of MD_RECOVERY_RECOVER clear/set_bit, the
> change is fine to me. However, better to follow old pattern unless there
> have resons.

It's just because MD_RECOVERY_SYNC is more complicated, and I move it to
the last, just programming habits. :)
> 
> 
>> +    if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
>> +        if (test_bit(MD_RECOVERY_CHECK, &recovery))
>> +            return ACTION_CHECK;
>> +        if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
>> +            return ACTION_REPAIR;
>> +        return ACTION_RESYNC;
>> +    }
>> +
>> +    return ACTION_IDLE;
>> +}
>> +
>> +enum sync_action md_sync_action_by_name(char *page)
>> +{
>> +    enum sync_action action;
>> +
>> +    for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
>> +        if (cmd_match(page, action_name[action]))
>> +            return action;
>> +    }
>> +
>> +    return NR_SYNC_ACTIONS;
>> +}
>> +
>> +char *md_sync_action_name(enum sync_action action)
>>
> 
> And 'const char *'

Yes

Thanks,
Kuai

> 
> -- 
> Su
> 
>> +{
>> +    return action_name[action];
>> +}
>> +
>>  static ssize_t
>>  action_show(struct mddev *mddev, char *page)
>>  {
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index 2edad966f90a..72ca7a796df5 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct mddev 
>> *mddev, struct md_thread __rcu **t
>>  extern void md_wakeup_thread(struct md_thread __rcu *thread);
>>  extern void md_check_recovery(struct mddev *mddev);
>>  extern void md_reap_sync_thread(struct mddev *mddev);
>> +extern enum sync_action md_sync_action(struct mddev *mddev);
>> +extern enum sync_action md_sync_action_by_name(char *page);
>> +extern char *md_sync_action_name(enum sync_action action);
>>  extern bool md_write_start(struct mddev *mddev, struct bio  *bi);
>>  extern void md_write_inc(struct mddev *mddev, struct bio *bi);
>>  extern void md_write_end(struct mddev *mddev);
> .
>
Xiao Ni May 21, 2024, 3:25 a.m. UTC | #7
On Mon, May 20, 2024 at 8:38 PM Su Yue <l@damenly.org> wrote:
>
>
> On Thu 09 May 2024 at 09:18, Yu Kuai <yukuai1@huaweicloud.com>
> wrote:
>
> > From: Yu Kuai <yukuai3@huawei.com>
> >
> > The new helpers will get current sync_action of the array, will
> > be used
> > in later patches to make code cleaner.
> >
> > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> > ---
> >  drivers/md/md.c | 64
> >  +++++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/md/md.h |  3 +++
> >  2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 00bbafcd27bb..48ec35342d1b 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -69,6 +69,16 @@
> >  #include "md-bitmap.h"
> >  #include "md-cluster.h"
> >
> > +static char *action_name[NR_SYNC_ACTIONS] = {
> >
>
> Th array will not be modified, so:
>
> static const char * const action_names[NR_SYNC_ACTIONS]
>
> > +     [ACTION_RESYNC]         = "resync",
> > +     [ACTION_RECOVER]        = "recover",
> > +     [ACTION_CHECK]          = "check",
> > +     [ACTION_REPAIR]         = "repair",
> > +     [ACTION_RESHAPE]        = "reshape",
> > +     [ACTION_FROZEN]         = "frozen",
> > +     [ACTION_IDLE]           = "idle",
> > +};
> > +
> >  /* pers_list is a list of registered personalities protected by
> >  pers_lock. */
> >  static LIST_HEAD(pers_list);
> >  static DEFINE_SPINLOCK(pers_lock);
> > @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const
> > char *buf, size_t len)
> >  static struct md_sysfs_entry md_metadata =
> >  __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR,
> >  metadata_show, metadata_store);
> >
> > +enum sync_action md_sync_action(struct mddev *mddev)
> > +{
> > +     unsigned long recovery = mddev->recovery;
> > +
> > +     /*
> > +      * frozen has the highest priority, means running sync_thread
> > will be
> > +      * stopped immediately, and no new sync_thread can start.
> > +      */
> > +     if (test_bit(MD_RECOVERY_FROZEN, &recovery))
> > +             return ACTION_FROZEN;
> > +
> > +     /*
> > +      * idle means no sync_thread is running, and no new
> > sync_thread is
> > +      * requested.
> > +      */
> > +     if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
> > +         (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED,
> > &recovery)))
> > +             return ACTION_IDLE;
> My brain was lost sometimes looking into nested conditions of md
> code...

agree+

> I agree with Xiao Ni's suggestion that more comments about the
> array
> state should be added.

In fact, I suggest to keep the logic which is in action_show now. The
logic in action_show is easier to understand for me.

Best Regards
Xiao
>
> > +     if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
> > +         mddev->reshape_position != MaxSector)
> > +             return ACTION_RESHAPE;
> > +
> > +     if (test_bit(MD_RECOVERY_RECOVER, &recovery))
> > +             return ACTION_RECOVER;
> > +
> >
> In action_show, MD_RECOVERY_SYNC is tested first then
> MD_RECOVERY_RECOVER.
> After looking through the logic of MD_RECOVERY_RECOVER
> clear/set_bit, the
> change is fine to me. However, better to follow old pattern unless
> there
> have resons.
>
>
> > +     if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
> > +             if (test_bit(MD_RECOVERY_CHECK, &recovery))
> > +                     return ACTION_CHECK;
> > +             if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
> > +                     return ACTION_REPAIR;
> > +             return ACTION_RESYNC;
> > +     }
> > +
> > +     return ACTION_IDLE;
> > +}
> > +
> > +enum sync_action md_sync_action_by_name(char *page)
> > +{
> > +     enum sync_action action;
> > +
> > +     for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
> > +             if (cmd_match(page, action_name[action]))
> > +                     return action;
> > +     }
> > +
> > +     return NR_SYNC_ACTIONS;
> > +}
> > +
> > +char *md_sync_action_name(enum sync_action action)
> >
>
> And 'const char *'
>
> --
> Su
>
> > +{
> > +     return action_name[action];
> > +}
> > +
> >  static ssize_t
> >  action_show(struct mddev *mddev, char *page)
> >  {
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index 2edad966f90a..72ca7a796df5 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct
> > mddev *mddev, struct md_thread __rcu **t
> >  extern void md_wakeup_thread(struct md_thread __rcu *thread);
> >  extern void md_check_recovery(struct mddev *mddev);
> >  extern void md_reap_sync_thread(struct mddev *mddev);
> > +extern enum sync_action md_sync_action(struct mddev *mddev);
> > +extern enum sync_action md_sync_action_by_name(char *page);
> > +extern char *md_sync_action_name(enum sync_action action);
> >  extern bool md_write_start(struct mddev *mddev, struct bio
> >  *bi);
> >  extern void md_write_inc(struct mddev *mddev, struct bio *bi);
> >  extern void md_write_end(struct mddev *mddev);
>
Su Yue May 21, 2024, 3:50 a.m. UTC | #8
On Tue 21 May 2024 at 11:25, Xiao Ni <xni@redhat.com> wrote:

> On Mon, May 20, 2024 at 8:38 PM Su Yue <l@damenly.org> wrote:
>>
>>
>> On Thu 09 May 2024 at 09:18, Yu Kuai <yukuai1@huaweicloud.com>
>> wrote:
>>
>> > From: Yu Kuai <yukuai3@huawei.com>
>> >
>> > The new helpers will get current sync_action of the array, 
>> > will
>> > be used
>> > in later patches to make code cleaner.
>> >
>> > Signed-off-by: Yu Kuai <yukuai3@huawei.com>
>> > ---
>> >  drivers/md/md.c | 64
>> >  +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  drivers/md/md.h |  3 +++
>> >  2 files changed, 67 insertions(+)
>> >
>> > diff --git a/drivers/md/md.c b/drivers/md/md.c
>> > index 00bbafcd27bb..48ec35342d1b 100644
>> > --- a/drivers/md/md.c
>> > +++ b/drivers/md/md.c
>> > @@ -69,6 +69,16 @@
>> >  #include "md-bitmap.h"
>> >  #include "md-cluster.h"
>> >
>> > +static char *action_name[NR_SYNC_ACTIONS] = {
>> >
>>
>> Th array will not be modified, so:
>>
>> static const char * const action_names[NR_SYNC_ACTIONS]
>>
>> > +     [ACTION_RESYNC]         = "resync",
>> > +     [ACTION_RECOVER]        = "recover",
>> > +     [ACTION_CHECK]          = "check",
>> > +     [ACTION_REPAIR]         = "repair",
>> > +     [ACTION_RESHAPE]        = "reshape",
>> > +     [ACTION_FROZEN]         = "frozen",
>> > +     [ACTION_IDLE]           = "idle",
>> > +};
>> > +
>> >  /* pers_list is a list of registered personalities protected 
>> >  by
>> >  pers_lock. */
>> >  static LIST_HEAD(pers_list);
>> >  static DEFINE_SPINLOCK(pers_lock);
>> > @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, 
>> > const
>> > char *buf, size_t len)
>> >  static struct md_sysfs_entry md_metadata =
>> >  __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR,
>> >  metadata_show, metadata_store);
>> >
>> > +enum sync_action md_sync_action(struct mddev *mddev)
>> > +{
>> > +     unsigned long recovery = mddev->recovery;
>> > +
>> > +     /*
>> > +      * frozen has the highest priority, means running 
>> > sync_thread
>> > will be
>> > +      * stopped immediately, and no new sync_thread can 
>> > start.
>> > +      */
>> > +     if (test_bit(MD_RECOVERY_FROZEN, &recovery))
>> > +             return ACTION_FROZEN;
>> > +
>> > +     /*
>> > +      * idle means no sync_thread is running, and no new
>> > sync_thread is
>> > +      * requested.
>> > +      */
>> > +     if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
>> > +         (!md_is_rdwr(mddev) || 
>> > !test_bit(MD_RECOVERY_NEEDED,
>> > &recovery)))
>> > +             return ACTION_IDLE;
>> My brain was lost sometimes looking into nested conditions of 
>> md
>> code...
>
> agree+
>
>> I agree with Xiao Ni's suggestion that more comments about the
>> array
>> state should be added.
>
> In fact, I suggest to keep the logic which is in action_show 
> now. The
> logic in action_show is easier to understand for me.
>

I'm in the middle, either of new/old logic looks good to me ;)
Thanks for clarifying.

--
Su
> Best Regards
> Xiao
>>
>> > +     if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
>> > +         mddev->reshape_position != MaxSector)
>> > +             return ACTION_RESHAPE;
>> > +
>> > +     if (test_bit(MD_RECOVERY_RECOVER, &recovery))
>> > +             return ACTION_RECOVER;
>> > +
>> >
>> In action_show, MD_RECOVERY_SYNC is tested first then
>> MD_RECOVERY_RECOVER.
>> After looking through the logic of MD_RECOVERY_RECOVER
>> clear/set_bit, the
>> change is fine to me. However, better to follow old pattern 
>> unless
>> there
>> have resons.
>>
>>
>> > +     if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
>> > +             if (test_bit(MD_RECOVERY_CHECK, &recovery))
>> > +                     return ACTION_CHECK;
>> > +             if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
>> > +                     return ACTION_REPAIR;
>> > +             return ACTION_RESYNC;
>> > +     }
>> > +
>> > +     return ACTION_IDLE;
>> > +}
>> > +
>> > +enum sync_action md_sync_action_by_name(char *page)
>> > +{
>> > +     enum sync_action action;
>> > +
>> > +     for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
>> > +             if (cmd_match(page, action_name[action]))
>> > +                     return action;
>> > +     }
>> > +
>> > +     return NR_SYNC_ACTIONS;
>> > +}
>> > +
>> > +char *md_sync_action_name(enum sync_action action)
>> >
>>
>> And 'const char *'
>>
>> --
>> Su
>>
>> > +{
>> > +     return action_name[action];
>> > +}
>> > +
>> >  static ssize_t
>> >  action_show(struct mddev *mddev, char *page)
>> >  {
>> > diff --git a/drivers/md/md.h b/drivers/md/md.h
>> > index 2edad966f90a..72ca7a796df5 100644
>> > --- a/drivers/md/md.h
>> > +++ b/drivers/md/md.h
>> > @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct
>> > mddev *mddev, struct md_thread __rcu **t
>> >  extern void md_wakeup_thread(struct md_thread __rcu 
>> >  *thread);
>> >  extern void md_check_recovery(struct mddev *mddev);
>> >  extern void md_reap_sync_thread(struct mddev *mddev);
>> > +extern enum sync_action md_sync_action(struct mddev *mddev);
>> > +extern enum sync_action md_sync_action_by_name(char *page);
>> > +extern char *md_sync_action_name(enum sync_action action);
>> >  extern bool md_write_start(struct mddev *mddev, struct bio
>> >  *bi);
>> >  extern void md_write_inc(struct mddev *mddev, struct bio 
>> >  *bi);
>> >  extern void md_write_end(struct mddev *mddev);
>>
diff mbox series

Patch

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 00bbafcd27bb..48ec35342d1b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -69,6 +69,16 @@ 
 #include "md-bitmap.h"
 #include "md-cluster.h"
 
+static char *action_name[NR_SYNC_ACTIONS] = {
+	[ACTION_RESYNC]		= "resync",
+	[ACTION_RECOVER]	= "recover",
+	[ACTION_CHECK]		= "check",
+	[ACTION_REPAIR]		= "repair",
+	[ACTION_RESHAPE]	= "reshape",
+	[ACTION_FROZEN]		= "frozen",
+	[ACTION_IDLE]		= "idle",
+};
+
 /* pers_list is a list of registered personalities protected by pers_lock. */
 static LIST_HEAD(pers_list);
 static DEFINE_SPINLOCK(pers_lock);
@@ -4867,6 +4877,60 @@  metadata_store(struct mddev *mddev, const char *buf, size_t len)
 static struct md_sysfs_entry md_metadata =
 __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, metadata_show, metadata_store);
 
+enum sync_action md_sync_action(struct mddev *mddev)
+{
+	unsigned long recovery = mddev->recovery;
+
+	/*
+	 * frozen has the highest priority, means running sync_thread will be
+	 * stopped immediately, and no new sync_thread can start.
+	 */
+	if (test_bit(MD_RECOVERY_FROZEN, &recovery))
+		return ACTION_FROZEN;
+
+	/*
+	 * idle means no sync_thread is running, and no new sync_thread is
+	 * requested.
+	 */
+	if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
+	    (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED, &recovery)))
+		return ACTION_IDLE;
+
+	if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
+	    mddev->reshape_position != MaxSector)
+		return ACTION_RESHAPE;
+
+	if (test_bit(MD_RECOVERY_RECOVER, &recovery))
+		return ACTION_RECOVER;
+
+	if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
+		if (test_bit(MD_RECOVERY_CHECK, &recovery))
+			return ACTION_CHECK;
+		if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
+			return ACTION_REPAIR;
+		return ACTION_RESYNC;
+	}
+
+	return ACTION_IDLE;
+}
+
+enum sync_action md_sync_action_by_name(char *page)
+{
+	enum sync_action action;
+
+	for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
+		if (cmd_match(page, action_name[action]))
+			return action;
+	}
+
+	return NR_SYNC_ACTIONS;
+}
+
+char *md_sync_action_name(enum sync_action action)
+{
+	return action_name[action];
+}
+
 static ssize_t
 action_show(struct mddev *mddev, char *page)
 {
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 2edad966f90a..72ca7a796df5 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -864,6 +864,9 @@  extern void md_unregister_thread(struct mddev *mddev, struct md_thread __rcu **t
 extern void md_wakeup_thread(struct md_thread __rcu *thread);
 extern void md_check_recovery(struct mddev *mddev);
 extern void md_reap_sync_thread(struct mddev *mddev);
+extern enum sync_action md_sync_action(struct mddev *mddev);
+extern enum sync_action md_sync_action_by_name(char *page);
+extern char *md_sync_action_name(enum sync_action action);
 extern bool md_write_start(struct mddev *mddev, struct bio *bi);
 extern void md_write_inc(struct mddev *mddev, struct bio *bi);
 extern void md_write_end(struct mddev *mddev);