The idea of stopping all writes if devices is failed, introduced by
62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs if
a member is gone") seems to be reasonable so use MD_BROKEN for RAID1 and
RAID10. Support in RAID456 is added in next commit.
If userspace (mdadm) forces md to fail the device (Failure state
written via sysfs), then EBUSY is expected if array will become failed.
To achieve that, check for MD_BROKEN and if is set, then return EBUSY to
be complaint with userspace.
For faulty state, handled via ioctl, let the error_handler to decide.
Signed-off-by: Mariusz Tkaczyk <redacted>
---
drivers/md/md.c | 16 ++++++++++------
drivers/md/md.h | 4 ++--
drivers/md/raid1.c | 1 +
drivers/md/raid10.c | 1 +
4 files changed, 14 insertions(+), 8 deletions(-)
@@ -259,8 +259,8 @@ enum mddev_flags {MD_NOT_READY,/* do_md_run() is active, so 'array_state'*mustnotreportthatarrayisreadyyet*/-MD_BROKEN,/* This is used in RAID-0/LINEAR only, to stop-*I/Oincaseanarraymemberisgone/failed.+MD_BROKEN,/* This is used to stop I/O and mark device as+*deadincaseanarraybecomesfailed.*/};
Raid456 module had allowed to achieve failed state, distinct from other
redundant levels. It was fixed by fb73b357fb9 ("raid5: block failing
device if raid will be failed").
This fix introduces a bug, now if raid5 fails during IO, it may result
with a hung task without completion. Faulty flag on the device is
necessary to process all requests and is checked many times, mainly in
anaylze_stripe().
Allow to set faulty flag on drive again and set MD_BROKEN if raid is
failed.
Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
Signed-off-by: Mariusz Tkaczyk <redacted>
---
drivers/md/raid5.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
From: Song Liu <song@kernel.org> Date: 2021-09-24 21:10:12
On Fri, Sep 17, 2021 at 8:35 AM Mariusz Tkaczyk
[off-list ref] wrote:
The idea of stopping all writes if devices is failed, introduced by
62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs if
a member is gone") seems to be reasonable so use MD_BROKEN for RAID1 and
RAID10. Support in RAID456 is added in next commit.
If userspace (mdadm) forces md to fail the device (Failure state
written via sysfs), then EBUSY is expected if array will become failed.
To achieve that, check for MD_BROKEN and if is set, then return EBUSY to
be complaint with userspace.
For faulty state, handled via ioctl, let the error_handler to decide.
Signed-off-by: Mariusz Tkaczyk <redacted>
Thanks for the patch and sorry for the late reply.
@@ -259,8 +259,8 @@ enum mddev_flags {MD_NOT_READY,/* do_md_run() is active, so 'array_state'*mustnotreportthatarrayisreadyyet*/-MD_BROKEN,/* This is used in RAID-0/LINEAR only, to stop-*I/Oincaseanarraymemberisgone/failed.+MD_BROKEN,/* This is used to stop I/O and mark device as+*deadincaseanarraybecomesfailed.*/};
From: Song Liu <song@kernel.org> Date: 2021-09-24 21:15:59
On Fri, Sep 24, 2021 at 2:09 PM Song Liu [off-list ref] wrote:
On Fri, Sep 17, 2021 at 8:35 AM Mariusz Tkaczyk
[off-list ref] wrote:
quoted
The idea of stopping all writes if devices is failed, introduced by
62f7b1989c0 ("md raid0/linear: Mark array as 'broken' and fail BIOs if
a member is gone") seems to be reasonable so use MD_BROKEN for RAID1 and
RAID10. Support in RAID456 is added in next commit.
If userspace (mdadm) forces md to fail the device (Failure state
written via sysfs), then EBUSY is expected if array will become failed.
To achieve that, check for MD_BROKEN and if is set, then return EBUSY to
be complaint with userspace.
For faulty state, handled via ioctl, let the error_handler to decide.
Signed-off-by: Mariusz Tkaczyk <redacted>
Thanks for the patch and sorry for the late reply.
From: Song Liu <song@kernel.org> Date: 2021-09-24 21:18:46
On Fri, Sep 17, 2021 at 8:35 AM Mariusz Tkaczyk
[off-list ref] wrote:
Raid456 module had allowed to achieve failed state, distinct from other
redundant levels. It was fixed by fb73b357fb9 ("raid5: block failing
device if raid will be failed").
This fix introduces a bug, now if raid5 fails during IO, it may result
with a hung task without completion.
It will be great if we can add repro steps and/or hung stack trace here.
Faulty flag on the device is
necessary to process all requests and is checked many times, mainly in
anaylze_stripe().
Allow to set faulty flag on drive again and set MD_BROKEN if raid is
failed.
Fixes: fb73b357fb9 ("raid5: block failing device if raid will be failed")
Signed-off-by: Mariusz Tkaczyk <redacted>
For both patches, please provide more information about what is being
fixed and describe the behavior.
Thanks,
Song
So with MD_BROKEN, we will not try to update the SB?
Array is dead, is there added value in writing failed state?
For external arrays failed state is not written, because drive is
not removed from MD device and metadata manager cannot detect array
failure. This is how it was originally implemented (expect raid5 but I
aligned it around two years ago[1]). I tried to make it consistent for
everyone but I failed. Second patch restores possibility to remove
drive by kernel for raid5 so failed state will be detected (for external)
again.
So, maybe I should drop this change for native. What do you think?
@@ -2979,7 +2980,8 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) int err = -EINVAL; if (cmd_match(buf, "faulty") && rdev->mddev->pers) { md_error(rdev->mddev, rdev);- if (test_bit(Faulty, &rdev->flags))++ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
I changed verification to checking MD_BROKEN flag instead. It is
potentially harmful for raid1 and raid10 as old way is working well but
main target was to unify it across levels. Current verification method
is not flexible enough for raid5 (second patch).
Small benefit is that new IO will be failed faster, in md_submit_io().
Thanks,
Mariusz
[1] https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?id=fb73b3
[2] https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=cb8f5371
So with MD_BROKEN, we will not try to update the SB?
Array is dead, is there added value in writing failed state?
I think there is still value to remember this. Say a raid1 with 2 drives,
A and B. If B is unpluged from the system, we would like to update SB
on A to remember that. Otherwise, if B is pluged back later (maybe after
system reset), we won't tell which one has the latest data.
Does this make sense?
For external arrays failed state is not written, because drive is
not removed from MD device and metadata manager cannot detect array
failure. This is how it was originally implemented (expect raid5 but I
aligned it around two years ago[1]). I tried to make it consistent for
everyone but I failed. Second patch restores possibility to remove
drive by kernel for raid5 so failed state will be detected (for external)
again.
So, maybe I should drop this change for native. What do you think?
@@ -2979,7 +2980,8 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) int err = -EINVAL; if (cmd_match(buf, "faulty") && rdev->mddev->pers) { md_error(rdev->mddev, rdev);- if (test_bit(Faulty, &rdev->flags))++ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
This changed the behavior for raid0, no?
W/o the change mdadm --fail on raid0 will get EBUSY. W/ this change,
it will get 0, and the device is NOT marked as faulty, right?
Song
So with MD_BROKEN, we will not try to update the SB?
Array is dead, is there added value in writing failed state?
I think there is still value to remember this. Say a raid1 with 2 drives,
A and B. If B is unpluged from the system, we would like to update SB
on A to remember that. Otherwise, if B is pluged back later (maybe after
system reset), we won't tell which one has the latest data.
Does this make sense?
Removing one drive from raid1 array doesn't cause raid failure.
So, removing B will be recorded on A.
Raid1 is not good example because to fail array we need to remove
all members, so MD_BROKEN doesn't matter because
!test_bit(Faulty, &rdev->flags) is true.
Let say that we have raid10 with member A, B, C, D. Member A is removed,
and it is recorded correctly (we are degraded now). Next, member B is
removed which causes array failure.
W/ my patch:
member B failure is not saved on members C and D. Raid is failed but
it is not recorded in metadata.
w/o my patch:
member B failure is saved on C and D, and metadata is in failed state.
@@ -2979,7 +2980,8 @@ state_store(struct md_rdev *rdev, const char *buf, size_t len) int err = -EINVAL; if (cmd_match(buf, "faulty") && rdev->mddev->pers) { md_error(rdev->mddev, rdev);- if (test_bit(Faulty, &rdev->flags))++ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
This changed the behavior for raid0, no?
W/o the change mdadm --fail on raid0 will get EBUSY. W/ this change,
it will get 0, and the device is NOT marked as faulty, right?
See commit mentioned in description. MD_BROKEN is used for raid0,
so EBUSY is returned, same as w/o patch.
Thanks,
Mariusz
So with MD_BROKEN, we will not try to update the SB?
Array is dead, is there added value in writing failed state?
I think there is still value to remember this. Say a raid1 with 2 drives,
A and B. If B is unpluged from the system, we would like to update SB
on A to remember that. Otherwise, if B is pluged back later (maybe after
system reset), we won't tell which one has the latest data.
Does this make sense?
Removing one drive from raid1 array doesn't cause raid failure.
So, removing B will be recorded on A.
Raid1 is not good example because to fail array we need to remove
all members, so MD_BROKEN doesn't matter because
!test_bit(Faulty, &rdev->flags) is true.
is false.
Oh, I messed it up. There is no faulty flag in this case, we cannot remove
last drive. Since member is (physically) gone then there is no change to
update metadata, even if it is requested.
Let say that we have raid10 with member A, B, C, D. Member A is removed,
and it is recorded correctly (we are degraded now). Next, member B is
removed which causes array failure.
W/ my patch:
member B failure is not saved on members C and D. Raid is failed but
it is not recorded in metadata.
w/o my patch:
member B failure is saved on C and D, and metadata is in failed state.
size_t len)
int err = -EINVAL;
if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
md_error(rdev->mddev, rdev);
- if (test_bit(Faulty, &rdev->flags))
+
+ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN
here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
This changed the behavior for raid0, no?
W/o the change mdadm --fail on raid0 will get EBUSY. W/ this change,
it will get 0, and the device is NOT marked as faulty, right?
See commit mentioned in description. MD_BROKEN is used for raid0,
so EBUSY is returned, same as w/o patch.
Thanks,
Mariusz
So with MD_BROKEN, we will not try to update the SB?
Array is dead, is there added value in writing failed state?
I think there is still value to remember this. Say a raid1 with 2 drives,
A and B. If B is unpluged from the system, we would like to update SB
on A to remember that. Otherwise, if B is pluged back later (maybe after
system reset), we won't tell which one has the latest data.
Does this make sense?
Removing one drive from raid1 array doesn't cause raid failure.
So, removing B will be recorded on A.
Raid1 is not good example because to fail array we need to remove
all members, so MD_BROKEN doesn't matter because
!test_bit(Faulty, &rdev->flags) is true.
is false.
Oh, I messed it up. There is no faulty flag in this case, we cannot remove
last drive. Since member is (physically) gone then there is no change to
update metadata, even if it is requested.
quoted
Let say that we have raid10 with member A, B, C, D. Member A is removed,
and it is recorded correctly (we are degraded now). Next, member B is
removed which causes array failure.
W/ my patch:
member B failure is not saved on members C and D. Raid is failed but
it is not recorded in metadata.
w/o my patch:
member B failure is saved on C and D, and metadata is in failed state.
size_t len)
int err = -EINVAL;
if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
md_error(rdev->mddev, rdev);
- if (test_bit(Faulty, &rdev->flags))
+
+ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN
here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
This changed the behavior for raid0, no?
W/o the change mdadm --fail on raid0 will get EBUSY. W/ this change,
it will get 0, and the device is NOT marked as faulty, right?
See commit mentioned in description. MD_BROKEN is used for raid0,
so EBUSY is returned, same as w/o patch.
Hmm... I am still confused. In state_store(), md_error is a no-op for raid0,
which will not set Faulty or MD_BROKEN. So we will get -EBUSY w/o
the patch and 0 w/ the patch, no? It is probably not a serious issue though.
Thanks,
Song
size_t len)
int err = -EINVAL;
if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
md_error(rdev->mddev, rdev);
- if (test_bit(Faulty, &rdev->flags))
+
+ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN
here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
This changed the behavior for raid0, no?
W/o the change mdadm --fail on raid0 will get EBUSY. W/ this change,
it will get 0, and the device is NOT marked as faulty, right?
See commit mentioned in description. MD_BROKEN is used for raid0,
so EBUSY is returned, same as w/o patch.
Hmm... I am still confused. In state_store(), md_error is a no-op for raid0,
which will not set Faulty or MD_BROKEN. So we will get -EBUSY w/o
the patch and 0 w/ the patch, no? It is probably not a serious issue though.
Yeah, you are right. There is no error_handler. I missed that.
Now, I reviewed raid0 again.
With my change result won't be clear for raid0, it is correlated with IO.
When drive disappears and there is IO, then it could return -EBUSY,
raid0_make_request() may set MD_BROKEN first.
In there is no IO then 0 will be returned. I need to close this gap.
Thanks for your curiosity.
So, please tell me, are you ok with idea of this patch? I will send
requested details with v2 but I want to know if I choose good way to close
raid5 issue, which is a main problem.
Thanks,
Mariusz
size_t len)
int err = -EINVAL;
if (cmd_match(buf, "faulty") && rdev->mddev->pers) {
md_error(rdev->mddev, rdev);
- if (test_bit(Faulty, &rdev->flags))
+
+ if (!test_bit(MD_BROKEN, &rdev->mddev->flags))
I don't think this makes much sense. EBUSY for already failed array
sounds weird.
Also, shall we also set MD_BROKEN here?
Actually, we just called md_error above, so we don't need to set MD_BROKEN
here.
But we shouldn't return EBUSY in such cases, right?
About EBUSY:
This is how it is implemented in mdadm, we are expecting it in
case of failure. See my fix[2].
I agree that it can be confusing, but this is how it is working.
Do you want to change it across mdadm and md?
This will break compatibility.
About MD_BROKEN:
As you see we are determining failure by checking rdev state, if "Faulty"
in flags after md_error() is not set, then it assumes that array is
failed and EBUSY is returned to userspace.
This changed the behavior for raid0, no?
W/o the change mdadm --fail on raid0 will get EBUSY. W/ this change,
it will get 0, and the device is NOT marked as faulty, right?
See commit mentioned in description. MD_BROKEN is used for raid0,
so EBUSY is returned, same as w/o patch.
Hmm... I am still confused. In state_store(), md_error is a no-op for raid0,
which will not set Faulty or MD_BROKEN. So we will get -EBUSY w/o
the patch and 0 w/ the patch, no? It is probably not a serious issue though.
Yeah, you are right. There is no error_handler. I missed that.
Now, I reviewed raid0 again.
With my change result won't be clear for raid0, it is correlated with IO.
When drive disappears and there is IO, then it could return -EBUSY,
raid0_make_request() may set MD_BROKEN first.
In there is no IO then 0 will be returned. I need to close this gap.
Thanks for your curiosity.
So, please tell me, are you ok with idea of this patch? I will send
requested details with v2 but I want to know if I choose good way to close
raid5 issue, which is a main problem.
I think the idea is good. Maybe we can just add an error_hander for raid0 so
it is more consistent?
Besides that, please also add more explanations about MD_BROKEN in the
next version.
Thanks,
Song