From: NeilBrown <neilb@suse.de> To: "Guilherme G. Piccoli" <gpiccoli@canonical.com>, linux-raid@vger.kernel.org Cc: Neil F Brown <nfbrown@suse.com>, Song Liu <songliubraving@fb.com>, liu.song.a23@gmail.com, linux-block@vger.kernel.org, dm-devel@redhat.com, jay.vosburgh@canonical.com Subject: Re: [PATCH v3 2/2] mdadm: Introduce new array state 'broken' for raid0/linear Date: Fri, 30 Aug 2019 18:17:08 +1000 [thread overview] Message-ID: <87a7brf4or.fsf@notabene.neil.brown.name> (raw) In-Reply-To: <20190822161318.26236-2-gpiccoli@canonical.com> [-- Attachment #1.1: Type: text/plain, Size: 6517 bytes --] On Thu, Aug 22 2019, Guilherme G. Piccoli wrote: > Currently if a md raid0/linear array gets one or more members removed while > being mounted, kernel keeps showing state 'clean' in the 'array_state' > sysfs attribute. Despite udev signaling the member device is gone, 'mdadm' > cannot issue the STOP_ARRAY ioctl successfully, given the array is mounted. > > Nothing else hints that something is wrong (except that the removed devices > don't show properly in the output of mdadm 'detail' command). There is no > other property to be checked, and if user is not performing reads/writes > to the array, even kernel log is quiet and doesn't give a clue about the > missing member. > > This patch is the mdadm counterpart of kernel new array state 'broken'. > The 'broken' state mimics the state 'clean' in every aspect, being useful > only to distinguish if an array has some member missing. All necessary > paths in mdadm were changed to deal with 'broken' state, and in case the > tool runs in a kernel that is not updated, it'll work normally, i.e., it > doesn't require the 'broken' state in order to work. > Also, this patch changes the way the array state is showed in the 'detail' > command (for raid0/linear only) - now it takes the 'array_state' sysfs > attribute into account instead of only rely in the MD_SB_CLEAN flag. > > Cc: NeilBrown <neilb@suse.com> > Cc: Song Liu <songliubraving@fb.com> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> > --- > > v2 -> v3: > * Nothing changed. > > v1 -> v2: > * Added handling for md/linear 'broken' state. > > > Detail.c | 17 +++++++++++++++-- > Monitor.c | 9 +++++++-- > maps.c | 1 + > mdadm.h | 1 + > mdmon.h | 2 +- > monitor.c | 4 ++-- > 6 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/Detail.c b/Detail.c > index ad60434..cc7e9f1 100644 > --- a/Detail.c > +++ b/Detail.c > @@ -81,6 +81,7 @@ int Detail(char *dev, struct context *c) > int external; > int inactive; > int is_container = 0; > + char arrayst[12] = { 0 }; /* no state is >10 chars currently */ Why do you have an array? Why not just a "char *". Then all the "strncpy" below become simple pointer assignment. > > if (fd < 0) { > pr_err("cannot open %s: %s\n", > @@ -485,9 +486,21 @@ int Detail(char *dev, struct context *c) > else > st = ", degraded"; > > + if (array.state & (1 << MD_SB_CLEAN)) { > + if ((array.level == 0) || > + (array.level == LEVEL_LINEAR)) > + strncpy(arrayst, > + map_num(sysfs_array_states, > + sra->array_state), > + sizeof(arrayst)-1); > + else > + strncpy(arrayst, "clean", > + sizeof(arrayst)-1); > + } else > + strncpy(arrayst, "active", sizeof(arrayst)-1); > + > printf(" State : %s%s%s%s%s%s \n", > - (array.state & (1 << MD_SB_CLEAN)) ? > - "clean" : "active", st, > + arrayst, st, > (!e || (e->percent < 0 && > e->percent != RESYNC_PENDING && > e->percent != RESYNC_DELAYED)) ? > diff --git a/Monitor.c b/Monitor.c > index 036103f..9fd5406 100644 > --- a/Monitor.c > +++ b/Monitor.c > @@ -1055,8 +1055,12 @@ int Wait(char *dev) > } > } > > +/* The state "broken" is used only for RAID0/LINEAR - it's the same as > + * "clean", but used in case the array has one or more members missing. > + */ > +#define CLEAN_STATES_LAST_POS 5 > static char *clean_states[] = { > - "clear", "inactive", "readonly", "read-auto", "clean", NULL }; > + "clear", "inactive", "readonly", "read-auto", "clean", "broken", NULL }; > > int WaitClean(char *dev, int verbose) > { > @@ -1116,7 +1120,8 @@ int WaitClean(char *dev, int verbose) > rv = read(state_fd, buf, sizeof(buf)); > if (rv < 0) > break; > - if (sysfs_match_word(buf, clean_states) <= 4) Arg. That is horrible. Who wrote that code??? Oh, it was me. And only 8 years ago. sysfs_match_word() should return a clear "didn't match" indicator, like "-1". Ideally that should be fixed, but I cannot really expect you to do that. Maybe make it if (clean_states[sysfs_match_word(buf, clean_states)] != NULL) ?? or if (sysfs_match_word(buf, clean_states) < ARRAY_SIZE(clean_states)-1) Otherwise the patch looks ok. Thanks, NeilBrown > + if (sysfs_match_word(buf, clean_states) > + <= CLEAN_STATES_LAST_POS) > break; > rv = sysfs_wait(state_fd, &delay); > if (rv < 0 && errno != EINTR) > diff --git a/maps.c b/maps.c > index 02a0474..49b7f2c 100644 > --- a/maps.c > +++ b/maps.c > @@ -150,6 +150,7 @@ mapping_t sysfs_array_states[] = { > { "read-auto", ARRAY_READ_AUTO }, > { "clean", ARRAY_CLEAN }, > { "write-pending", ARRAY_WRITE_PENDING }, > + { "broken", ARRAY_BROKEN }, > { NULL, ARRAY_UNKNOWN_STATE } > }; > > diff --git a/mdadm.h b/mdadm.h > index 43b07d5..c88ceab 100644 > --- a/mdadm.h > +++ b/mdadm.h > @@ -373,6 +373,7 @@ struct mdinfo { > ARRAY_ACTIVE, > ARRAY_WRITE_PENDING, > ARRAY_ACTIVE_IDLE, > + ARRAY_BROKEN, > ARRAY_UNKNOWN_STATE, > } array_state; > struct md_bb bb; > diff --git a/mdmon.h b/mdmon.h > index 818367c..b3d72ac 100644 > --- a/mdmon.h > +++ b/mdmon.h > @@ -21,7 +21,7 @@ > extern const char Name[]; > > enum array_state { clear, inactive, suspended, readonly, read_auto, > - clean, active, write_pending, active_idle, bad_word}; > + clean, active, write_pending, active_idle, broken, bad_word}; > > enum sync_action { idle, reshape, resync, recover, check, repair, bad_action }; > > diff --git a/monitor.c b/monitor.c > index 81537ed..e0d3be6 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -26,7 +26,7 @@ > > static char *array_states[] = { > "clear", "inactive", "suspended", "readonly", "read-auto", > - "clean", "active", "write-pending", "active-idle", NULL }; > + "clean", "active", "write-pending", "active-idle", "broken", NULL }; > static char *sync_actions[] = { > "idle", "reshape", "resync", "recover", "check", "repair", NULL > }; > @@ -476,7 +476,7 @@ static int read_and_act(struct active_array *a, fd_set *fds) > a->next_state = clean; > ret |= ARRAY_DIRTY; > } > - if (a->curr_state == clean) { > + if ((a->curr_state == clean) || (a->curr_state == broken)) { > a->container->ss->set_array_state(a, 1); > } > if (a->curr_state == active || > -- > 2.22.0 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] [-- Attachment #2: Type: text/plain, Size: 0 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: NeilBrown <neilb@suse.de> To: "Guilherme G. Piccoli" <gpiccoli@canonical.com>, linux-raid@vger.kernel.org Cc: jay.vosburgh@canonical.com, Song Liu <songliubraving@fb.com>, liu.song.a23@gmail.com, dm-devel@redhat.com, Neil F Brown <nfbrown@suse.com>, linux-block@vger.kernel.org Subject: Re: [PATCH v3 2/2] mdadm: Introduce new array state 'broken' for raid0/linear Date: Fri, 30 Aug 2019 18:17:08 +1000 [thread overview] Message-ID: <87a7brf4or.fsf@notabene.neil.brown.name> (raw) In-Reply-To: <20190822161318.26236-2-gpiccoli@canonical.com> [-- Attachment #1: Type: text/plain, Size: 6517 bytes --] On Thu, Aug 22 2019, Guilherme G. Piccoli wrote: > Currently if a md raid0/linear array gets one or more members removed while > being mounted, kernel keeps showing state 'clean' in the 'array_state' > sysfs attribute. Despite udev signaling the member device is gone, 'mdadm' > cannot issue the STOP_ARRAY ioctl successfully, given the array is mounted. > > Nothing else hints that something is wrong (except that the removed devices > don't show properly in the output of mdadm 'detail' command). There is no > other property to be checked, and if user is not performing reads/writes > to the array, even kernel log is quiet and doesn't give a clue about the > missing member. > > This patch is the mdadm counterpart of kernel new array state 'broken'. > The 'broken' state mimics the state 'clean' in every aspect, being useful > only to distinguish if an array has some member missing. All necessary > paths in mdadm were changed to deal with 'broken' state, and in case the > tool runs in a kernel that is not updated, it'll work normally, i.e., it > doesn't require the 'broken' state in order to work. > Also, this patch changes the way the array state is showed in the 'detail' > command (for raid0/linear only) - now it takes the 'array_state' sysfs > attribute into account instead of only rely in the MD_SB_CLEAN flag. > > Cc: NeilBrown <neilb@suse.com> > Cc: Song Liu <songliubraving@fb.com> > Signed-off-by: Guilherme G. Piccoli <gpiccoli@canonical.com> > --- > > v2 -> v3: > * Nothing changed. > > v1 -> v2: > * Added handling for md/linear 'broken' state. > > > Detail.c | 17 +++++++++++++++-- > Monitor.c | 9 +++++++-- > maps.c | 1 + > mdadm.h | 1 + > mdmon.h | 2 +- > monitor.c | 4 ++-- > 6 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/Detail.c b/Detail.c > index ad60434..cc7e9f1 100644 > --- a/Detail.c > +++ b/Detail.c > @@ -81,6 +81,7 @@ int Detail(char *dev, struct context *c) > int external; > int inactive; > int is_container = 0; > + char arrayst[12] = { 0 }; /* no state is >10 chars currently */ Why do you have an array? Why not just a "char *". Then all the "strncpy" below become simple pointer assignment. > > if (fd < 0) { > pr_err("cannot open %s: %s\n", > @@ -485,9 +486,21 @@ int Detail(char *dev, struct context *c) > else > st = ", degraded"; > > + if (array.state & (1 << MD_SB_CLEAN)) { > + if ((array.level == 0) || > + (array.level == LEVEL_LINEAR)) > + strncpy(arrayst, > + map_num(sysfs_array_states, > + sra->array_state), > + sizeof(arrayst)-1); > + else > + strncpy(arrayst, "clean", > + sizeof(arrayst)-1); > + } else > + strncpy(arrayst, "active", sizeof(arrayst)-1); > + > printf(" State : %s%s%s%s%s%s \n", > - (array.state & (1 << MD_SB_CLEAN)) ? > - "clean" : "active", st, > + arrayst, st, > (!e || (e->percent < 0 && > e->percent != RESYNC_PENDING && > e->percent != RESYNC_DELAYED)) ? > diff --git a/Monitor.c b/Monitor.c > index 036103f..9fd5406 100644 > --- a/Monitor.c > +++ b/Monitor.c > @@ -1055,8 +1055,12 @@ int Wait(char *dev) > } > } > > +/* The state "broken" is used only for RAID0/LINEAR - it's the same as > + * "clean", but used in case the array has one or more members missing. > + */ > +#define CLEAN_STATES_LAST_POS 5 > static char *clean_states[] = { > - "clear", "inactive", "readonly", "read-auto", "clean", NULL }; > + "clear", "inactive", "readonly", "read-auto", "clean", "broken", NULL }; > > int WaitClean(char *dev, int verbose) > { > @@ -1116,7 +1120,8 @@ int WaitClean(char *dev, int verbose) > rv = read(state_fd, buf, sizeof(buf)); > if (rv < 0) > break; > - if (sysfs_match_word(buf, clean_states) <= 4) Arg. That is horrible. Who wrote that code??? Oh, it was me. And only 8 years ago. sysfs_match_word() should return a clear "didn't match" indicator, like "-1". Ideally that should be fixed, but I cannot really expect you to do that. Maybe make it if (clean_states[sysfs_match_word(buf, clean_states)] != NULL) ?? or if (sysfs_match_word(buf, clean_states) < ARRAY_SIZE(clean_states)-1) Otherwise the patch looks ok. Thanks, NeilBrown > + if (sysfs_match_word(buf, clean_states) > + <= CLEAN_STATES_LAST_POS) > break; > rv = sysfs_wait(state_fd, &delay); > if (rv < 0 && errno != EINTR) > diff --git a/maps.c b/maps.c > index 02a0474..49b7f2c 100644 > --- a/maps.c > +++ b/maps.c > @@ -150,6 +150,7 @@ mapping_t sysfs_array_states[] = { > { "read-auto", ARRAY_READ_AUTO }, > { "clean", ARRAY_CLEAN }, > { "write-pending", ARRAY_WRITE_PENDING }, > + { "broken", ARRAY_BROKEN }, > { NULL, ARRAY_UNKNOWN_STATE } > }; > > diff --git a/mdadm.h b/mdadm.h > index 43b07d5..c88ceab 100644 > --- a/mdadm.h > +++ b/mdadm.h > @@ -373,6 +373,7 @@ struct mdinfo { > ARRAY_ACTIVE, > ARRAY_WRITE_PENDING, > ARRAY_ACTIVE_IDLE, > + ARRAY_BROKEN, > ARRAY_UNKNOWN_STATE, > } array_state; > struct md_bb bb; > diff --git a/mdmon.h b/mdmon.h > index 818367c..b3d72ac 100644 > --- a/mdmon.h > +++ b/mdmon.h > @@ -21,7 +21,7 @@ > extern const char Name[]; > > enum array_state { clear, inactive, suspended, readonly, read_auto, > - clean, active, write_pending, active_idle, bad_word}; > + clean, active, write_pending, active_idle, broken, bad_word}; > > enum sync_action { idle, reshape, resync, recover, check, repair, bad_action }; > > diff --git a/monitor.c b/monitor.c > index 81537ed..e0d3be6 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -26,7 +26,7 @@ > > static char *array_states[] = { > "clear", "inactive", "suspended", "readonly", "read-auto", > - "clean", "active", "write-pending", "active-idle", NULL }; > + "clean", "active", "write-pending", "active-idle", "broken", NULL }; > static char *sync_actions[] = { > "idle", "reshape", "resync", "recover", "check", "repair", NULL > }; > @@ -476,7 +476,7 @@ static int read_and_act(struct active_array *a, fd_set *fds) > a->next_state = clean; > ret |= ARRAY_DIRTY; > } > - if (a->curr_state == clean) { > + if ((a->curr_state == clean) || (a->curr_state == broken)) { > a->container->ss->set_array_state(a, 1); > } > if (a->curr_state == active || > -- > 2.22.0 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --]
next prev parent reply other threads:[~2019-08-30 8:17 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-22 16:13 [PATCH v3 1/2] md raid0/linear: Mark array as 'broken' and fail BIOs if a member is gone Guilherme G. Piccoli 2019-08-22 16:13 ` Guilherme G. Piccoli 2019-08-22 16:13 ` [PATCH v3 2/2] mdadm: Introduce new array state 'broken' for raid0/linear Guilherme G. Piccoli 2019-08-22 16:13 ` Guilherme G. Piccoli 2019-08-22 21:56 ` Song Liu 2019-08-22 21:56 ` Song Liu 2019-08-29 12:51 ` Guilherme G. Piccoli 2019-08-29 12:51 ` Guilherme G. Piccoli 2019-08-30 8:17 ` NeilBrown [this message] 2019-08-30 8:17 ` NeilBrown 2019-08-30 12:48 ` Guilherme G. Piccoli 2019-08-30 12:48 ` Guilherme G. Piccoli 2019-08-22 21:55 ` [PATCH v3 1/2] md raid0/linear: Mark array as 'broken' and fail BIOs if a member is gone Song Liu 2019-08-22 21:55 ` Song Liu 2019-08-23 17:48 ` Guilherme G. Piccoli 2019-08-23 17:48 ` Guilherme G. Piccoli 2019-08-23 17:51 ` Song Liu 2019-08-23 17:51 ` Song Liu 2019-08-30 10:47 ` Wols Lists 2019-08-30 10:47 ` Wols Lists 2019-08-30 11:25 ` Guilherme Piccoli 2019-08-30 11:25 ` Guilherme Piccoli 2019-09-03 19:53 ` Guilherme G. Piccoli 2019-09-03 19:53 ` Guilherme G. Piccoli 2019-08-30 8:08 ` NeilBrown 2019-08-30 8:08 ` NeilBrown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87a7brf4or.fsf@notabene.neil.brown.name \ --to=neilb@suse.de \ --cc=dm-devel@redhat.com \ --cc=gpiccoli@canonical.com \ --cc=jay.vosburgh@canonical.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-raid@vger.kernel.org \ --cc=liu.song.a23@gmail.com \ --cc=nfbrown@suse.com \ --cc=songliubraving@fb.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.