| 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 |
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 >>>> >>> >>> . >>> >> >> > > . >
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);
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 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); >>
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);