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;
+
+ 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);
--
2.39.2
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);
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);
>
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);
>>
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);
> .
>
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
>
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
>>
>
> .
>
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
> >>
> >
> > .
> >
>
>
在 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
>>>>
>>>
>>> .
>>>
>>
>>
>
> .
>
© 2016 - 2025 Red Hat, Inc.