[PATCH 0/1] Make failure message on re-add more explcit

[PATCH 0/1] Make failure message on re-add more explcit

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

I have seen this come up on the list a couple of times, and also had
bugs filed over it, since this used to 'work'. Making the printed
error message a little more explicit should hopefully make it clearer
why this is being rejected.

Thoughts?

Cheers,
Jes


Jes Sorensen (1):
  Make error message on failure to --re-add more explicit

 Manage.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

-- 
1.7.4.4

[PATCH 1/1] Make error message on failure to --re-add more explicit

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Manage.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Manage.c b/Manage.c
index d9775de..0f9135b 100644
--- a/Manage.c
+++ b/Manage.c
@@ -861,8 +861,12 @@ int Manage_subdevs(char *devname, int fd,
 						dv->devname, devname);
 					fprintf(stderr, Name ": not performing --add as that would convert %s in to a spare.\n",
 						dv->devname);
-					fprintf(stderr, Name ": To make this a spare, use \"mdadm --zero-superblock %s\" first.\n",	
+					fprintf(stderr, Name ": To make this a spare, use \"mdadm --zero-superblock %s\" first.\n",
 						dv->devname);
+					fprintf(stderr, Name ": Note mdadm used to accept re-adding a drive to a raid1 array,\n");
+					fprintf(stderr, Name ": which was a bug. For raid1 arrays it is not possible to\n");
+					fprintf(stderr, Name ": determine which disk is authoritative, and the admin must\n");
+					fprintf(stderr, Name ": decide whether the drive should be converted into a spare.\n");
 					if (tfd >= 0)
 						close(tfd);
 					return 1;
-- 
1.7.4.4

Re: [PATCH 0/1] Make failure message on re-add more explcit

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Wed, 22 Feb 2012 17:59:59 +0100 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> I have seen this come up on the list a couple of times, and also had
> bugs filed over it, since this used to 'work'. Making the printed
> error message a little more explicit should hopefully make it clearer
> why this is being rejected.
> 
> Thoughts?

While I'm always happy to make the error messages more helpful, I don't think
this one does :-(

The reason for the change was that people seemed to often use "--add" when
what they really wanted was "--re-add".
--add will try --re-add first, but it if that doesn't succeed it would do the
plain add and destroy the metadata.

So I introduced the requirement that if you want to destroy metadata, you
need to do it explicitly (and I know that won't stop people, but hopefully it
will slow them down).

Also, this is not at all specific to raid1 - it applies equally to
raid4/5/6/10.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/1] Make failure message on re-add more explcit

[Jes Sorensen]

On 02/22/12 23:04, NeilBrown wrote:
> On Wed, 22 Feb 2012 17:59:59 +0100 Jes.Sorensen@redhat.com wrote:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Hi,
>>
>> I have seen this come up on the list a couple of times, and also had
>> bugs filed over it, since this used to 'work'. Making the printed
>> error message a little more explicit should hopefully make it clearer
>> why this is being rejected.
>>
>> Thoughts?
> 
> While I'm always happy to make the error messages more helpful, I don't think
> this one does :-(
> 
> The reason for the change was that people seemed to often use "--add" when
> what they really wanted was "--re-add".
> --add will try --re-add first, but it if that doesn't succeed it would do the
> plain add and destroy the metadata.
> 
> So I introduced the requirement that if you want to destroy metadata, you
> need to do it explicitly (and I know that won't stop people, but hopefully it
> will slow them down).
> 
> Also, this is not at all specific to raid1 - it applies equally to
> raid4/5/6/10.

Yeah I realize it is not ideal. The reason I tried to make it more
descriptive is that I have been hit with a couple of bug reports where
users suddenly found that things no longer behave like they used to and
just file a bug against it, because the error message doesn't spell it
out in flashing neon. I was trying to improve the message somehow, but I
am sure my attempt wasn't perfect.

The goal was to try and reduce the number of bug reports over this by
making it more obvious/explicit, so if you have a suggestion for how to
do so in a better way, I am all game.

Cheers,
Jes

Re: [PATCH 0/1] Make failure message on re-add more explcit

[John Robinson]

On 22/02/2012 23:16, Jes Sorensen wrote:
[...]
> The goal was to try and reduce the number of bug reports over this by
> making it more obvious/explicit, so if you have a suggestion for how to
> do so in a better way, I am all game.

How about:

mdadm /dev/md0 --add %s
: %s was already a member of /dev/md0, attempting re-add
: Re-add failed because <reason>
: Not performing add as that would zero the superblock on %s and make it 
a spare
: mdadm --add used to do that automatically but it was potentially dangerous
: If that is what you really want to do, use mdadm --zero-superblock %s 
first.

Re: [PATCH 0/1] Make failure message on re-add more explcit

[Jes Sorensen]

On 02/23/12 02:57, John Robinson wrote:
> On 22/02/2012 23:16, Jes Sorensen wrote:
> [...]
>> The goal was to try and reduce the number of bug reports over this by
>> making it more obvious/explicit, so if you have a suggestion for how to
>> do so in a better way, I am all game.
> 
> How about:
> 
> mdadm /dev/md0 --add %s
> : %s was already a member of /dev/md0, attempting re-add
> : Re-add failed because <reason>
> : Not performing add as that would zero the superblock on %s and make it
> a spare
> : mdadm --add used to do that automatically but it was potentially
> dangerous
> : If that is what you really want to do, use mdadm --zero-superblock %s
> first.
> 

This would get my vote, way better than my messy attempt.

Cheers,
Jes

Re: [PATCH 0/1] Make failure message on re-add more explcit

[J. Ali Harlow]

On Thu, Feb 23, 2012, at 09:34 AM, Jes Sorensen wrote:
> On 02/23/12 02:57, John Robinson wrote:
> > On 22/02/2012 23:16, Jes Sorensen wrote:
> > [...]
> >> The goal was to try and reduce the number of bug reports over this by
> >> making it more obvious/explicit, so if you have a suggestion for how to
> >> do so in a better way, I am all game.
> > 
> > How about:
> > 
> > mdadm /dev/md0 --add %s
> > : %s was already a member of /dev/md0, attempting re-add
> > : Re-add failed because <reason>
> > : Not performing add as that would zero the superblock on %s and make it
> > a spare
> > : mdadm --add used to do that automatically but it was potentially
> > dangerous
> > : If that is what you really want to do, use mdadm --zero-superblock %s
> > first.
> > 
> 
> This would get my vote, way better than my messy attempt.

Would
: If %s doesn't hold any useful data, use "mdadm --zero-superblock %s"
before adding.
be a further refinement?

Ali.

Re: [PATCH 0/1] Make failure message on re-add more explcit

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

On Thu, 23 Feb 2012 08:47:27 +0000 "J. Ali Harlow" <ali@juiblex.co.uk> wrote:

> On Thu, Feb 23, 2012, at 09:34 AM, Jes Sorensen wrote:
> > On 02/23/12 02:57, John Robinson wrote:
> > > On 22/02/2012 23:16, Jes Sorensen wrote:
> > > [...]
> > >> The goal was to try and reduce the number of bug reports over this by
> > >> making it more obvious/explicit, so if you have a suggestion for how to
> > >> do so in a better way, I am all game.
> > > 
> > > How about:
> > > 
> > > mdadm /dev/md0 --add %s
> > > : %s was already a member of /dev/md0, attempting re-add
> > > : Re-add failed because <reason>
> > > : Not performing add as that would zero the superblock on %s and make it
> > > a spare
> > > : mdadm --add used to do that automatically but it was potentially
> > > dangerous
> > > : If that is what you really want to do, use mdadm --zero-superblock %s
> > > first.
> > > 
> > 
> > This would get my vote, way better than my messy attempt.
> 
> Would
> : If %s doesn't hold any useful data, use "mdadm --zero-superblock %s"
> before adding.
> be a further refinement?

Maybe .. though "useful" seems a bit of a .... soft(?) word.  What exactly
does it mean in this context?
I would probably prefer something like "If you are happy to discard the data
on .....".

And in the earlier suggestion with have "<reason>" which would certainly be
good to have, but filling in the details is not straight forward
unfortunately.

I'm happy to take patches .... they don't have to be perfect, just an
improvement on what we currently have is enough.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/1] Make failure message on re-add more explcit

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3869 bytes --]

On 02/22/2012 05:04 PM, NeilBrown wrote:
> On Wed, 22 Feb 2012 17:59:59 +0100 Jes.Sorensen@redhat.com wrote:
> 
>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>
>> Hi,
>>
>> I have seen this come up on the list a couple of times, and also had
>> bugs filed over it, since this used to 'work'. Making the printed
>> error message a little more explicit should hopefully make it clearer
>> why this is being rejected.
>>
>> Thoughts?

My apologies for coming into this late.  However, this change is causing
support isses, so I looked up the old thread so that I could put my
comments into context.

> While I'm always happy to make the error messages more helpful, I don't think
> this one does :-(
> 
> The reason for the change was that people seemed to often use "--add" when
> what they really wanted was "--re-add".

This is not an assumption you can make (that people meant re-add instead
of add when they specifically used add).

> --add will try --re-add first,

Generally speaking this is fine, but there are instances where re-add
will never work (such as a device with no bitmap) and mdadm upgrades all
add attempts to re-add attempts without regard for this fact.

> but it if that doesn't succeed it would do the
> plain add and destroy the metadata.

Yes.  So?  The user actually passed add in this instance.  If the user
passes re-add and it fails, we should not automatically attempt to do an
add.  If the user passes in add, and we attempt a re-add instead and it
works, then great.  But if the user passes in add, we attempt a re-add
and fail, then we can't turn around and not even attempt to add or else
we have essentially just thrown add out the window.  It would no longer
have any meaning at all.  And that is in fact the case now.  Add is dead
all except for superblockless devices, for any devices with a superblock
only re-add does anything useful, and it only works with devices that
have a bitmap.

> So I introduced the requirement that if you want to destroy metadata, you
> need to do it explicitly (and I know that won't stop people, but hopefully it
> will slow them down).

Yes you did.  You totally neutered add in the process.

> Also, this is not at all specific to raid1 - it applies equally to
> raid4/5/6/10.

I have a user issue already opened because of this change, and I have to
say I agree with the user completely.  In their case, they set up
machines that go out to remote locations.  The users at those locations
are not highly skilled technical people.  This admin does not *ever*
want those users to run zero-superblock, he's afraid they will zero the
wrong one.  And he's right.  Before, with the old behavior of add, we at
least had some sanity checks in place: did this device used to belong to
this array or no array at all, is it just out of date, does it have a
higher event counter than the array, etc.  When you used add, mdadm
could at least perform a few of these sanity checks to make sure things
are cool and alert the user if they aren't.  But with a workflow of
zero-superblock/add, there is absolutely no double checks we can perform
for the user, no ability to do any sanity checking at all, because we
don't know what the user will be doing with the device next.

Neil, this was a *huge* step backwards.  I think you let the idea that
an add destroys metadata cause a problem here.  Add doesn't destroy
metadata, it rewrites it.  But in the process it at least has the chance
to sanity check things.  The zero-superblock/add workflow really *does*
destroy metadata, and in a much more real way than the old add behavior.
 I will definitely be reverting this change, and I suggest you do the same.


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [PATCH 0/1] Make failure message on re-add more explcit

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6002 bytes --]

On Thu, 05 Apr 2012 14:59:29 -0400 Doug Ledford <dledford@redhat.com> wrote:

> On 02/22/2012 05:04 PM, NeilBrown wrote:
> > On Wed, 22 Feb 2012 17:59:59 +0100 Jes.Sorensen@redhat.com wrote:
> > 
> >> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> >>
> >> Hi,
> >>
> >> I have seen this come up on the list a couple of times, and also had
> >> bugs filed over it, since this used to 'work'. Making the printed
> >> error message a little more explicit should hopefully make it clearer
> >> why this is being rejected.
> >>
> >> Thoughts?
> 
> My apologies for coming into this late.  However, this change is causing
> support isses, so I looked up the old thread so that I could put my
> comments into context.
> 
> > While I'm always happy to make the error messages more helpful, I don't think
> > this one does :-(
> > 
> > The reason for the change was that people seemed to often use "--add" when
> > what they really wanted was "--re-add".
> 
> This is not an assumption you can make (that people meant re-add instead
> of add when they specifically used add).

I don't assume that they "do" but that they "might" - and I assume this
because observation confirms it.

> 
> > --add will try --re-add first,
> 
> Generally speaking this is fine, but there are instances where re-add
> will never work (such as a device with no bitmap) and mdadm upgrades all
> add attempts to re-add attempts without regard for this fact.

Minor nit: --re-add can work on a device with no bitmap.  If you assemble 4
of the 5 devices in a 5-device RAID5, then re-add the missing device before
any writes, it *should* re-add successfully (I haven't tested lately, but I
think it works).

> 
> > but it if that doesn't succeed it would do the
> > plain add and destroy the metadata.
> 
> Yes.  So?  The user actually passed add in this instance.  If the user
> passes re-add and it fails, we should not automatically attempt to do an
> add.  If the user passes in add, and we attempt a re-add instead and it
> works, then great.  But if the user passes in add, we attempt a re-add
> and fail, then we can't turn around and not even attempt to add or else
> we have essentially just thrown add out the window.  It would no longer
> have any meaning at all.  And that is in fact the case now.  Add is dead
> all except for superblockless devices, for any devices with a superblock
> only re-add does anything useful, and it only works with devices that
> have a bitmap.

While there is some validity in your case you are over-stating it here which
is not a good thing.
--add has not become meaningless (or neutered as you say below).  The only
case where it doesn't work is when we attempted --re-add, and we don't
always do that.   In cases where we don't try --re-add, --add is just as good
as it was before.  There may well be a problem, but it doesn't help to
over-state it.

> 
> > So I introduced the requirement that if you want to destroy metadata, you
> > need to do it explicitly (and I know that won't stop people, but hopefully it
> > will slow them down).
> 
> Yes you did.  You totally neutered add in the process.
> 
> > Also, this is not at all specific to raid1 - it applies equally to
> > raid4/5/6/10.
> 
> I have a user issue already opened because of this change, and I have to
> say I agree with the user completely.  In their case, they set up
> machines that go out to remote locations.  The users at those locations
> are not highly skilled technical people.  This admin does not *ever*
> want those users to run zero-superblock, he's afraid they will zero the
> wrong one.  And he's right.  Before, with the old behavior of add, we at
> least had some sanity checks in place: did this device used to belong to
> this array or no array at all, is it just out of date, does it have a
> higher event counter than the array, etc.  When you used add, mdadm
> could at least perform a few of these sanity checks to make sure things
> are cool and alert the user if they aren't.  But with a workflow of
> zero-superblock/add, there is absolutely no double checks we can perform
> for the user, no ability to do any sanity checking at all, because we
> don't know what the user will be doing with the device next.

Did "--add" perform those sanity checks? or do anything with them?
I don't think so.  It would just do a --re-add if it could and a --add if it
couldn't, and --add would replace the metadata (except the device uuid, but
I don't think anyone cares about that).

--add doesn't do all the same checks that --create does so it really is
(was) a lot like --zero-superblock in its ability to make a mess.
 
> 
> Neil, this was a *huge* step backwards.  I think you let the idea that
> an add destroys metadata cause a problem here.  Add doesn't destroy
> metadata, it rewrites it.  But in the process it at least has the chance
> to sanity check things.  The zero-superblock/add workflow really *does*
> destroy metadata, and in a much more real way than the old add behavior.
>  I will definitely be reverting this change, and I suggest you do the same.

Also a step forwards I believe.


The real problem I was trying to protect against (I think) was when someone
ends up with a failed RAID5 or RAID6 array and they try to --remove failed
devices and  --add them back in.  This cannot work so the devices get marked
as spares which is bad.
So I probably want to restrict the failure to only happen when the array is
failed.  I have a half-memory that I did that but cannot find the code, so
maybe I only thought of doing it.
RAID1 and RAID10 cannot be failed (we never fail the 'last' device) so it is
probably safe to always allow --add on these arrays.

Could you say more about the sanity checks that you think mdadm did or could
or should do on --add.  Maybe I misunderstood, or maybe there are some useful
improvements we can make there.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/1] Make failure message on re-add more explcit

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 11621 bytes --]

On 4/9/2012 7:41 PM, NeilBrown wrote:
> On Thu, 05 Apr 2012 14:59:29 -0400 Doug Ledford <dledford@redhat.com> wrote:
> 
>> On 02/22/2012 05:04 PM, NeilBrown wrote:
>>> On Wed, 22 Feb 2012 17:59:59 +0100 Jes.Sorensen@redhat.com wrote:
>>>
>>>> From: Jes Sorensen <Jes.Sorensen@redhat.com>
>>>>
>>>> Hi,
>>>>
>>>> I have seen this come up on the list a couple of times, and also had
>>>> bugs filed over it, since this used to 'work'. Making the printed
>>>> error message a little more explicit should hopefully make it clearer
>>>> why this is being rejected.
>>>>
>>>> Thoughts?
>>
>> My apologies for coming into this late.  However, this change is causing
>> support isses, so I looked up the old thread so that I could put my
>> comments into context.
>>
>>> While I'm always happy to make the error messages more helpful, I don't think
>>> this one does :-(
>>>
>>> The reason for the change was that people seemed to often use "--add" when
>>> what they really wanted was "--re-add".
>>
>> This is not an assumption you can make (that people meant re-add instead
>> of add when they specifically used add).
> 
> I don't assume that they "do" but that they "might" - and I assume this
> because observation confirms it.

I don't doubt that some do, I do on a regular basis.  But as of this
change, I can no longer specify add in many instances and get a
reasonable result because it assumes re-add, fails, then quits.  So
while the assumption that they "might" is fine and should be a good
reason to try re-add, on failure of re-add the error is a hard error
that stops anything else from happening and so is not treated as a
"might" but as a "must have really meant".

>>> --add will try --re-add first,
>>
>> Generally speaking this is fine, but there are instances where re-add
>> will never work (such as a device with no bitmap) and mdadm upgrades all
>> add attempts to re-add attempts without regard for this fact.
> 
> Minor nit: --re-add can work on a device with no bitmap.  If you assemble 4
> of the 5 devices in a 5-device RAID5, then re-add the missing device before
> any writes, it *should* re-add successfully (I haven't tested lately, but I
> think it works).

That's not really a re-add, that's no different than incremental
assembly where you go to auto read-only and then add the last device
before the first write.  To my knowledge, there is no condition where
the array has a different event counter than the device to be re-added
and where the array does not have a bitmap where a re-add works.

>>
>>> but it if that doesn't succeed it would do the
>>> plain add and destroy the metadata.
>>
>> Yes.  So?  The user actually passed add in this instance.  If the user
>> passes re-add and it fails, we should not automatically attempt to do an
>> add.  If the user passes in add, and we attempt a re-add instead and it
>> works, then great.  But if the user passes in add, we attempt a re-add
>> and fail, then we can't turn around and not even attempt to add or else
>> we have essentially just thrown add out the window.  It would no longer
>> have any meaning at all.  And that is in fact the case now.  Add is dead
>> all except for superblockless devices, for any devices with a superblock
>> only re-add does anything useful, and it only works with devices that
>> have a bitmap.
> 
> While there is some validity in your case you are over-stating it here which
> is not a good thing.
> --add has not become meaningless (or neutered as you say below).  The only
> case where it doesn't work is when we attempted --re-add, and we don't
> always do that.   In cases where we don't try --re-add, --add is just as good
> as it was before.  There may well be a problem, but it doesn't help to
> over-state it.

In all of my use cases it seems to be effected.  If the event counters
don't match and there is no bitmap (which is the common transient
failure followed by re-add case), it is neutered.  For the case like you
describe above, it may still work.

>>> So I introduced the requirement that if you want to destroy metadata, you
>>> need to do it explicitly (and I know that won't stop people, but hopefully it
>>> will slow them down).
>>
>> Yes you did.  You totally neutered add in the process.
>>
>>> Also, this is not at all specific to raid1 - it applies equally to
>>> raid4/5/6/10.
>>
>> I have a user issue already opened because of this change, and I have to
>> say I agree with the user completely.  In their case, they set up
>> machines that go out to remote locations.  The users at those locations
>> are not highly skilled technical people.  This admin does not *ever*
>> want those users to run zero-superblock, he's afraid they will zero the
>> wrong one.  And he's right.  Before, with the old behavior of add, we at
>> least had some sanity checks in place: did this device used to belong to
>> this array or no array at all, is it just out of date, does it have a
>> higher event counter than the array, etc.  When you used add, mdadm
>> could at least perform a few of these sanity checks to make sure things
>> are cool and alert the user if they aren't.  But with a workflow of
>> zero-superblock/add, there is absolutely no double checks we can perform
>> for the user, no ability to do any sanity checking at all, because we
>> don't know what the user will be doing with the device next.
> 
> Did "--add" perform those sanity checks? or do anything with them?

At least one of them, yes.  It would warn you if the device didn't
appear to belong to the array you were adding it to.

> I don't think so.  It would just do a --re-add if it could and a --add if it
> couldn't, and --add would replace the metadata (except the device uuid, but
> I don't think anyone cares about that).

Maybe we need to split our discussion into raid1 (and maybe raid10)
arrays and raid4/5/6 arrays.  It would appear from your earlier comments
and also you later comments that this change was initiated because of
issues with raid4/5/6 arrays where being converted to a spare and then
rebuilding a new slot position onto a disk really does destroy the
current state, where as with a raid1 array this is not the case (and not
with a raid10 depending on layout).

The discussion also appears to hinge on the issue of the event counter.
 A re-add only works under two conditions: the event counters match and
the device can be re-added because it isn't really out of date or there
is a bitmap and we can re-add and re-sync only the out of date parts.
If there is no bitmap, and the event counters don't match, then the
device must be re-synced.  The support issues I'm seeing are this last
case, and they are legitimate.

> --add doesn't do all the same checks that --create does so it really is
> (was) a lot like --zero-superblock in its ability to make a mess.
>  
>>
>> Neil, this was a *huge* step backwards.  I think you let the idea that
>> an add destroys metadata cause a problem here.  Add doesn't destroy
>> metadata, it rewrites it.  But in the process it at least has the chance
>> to sanity check things.  The zero-superblock/add workflow really *does*
>> destroy metadata, and in a much more real way than the old add behavior.
>>  I will definitely be reverting this change, and I suggest you do the same.
> 
> Also a step forwards I believe.
> 
> 
> The real problem I was trying to protect against (I think) was when someone
> ends up with a failed RAID5 or RAID6 array and they try to --remove failed
> devices and  --add them back in.

Fair enough.  This scenario warrants attention as people can totally
hose their data doing this.  But I don't think your solution was
correct.  First, it didn't solve anything.  If a re-add fails, then what
corrective action can a user take to make it succeed?  Nothing according
to the message other than zero-superblock.  The add used to do that for
them, then add the device.  The new work flow makes them zero the
superblock manually and do the same thing.  Has anything changed?  No.
They still have no option.  Since a re-add won't work, there is
*nothing* they can do but fall back to zero superblock and add.  We
haven't presented an alternative.  So, while this makes the fact that
they are blowing away their device's current state and data and totally
rebuilding it obvious, it provides no means to proceed down any other
path, no alternative, and so makes no positive impact what so ever.

I would note here that I suspect that the failure case you are really
concerned about is those times where a device is assembled with too few
drives to continue, or where we degrade to a state that we fail due to a
transient error, and the user tries to fix the problem by failing and
adding a drive.  This is a real and legitimate issue, but it is not
solved by your solution.  If any parity based array gets to the point of
non-functional because of too many failed devices, there there is
currently only one way to save the data: remake the array using
--assume-clean.  Stopping someone from re-adding a device to avoid
blowing away the metadata needed to enable that reconstruction effort
has the right intent in mind, but not the right solution.  You would
need to stop them at the remove/fail stage, not at the add stage, and
need to give them advice on how to try and recover, not advice on how to
make the changes unrecoverable.  So detecting a situation where too many
devices have failed, you could print out something like:

You have too few devices in a running state to start this array.  In a
last ditch effort to restore the array, you could recreate the array
with the devices that last failed added back in so that the array is in
a runnable state again (hoping not to have file corruption) by calling
mdadm with the following command line: (print out what they would need,
we should be able to detect what drives have what even counters and see
what the last removed drives where, what their slots in the array where,
what the necessary options to mdadm to recreate the array parameters
where, and print that out).

>  This cannot work so the devices get marked
> as spares which is bad.
> So I probably want to restrict the failure to only happen when the array is
> failed.  I have a half-memory that I did that but cannot find the code, so
> maybe I only thought of doing it.
> RAID1 and RAID10 cannot be failed (we never fail the 'last' device) so it is
> probably safe to always allow --add on these arrays.
> 
> Could you say more about the sanity checks that you think mdadm did or could
> or should do on --add.  Maybe I misunderstood, or maybe there are some useful
> improvements we can make there.

1) That the device is not part of an array, or that it was previously
part of this array and the array thinks this device is failed.
2) That the event counter is less than that of the array (this isn't
totally foolproof, raid1 and certain layouts of raid10 arrays could fool
this, I posted a possible solution in the bugzilla I have for this issue
at: https://bugzilla.redhat.com/show_bug.cgi?id=808776 however please
ignore the fact I was still a bit annoyed at this issue when I wrote the
bug so my language is not as calm as it should have been, my apologies).



-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford

Infiniband specific RPMs available at
	      http://people.redhat.com/dledford/Infiniband


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 898 bytes --]

Re: [PATCH 0/1] Make failure message on re-add more explcit

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]


Hi Doug.

Thanks for your long reply.

I would like to propose the following patch.

It addresses the issues that I am concerned about, and I don't think it
interferes with the usage patterns that you are concerned about.

Does it seems reasonable to you?

My apologies for the frustration this has cause you and your customers.

NeilBrown

commit 0a999759b54f94fd63ac0ee298a549acef6f7d6f
Author: NeilBrown <neilb@suse.de>
Date:   Wed Apr 18 14:19:49 2012 +1000

    Relax restrictions on when --add is permitted.
    
    The restriction that --add was not allowed on a device which
    looked like a recent member of an array was overly harsh.
    
    The real requirement was to avoid using --add when the array had
    failed, and the device being added might contain necessary
    information which can only be incorporated by stopping and
    re-assembling with --force.
    
    So change the test to reflect the need.
    
    Reported-by: Doug Ledford <dledford@redhat.com>
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/Manage.c b/Manage.c
index 3767f01..95aa270 100644
--- a/Manage.c
+++ b/Manage.c
@@ -448,7 +448,7 @@ int Manage_subdevs(char *devname, int fd,
 		char *dnprintable = dv->devname;
 		char *add_dev = dv->devname;
 		int err;
-		int re_add_failed = 0;
+		int array_failed;
 
 		next = dv->next;
 		jnext = 0;
@@ -851,9 +851,8 @@ int Manage_subdevs(char *devname, int fd,
 								continue;
 							goto abort;
 						}
-					skip_re_add:
-						re_add_failed = 1;
 					}
+				skip_re_add:
 					st->ss->free_super(st);
 				}
 				if (add_dev != dv->devname) {
@@ -875,12 +874,30 @@ int Manage_subdevs(char *devname, int fd,
 						dv->devname, devname);
 					goto abort;
 				}
-				if (re_add_failed) {
-					fprintf(stderr, Name ": %s reports being an active member for %s, but a --re-add fails.\n",
-						dv->devname, devname);
-					fprintf(stderr, Name ": not performing --add as that would convert %s in to a spare.\n",
-						dv->devname);
-					fprintf(stderr, Name ": To make this a spare, use \"mdadm --zero-superblock %s\" first.\n",	
+				if (array.active_disks < array.raid_disks) {
+					char *avail = calloc(array.raid_disks, 1);
+					int d;
+					int found = 0;
+
+					for (d = 0; d < MAX_DISKS && found < array.active_disks; d++) {
+						disc.number = d;
+						if (ioctl(fd, GET_DISK_INFO, &disc))
+							continue;
+						if (disc.major == 0 && disc.minor == 0)
+							continue;
+						if (!(disc.state & (1<<MD_DISK_SYNC)))
+							continue;
+						avail[disc.raid_disk] = 1;
+						found++;
+					}
+					array_failed = !enough(array.level, array.raid_disks, 
+							       array.layout, 1, avail);
+				} else
+					array_failed = 0;
+				if (array_failed) {
+					fprintf(stderr, Name ": %s has failed so using --add cannot work and might destroy\n",
+						devname);
+					fprintf(stderr, Name ": data on %s.  You should stop the array and re-assemble it.\n",
 						dv->devname);
 					if (tfd >= 0)
 						close(tfd);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/1] Make failure message on re-add more explcit

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On 04/18/2012 12:25 AM, NeilBrown wrote:
> 
> Hi Doug.
> 
> Thanks for your long reply.
> 
> I would like to propose the following patch.
> 
> It addresses the issues that I am concerned about, and I don't think it
> interferes with the usage patterns that you are concerned about.
> 
> Does it seems reasonable to you?

It looks reasonable to me, thanks for that!

> My apologies for the frustration this has cause you and your customers.

No worries ;-)

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]