* [PATCH v2] md: also hot remove disk from pers when hot removing
@ 2012-11-16 16:04 Sebastian Riemer
2012-11-17 0:07 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Riemer @ 2012-11-16 16:04 UTC (permalink / raw)
To: linux-raid; +Cc: Sebastian Riemer
If hot adding a disk not as spare but at the same slot, the disk isn't added
correctly in the pers and it is possible to cause a kernel panic/oops due to
the invalid pointer to the old rdev in the pers. So hot remove the disk from
the personality before removing it from the array.
Tested with raid1.
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
---
drivers/md/md.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 9ab768a..1fc545b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5852,6 +5852,7 @@ static int hot_remove_disk(struct mddev * mddev, dev_t dev)
{
char b[BDEVNAME_SIZE];
struct md_rdev *rdev;
+ int err;
rdev = find_rdev(mddev, dev);
if (!rdev)
@@ -5860,6 +5861,12 @@ static int hot_remove_disk(struct mddev * mddev, dev_t dev)
if (rdev->raid_disk >= 0)
goto busy;
+ if (mddev->pers && mddev->pers->hot_remove_disk) {
+ err = mddev->pers->hot_remove_disk(mddev, rdev);
+ if (err)
+ return err;
+ sysfs_unlink_rdev(mddev, rdev);
+ }
kick_rdev_from_array(rdev);
md_update_sb(mddev, 1);
md_new_event(mddev);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] md: also hot remove disk from pers when hot removing
2012-11-16 16:04 [PATCH v2] md: also hot remove disk from pers when hot removing Sebastian Riemer
@ 2012-11-17 0:07 ` Dan Williams
2012-11-19 10:27 ` Sebastian Riemer
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2012-11-17 0:07 UTC (permalink / raw)
To: linux-raid; +Cc: Sebastian Riemer
On Fri, Nov 16, 2012 at 8:04 AM, Sebastian Riemer
<sebastian.riemer@profitbricks.com> wrote:
> If hot adding a disk not as spare but at the same slot, the disk isn't added
> correctly in the pers and it is possible to cause a kernel panic/oops due to
> the invalid pointer to the old rdev in the pers. So hot remove the disk from
> the personality before removing it from the array.
>
> Tested with raid1.
>
> Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
> ---
> drivers/md/md.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 9ab768a..1fc545b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5852,6 +5852,7 @@ static int hot_remove_disk(struct mddev * mddev, dev_t dev)
> {
> char b[BDEVNAME_SIZE];
> struct md_rdev *rdev;
> + int err;
>
> rdev = find_rdev(mddev, dev);
> if (!rdev)
> @@ -5860,6 +5861,12 @@ static int hot_remove_disk(struct mddev * mddev, dev_t dev)
> if (rdev->raid_disk >= 0)
> goto busy;
>
> + if (mddev->pers && mddev->pers->hot_remove_disk) {
> + err = mddev->pers->hot_remove_disk(mddev, rdev);
> + if (err)
> + return err;
> + sysfs_unlink_rdev(mddev, rdev);
> + }
Hmm, how are you getting ->raid_disk set to -1 without the personality
being notified? That seems to be the source of the bug. Otherwise it
would seem the state_store("remove") path is also susceptible.
--
Dan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] md: also hot remove disk from pers when hot removing
2012-11-17 0:07 ` Dan Williams
@ 2012-11-19 10:27 ` Sebastian Riemer
2012-11-19 16:08 ` Sebastian Riemer
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Riemer @ 2012-11-19 10:27 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-raid
On 17.11.2012 01:07, Dan Williams wrote:
> Hmm, how are you getting ->raid_disk set to -1 without the personality
> being notified? That seems to be the source of the bug. Otherwise it
> would seem the state_store("remove") path is also susceptible.
>
> --
> Dan
Thanks for your feedback!
Perhaps it's only something for my use-case of read-only
raid1-replicated volumes where I never have any spares. I don't set
->raid_disk to -1 when hot adding them. I have to put them directly to
their stored slot because I assemble read-only on read-only rdevs. So
the syncer never runs. Those read-only rdevs are coming from remote
storage. Imagine HA CD-ROM images.
My thought was that it would be cleaner behavior if the cleanup of an
rdev in the personality already happens when hot removing the disk.
You're right, the state_store("remove") path would need this change, too.
With the "slot_store" path I could trigger a panic when I hot added the
disk as spare, marked it insync and tried to give it the slot it belongs
to. I'll test this with the latest vanilla kernel and try to reproduce
this on rw volumes which I set to read-only. If I can also crash it that
way, I've got a proof that there is a bug.
If the disk isn't cleaned up in the pers, then hot_add_disk doesn't
return any error for raid1. Instead it crashes when referencing the old
pointer when calling "print_conf" in raid1.c.
Cheers,
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] md: also hot remove disk from pers when hot removing
2012-11-19 10:27 ` Sebastian Riemer
@ 2012-11-19 16:08 ` Sebastian Riemer
2012-11-19 21:19 ` NeilBrown
0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Riemer @ 2012-11-19 16:08 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-raid
On 19.11.2012 11:27, Sebastian Riemer wrote:
> On 17.11.2012 01:07, Dan Williams wrote:
>> Hmm, how are you getting ->raid_disk set to -1 without the personality
>> being notified? That seems to be the source of the bug. Otherwise it
>> would seem the state_store("remove") path is also susceptible.
>>
>> --
>> Dan
> Thanks for your feedback!
>
> Perhaps it's only something for my use-case of read-only
> raid1-replicated volumes where I never have any spares. I don't set
> ->raid_disk to -1 when hot adding them. I have to put them directly to
> their stored slot because I assemble read-only on read-only rdevs. So
> the syncer never runs. Those read-only rdevs are coming from remote
> storage. Imagine HA CD-ROM images.
>
> My thought was that it would be cleaner behavior if the cleanup of an
> rdev in the personality already happens when hot removing the disk.
>
> You're right, the state_store("remove") path would need this change, too.
>
> With the "slot_store" path I could trigger a panic when I hot added the
> disk as spare, marked it insync and tried to give it the slot it belongs
> to. I'll test this with the latest vanilla kernel and try to reproduce
> this on rw volumes which I set to read-only. If I can also crash it that
> way, I've got a proof that there is a bug.
The kernel blocks most ioctls on a read-only mddev. So I guess I've got
another custom patch. IMHO it would be still cleaner to also hot remove
the disk from the personality when hot removing an rdev.
> If the disk isn't cleaned up in the pers, then hot_add_disk doesn't
> return any error for raid1. Instead it crashes when referencing the old
> pointer when calling "print_conf" in raid1.c.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] md: also hot remove disk from pers when hot removing
2012-11-19 16:08 ` Sebastian Riemer
@ 2012-11-19 21:19 ` NeilBrown
2012-11-20 9:52 ` Sebastian Riemer
0 siblings, 1 reply; 6+ messages in thread
From: NeilBrown @ 2012-11-19 21:19 UTC (permalink / raw)
To: Sebastian Riemer; +Cc: Dan Williams, linux-raid
[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]
On Mon, 19 Nov 2012 17:08:13 +0100 Sebastian Riemer
<sebastian.riemer@profitbricks.com> wrote:
> On 19.11.2012 11:27, Sebastian Riemer wrote:
> > On 17.11.2012 01:07, Dan Williams wrote:
> >> Hmm, how are you getting ->raid_disk set to -1 without the personality
> >> being notified? That seems to be the source of the bug. Otherwise it
> >> would seem the state_store("remove") path is also susceptible.
> >>
> >> --
> >> Dan
> > Thanks for your feedback!
> >
> > Perhaps it's only something for my use-case of read-only
> > raid1-replicated volumes where I never have any spares. I don't set
> > ->raid_disk to -1 when hot adding them. I have to put them directly to
> > their stored slot because I assemble read-only on read-only rdevs. So
> > the syncer never runs. Those read-only rdevs are coming from remote
> > storage. Imagine HA CD-ROM images.
> >
> > My thought was that it would be cleaner behavior if the cleanup of an
> > rdev in the personality already happens when hot removing the disk.
> >
> > You're right, the state_store("remove") path would need this change, too.
> >
> > With the "slot_store" path I could trigger a panic when I hot added the
> > disk as spare, marked it insync and tried to give it the slot it belongs
> > to. I'll test this with the latest vanilla kernel and try to reproduce
> > this on rw volumes which I set to read-only. If I can also crash it that
> > way, I've got a proof that there is a bug.
>
> The kernel blocks most ioctls on a read-only mddev. So I guess I've got
> another custom patch. IMHO it would be still cleaner to also hot remove
> the disk from the personality when hot removing an rdev.
But you *cannot* hot-remove an rdev when it is owned by the personality.
The "remove from the personality" operation is called "--fail".
The "remove from array" operation is call "--remove".
It would be wrong to conflate these two.
Thanks for confirming it was a local problem - I won't go hunting.
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] md: also hot remove disk from pers when hot removing
2012-11-19 21:19 ` NeilBrown
@ 2012-11-20 9:52 ` Sebastian Riemer
0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Riemer @ 2012-11-20 9:52 UTC (permalink / raw)
To: NeilBrown; +Cc: Dan Williams, linux-raid
On 19.11.2012 22:19, NeilBrown wrote:
> On Mon, 19 Nov 2012 17:08:13 +0100 Sebastian Riemer
> <sebastian.riemer@profitbricks.com> wrote:
>
>> On 19.11.2012 11:27, Sebastian Riemer wrote:
>>> On 17.11.2012 01:07, Dan Williams wrote:
>>>> Hmm, how are you getting ->raid_disk set to -1 without the personality
>>>> being notified? That seems to be the source of the bug. Otherwise it
>>>> would seem the state_store("remove") path is also susceptible.
>>>>
>>>> --
>>>> Dan
>>> Thanks for your feedback!
>>>
>>> Perhaps it's only something for my use-case of read-only
>>> raid1-replicated volumes where I never have any spares. I don't set
>>> ->raid_disk to -1 when hot adding them. I have to put them directly to
>>> their stored slot because I assemble read-only on read-only rdevs. So
>>> the syncer never runs. Those read-only rdevs are coming from remote
>>> storage. Imagine HA CD-ROM images.
>>>
>>> My thought was that it would be cleaner behavior if the cleanup of an
>>> rdev in the personality already happens when hot removing the disk.
>>>
>>> You're right, the state_store("remove") path would need this change,
too.
>>>
>>> With the "slot_store" path I could trigger a panic when I hot added the
>>> disk as spare, marked it insync and tried to give it the slot it belongs
>>> to. I'll test this with the latest vanilla kernel and try to reproduce
>>> this on rw volumes which I set to read-only. If I can also crash it that
>>> way, I've got a proof that there is a bug.
>>
>> The kernel blocks most ioctls on a read-only mddev. So I guess I've got
>> another custom patch. IMHO it would be still cleaner to also hot remove
>> the disk from the personality when hot removing an rdev.
>
> But you *cannot* hot-remove an rdev when it is owned by the personality.
> The "remove from the personality" operation is called "--fail".
> The "remove from array" operation is call "--remove".
>
> It would be wrong to conflate these two.
>
> Thanks for confirming it was a local problem - I won't go hunting.
>
> NeilBrown
>
Thanks for the hint!
Ahhh, O.K., found it: md_check_recovery() and remove_and_add_spares()
handle the hot remove from the personality. I have to convince them to
handle my special read-only rdevs as well. The error handler of the
personality just sets the "Faulty" flag and the raid1d() calls
md_check_recovery() afterwards.
Cheers,
Sebastian
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-20 9:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-16 16:04 [PATCH v2] md: also hot remove disk from pers when hot removing Sebastian Riemer
2012-11-17 0:07 ` Dan Williams
2012-11-19 10:27 ` Sebastian Riemer
2012-11-19 16:08 ` Sebastian Riemer
2012-11-19 21:19 ` NeilBrown
2012-11-20 9:52 ` Sebastian Riemer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).