* RE: [PATCH 0/5] Make OLCE workeable
2011-03-09 0:53 ` [PATCH 0/5] Make OLCE workeable NeilBrown
@ 2011-03-09 7:45 ` Kwolek, Adam
2011-03-09 7:56 ` NeilBrown
2011-03-09 8:13 ` Kwolek, Adam
2011-03-09 8:35 ` Kwolek, Adam
2 siblings, 1 reply; 12+ messages in thread
From: Kwolek, Adam @ 2011-03-09 7:45 UTC (permalink / raw)
To: NeilBrown
Cc: linux-raid@vger.kernel.org, Williams, Dan J, Ciechanowski, Ed,
Neubauer, Wojciech
> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Wednesday, March 09, 2011 1:53 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 0/5] Make OLCE workeable
>
> On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek <adam.kwolek@intel.com>
> wrote:
>
> > This is first part of my comments for your latest changes on devel3-2
> regarding reshape and reshape restart.
> > Unfortunately those changes breaks OLCE functionality. This patch
> series makes OLCE workable (mdadm UT suits 12 and 13 succeed).
>
> That's really strange as the patches all look wrong. I'll do some
> testing
> my self and see what I can come up with.
>
> >
> > Problems:
> > 1. In passing in mdinfo (array.level) raid4 for transition raid0-
> >raid0.
> > This causes problems in takeover
> > a.Raid0 takeover doesn't occur. array.level == reshape.level == 4
> > Resolution: Decision about takeover has to be made based on real
> md level not info->array.level
>
> But takeover doesn't *need* to occur.
> When we have a RAID0 array and we want to make it grow, we have to
> 'takeover'
> to RAID4 first.
> But when assembling an array that is in the middle of a growth we
> assemble
> it as a RAID4 array - so it never was a RAID0 array. It starts out as
> RAID4.
Takeover has to occur during 'normal' reshape (patches doesn't even touch restart problem).
When during normal start mdadm enters reshape_array() metadata is in reshape state already (reshape_active == 1)
This means that in case raid0, raid4 is reported but in md raid0 is still present.
Due to this 'takeover' has to be executed.
>
> > b.Decision about backward takeover cannot be always taken by mdadm.
> mdadm can think that original level is raid4
> > Resolution: use level read from md as orig_level (not from
> metadata)
>
> I see that is a problem. We should record '0' as the 'new_level' and
> get mdadm to make use of that information.
This is true for success path. In error case mdadm exits via 'release' label, where
md is switched to orig_level (not new_level). As I remember for 'succeess' path it works as you describes already.
>
> >
> > 2.On the begin of reshape_array() array structure is read from md (via
> ioctrl). Md knows nothing about raid4 working level, so spares_needed is
> wrongly computed
> > Resolution: use info->array.raid_disks instead array.raid_disks to
> compute spares_needed.
>
> md certainly should know about RAID4 working level because it is
> assembled
> as RAID4.... hopefully some testing will show me what you really mean.
During assembly - yes, but for regular reshape start no.
Regular reshape start:
1. md knows raid0
2. metadata reports raid4 (metadata has reshape_active == 1 already)
Reshape restart
1. md knows raid4
2. metadata reports raid4
>
> > 3.Analyse_changes() delta_parity concept doesn't match information
> currently metadata reports. Result of this is wrongly computed
> reshape.after.data_disks field.
> > Resolution: do not use delta_parity, base on information from
> metadata handle
>
> You cannot just remove delta_parity like that. It is needed when we
> start
> a growth. Maybe it is confusing when we restart a growth - I will look
> in to in.
It looks for me that when metadata handler takes responsibility for reporting
Different raid level and increasing raid disks by 'delta parity' we should not
Count delta parity in different place.
If you think that analyse_change() should support both cases:
1. reshape handler increases raid_disks by delta parity
2. reshape handler doesn't increase raid_disks by delta parity
I'll fix analyse_changes() for this.
>
> > 4.Defects/potential defects
> > a.delta_disks should be checked in sysfs_set_array()
>
> No it shouldn't. If delta_disks is UnSet, then the metadata handler has
> done the wrong thing.
>
> > b.array assembly problem
> > reshape_active flag has to be evaluated. Existing second map doesn't
> means reshape always (rebuild, initialization)
>
> There is no problem here. I agree with your second sentence, but that
> code already does this.
> That first patch in your series makes *no* change to the meaning
> of the code.
> Before the patch it is
>
> if (condition) {
> variable = 1;
> stuff;
> ...
>
> After the patch it is
>
> variable = condition;
> if (variable) {
> stuff;
> ...
>
>
> which has exactly the same effect.
Of course you are right !!!, I don't know, how I've look at this condition yesterday.
BR
Adam
>
> So I haven't applied any of these matches as none of them make any sense
> to
> me - sorry.
>
> NeilBrown
>
>
>
> >
> > I've tried to follow your idea in this series. Generally it seems ok
> (proved by UT). I like putting more array configuration in
> sysfs_set_array().
> >
> > Now I'm going to look at patches at the reshape restart point of view.
> > I'll let you know results soon.
> >
> > BR
> > Adam
> >
> >
> > ---
> >
> > Adam Kwolek (5):
> > FIX: Compute spares_needed basing on metadata info
> > FIX: Remove delta_parity concept
> > FIX: Allow for takeover
> > FIX: Set new raid disks when delta_disks is valid
> > fix: array is reassembled inactive if stopped during resync
> >
> >
> > Grow.c | 22 +++++-----------------
> > super-intel.c | 5 +++--
> > sysfs.c | 4 +++-
> > 3 files changed, 11 insertions(+), 20 deletions(-)
> >
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/5] Make OLCE workeable
2011-03-09 7:45 ` Kwolek, Adam
@ 2011-03-09 7:56 ` NeilBrown
0 siblings, 0 replies; 12+ messages in thread
From: NeilBrown @ 2011-03-09 7:56 UTC (permalink / raw)
To: Kwolek, Adam
Cc: linux-raid@vger.kernel.org, Williams, Dan J, Ciechanowski, Ed,
Neubauer, Wojciech
On Wed, 9 Mar 2011 07:45:07 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
wrote:
> >
> > >
> > > 2.On the begin of reshape_array() array structure is read from md (via
> > ioctrl). Md knows nothing about raid4 working level, so spares_needed is
> > wrongly computed
> > > Resolution: use info->array.raid_disks instead array.raid_disks to
> > compute spares_needed.
> >
> > md certainly should know about RAID4 working level because it is
> > assembled
> > as RAID4.... hopefully some testing will show me what you really mean.
>
>
>
> During assembly - yes, but for regular reshape start no.
> Regular reshape start:
> 1. md knows raid0
> 2. metadata reports raid4 (metadata has reshape_active == 1 already)
>
> Reshape restart
> 1. md knows raid4
> 2. metadata reports raid4
The difference was between a container reshape and a single array reshape.
>
>
> >
> > > 3.Analyse_changes() delta_parity concept doesn't match information
> > currently metadata reports. Result of this is wrongly computed
> > reshape.after.data_disks field.
> > > Resolution: do not use delta_parity, base on information from
> > metadata handle
> >
> > You cannot just remove delta_parity like that. It is needed when we
> > start
> > a growth. Maybe it is confusing when we restart a growth - I will look
> > in to in.
>
> It looks for me that when metadata handler takes responsibility for reporting
> Different raid level and increasing raid disks by 'delta parity' we should not
> Count delta parity in different place.
> If you think that analyse_change() should support both cases:
> 1. reshape handler increases raid_disks by delta parity
> 2. reshape handler doesn't increase raid_disks by delta parity
>
> I'll fix analyse_changes() for this.
>
Before you do, have a look at the changes I just pushed out.
With those, most of the tests pass - maybe all of them, I'm not sure.
Thanks,
NeilBrown
^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 0/5] Make OLCE workeable
2011-03-09 0:53 ` [PATCH 0/5] Make OLCE workeable NeilBrown
2011-03-09 7:45 ` Kwolek, Adam
@ 2011-03-09 8:13 ` Kwolek, Adam
2011-03-09 8:35 ` Kwolek, Adam
2 siblings, 0 replies; 12+ messages in thread
From: Kwolek, Adam @ 2011-03-09 8:13 UTC (permalink / raw)
To: NeilBrown
Cc: linux-raid@vger.kernel.org, Williams, Dan J, Ciechanowski, Ed,
Neubauer, Wojciech
> -----Original Message-----
> From: Kwolek, Adam
> Sent: Wednesday, March 09, 2011 8:45 AM
> To: 'NeilBrown'
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH 0/5] Make OLCE workeable
>
>
>
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Wednesday, March 09, 2011 1:53 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 0/5] Make OLCE workeable
> >
> > On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek <adam.kwolek@intel.com>
> > wrote:
> >
> > > This is first part of my comments for your latest changes on devel3-
> 2
> > regarding reshape and reshape restart.
> > > Unfortunately those changes breaks OLCE functionality. This patch
> > series makes OLCE workable (mdadm UT suits 12 and 13 succeed).
> >
> > That's really strange as the patches all look wrong. I'll do some
> > testing
> > my self and see what I can come up with.
> >
> > >
> > > Problems:
> > > 1. In passing in mdinfo (array.level) raid4 for transition raid0-
> > >raid0.
> > > This causes problems in takeover
> > > a.Raid0 takeover doesn't occur. array.level == reshape.level == 4
> > > Resolution: Decision about takeover has to be made based on
> real
> > md level not info->array.level
> >
> > But takeover doesn't *need* to occur.
> > When we have a RAID0 array and we want to make it grow, we have to
> > 'takeover'
> > to RAID4 first.
> > But when assembling an array that is in the middle of a growth we
> > assemble
> > it as a RAID4 array - so it never was a RAID0 array. It starts out as
> > RAID4.
>
> Takeover has to occur during 'normal' reshape (patches doesn't even
> touch restart problem).
> When during normal start mdadm enters reshape_array() metadata is in
> reshape state already (reshape_active == 1)
> This means that in case raid0, raid4 is reported but in md raid0 is
> still present.
> Due to this 'takeover' has to be executed.
>
> >
> > > b.Decision about backward takeover cannot be always taken by
> mdadm.
> > mdadm can think that original level is raid4
> > > Resolution: use level read from md as orig_level (not from
> > metadata)
> >
> > I see that is a problem. We should record '0' as the 'new_level' and
> > get mdadm to make use of that information.
>
> This is true for success path. In error case mdadm exits via 'release'
> label, where
> md is switched to orig_level (not new_level). As I remember for
> 'succeess' path it works as you describes already.
>
>
> >
> > >
> > > 2.On the begin of reshape_array() array structure is read from md
> (via
> > ioctrl). Md knows nothing about raid4 working level, so spares_needed
> is
> > wrongly computed
> > > Resolution: use info->array.raid_disks instead array.raid_disks to
> > compute spares_needed.
> >
> > md certainly should know about RAID4 working level because it is
> > assembled
> > as RAID4.... hopefully some testing will show me what you really mean.
>
>
>
> During assembly - yes, but for regular reshape start no.
> Regular reshape start:
> 1. md knows raid0
> 2. metadata reports raid4 (metadata has reshape_active == 1 already)
>
> Reshape restart
> 1. md knows raid4
> 2. metadata reports raid4
>
>
> >
> > > 3.Analyse_changes() delta_parity concept doesn't match information
> > currently metadata reports. Result of this is wrongly computed
> > reshape.after.data_disks field.
> > > Resolution: do not use delta_parity, base on information from
> > metadata handle
> >
> > You cannot just remove delta_parity like that. It is needed when we
> > start
> > a growth. Maybe it is confusing when we restart a growth - I will
> look
> > in to in.
>
> It looks for me that when metadata handler takes responsibility for
> reporting
> Different raid level and increasing raid disks by 'delta parity' we
> should not
> Count delta parity in different place.
> If you think that analyse_change() should support both cases:
> 1. reshape handler increases raid_disks by delta parity
> 2. reshape handler doesn't increase raid_disks by delta parity
>
> I'll fix analyse_changes() for this.
I've have a look at this and problem is as follow:
Configuration during reshape start:
1. md: raid0, n disks
2. metadata: raid4, n+1 disks
3. new_level: raid0, delta_disks = 1
analyse_change() detects transition raid4->raid0 so:
1. delta_parity = -1
2. before.data_disks = n
3. after.data_disks = before.data_disks + delta_disks - delta_parity
This gives: after.raid_disks = n + 2
1. What parity disk has to do with data disks?
2. we have transition raid0->raid0, so data disks has to increase by delta_disks only
How to differentiate cases:
a) raid0->raid0
b) raid4->raid0
For both:
Array.level == 4 and new_level == 0
Pass md level to analyse changes() to make difference or add field to mdinfo (orig_level)?
3. If we set delta_parity to '-1' then I think it should be added (we removing it twice -> this means adding)?
Please let me know your opinion.
BR
Adam
>
>
>
>
> >
> > > 4.Defects/potential defects
> > > a.delta_disks should be checked in sysfs_set_array()
> >
> > No it shouldn't. If delta_disks is UnSet, then the metadata handler
> has
> > done the wrong thing.
> >
> > > b.array assembly problem
> > > reshape_active flag has to be evaluated. Existing second map doesn't
> > means reshape always (rebuild, initialization)
> >
> > There is no problem here. I agree with your second sentence, but that
> > code already does this.
> > That first patch in your series makes *no* change to the meaning
> > of the code.
> > Before the patch it is
> >
> > if (condition) {
> > variable = 1;
> > stuff;
> > ...
> >
> > After the patch it is
> >
> > variable = condition;
> > if (variable) {
> > stuff;
> > ...
> >
> >
> > which has exactly the same effect.
>
> Of course you are right !!!, I don't know, how I've look at this
> condition yesterday.
>
> BR
> Adam
>
> >
> > So I haven't applied any of these matches as none of them make any
> sense
> > to
> > me - sorry.
> >
> > NeilBrown
> >
> >
> >
> > >
> > > I've tried to follow your idea in this series. Generally it seems ok
> > (proved by UT). I like putting more array configuration in
> > sysfs_set_array().
> > >
> > > Now I'm going to look at patches at the reshape restart point of
> view.
> > > I'll let you know results soon.
> > >
> > > BR
> > > Adam
> > >
> > >
> > > ---
> > >
> > > Adam Kwolek (5):
> > > FIX: Compute spares_needed basing on metadata info
> > > FIX: Remove delta_parity concept
> > > FIX: Allow for takeover
> > > FIX: Set new raid disks when delta_disks is valid
> > > fix: array is reassembled inactive if stopped during resync
> > >
> > >
> > > Grow.c | 22 +++++-----------------
> > > super-intel.c | 5 +++--
> > > sysfs.c | 4 +++-
> > > 3 files changed, 11 insertions(+), 20 deletions(-)
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH 0/5] Make OLCE workeable
2011-03-09 0:53 ` [PATCH 0/5] Make OLCE workeable NeilBrown
2011-03-09 7:45 ` Kwolek, Adam
2011-03-09 8:13 ` Kwolek, Adam
@ 2011-03-09 8:35 ` Kwolek, Adam
2 siblings, 0 replies; 12+ messages in thread
From: Kwolek, Adam @ 2011-03-09 8:35 UTC (permalink / raw)
To: NeilBrown
Cc: linux-raid@vger.kernel.org, Williams, Dan J, Ciechanowski, Ed,
Neubauer, Wojciech
I've tested current mdadm (devel3-2), expansion works again :).
Thanks
Adam
> -----Original Message-----
> From: Kwolek, Adam
> Sent: Wednesday, March 09, 2011 9:14 AM
> To: 'NeilBrown'
> Cc: 'linux-raid@vger.kernel.org'; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH 0/5] Make OLCE workeable
>
>
>
> > -----Original Message-----
> > From: Kwolek, Adam
> > Sent: Wednesday, March 09, 2011 8:45 AM
> > To: 'NeilBrown'
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: RE: [PATCH 0/5] Make OLCE workeable
> >
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Wednesday, March 09, 2011 1:53 AM
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > > Neubauer, Wojciech
> > > Subject: Re: [PATCH 0/5] Make OLCE workeable
> > >
> > > On Tue, 08 Mar 2011 14:24:29 +0100 Adam Kwolek
> <adam.kwolek@intel.com>
> > > wrote:
> > >
> > > > This is first part of my comments for your latest changes on
> devel3-
> > 2
> > > regarding reshape and reshape restart.
> > > > Unfortunately those changes breaks OLCE functionality. This patch
> > > series makes OLCE workable (mdadm UT suits 12 and 13 succeed).
> > >
> > > That's really strange as the patches all look wrong. I'll do some
> > > testing
> > > my self and see what I can come up with.
> > >
> > > >
> > > > Problems:
> > > > 1. In passing in mdinfo (array.level) raid4 for transition raid0-
> > > >raid0.
> > > > This causes problems in takeover
> > > > a.Raid0 takeover doesn't occur. array.level == reshape.level ==
> 4
> > > > Resolution: Decision about takeover has to be made based on
> > real
> > > md level not info->array.level
> > >
> > > But takeover doesn't *need* to occur.
> > > When we have a RAID0 array and we want to make it grow, we have to
> > > 'takeover'
> > > to RAID4 first.
> > > But when assembling an array that is in the middle of a growth we
> > > assemble
> > > it as a RAID4 array - so it never was a RAID0 array. It starts out
> as
> > > RAID4.
> >
> > Takeover has to occur during 'normal' reshape (patches doesn't even
> > touch restart problem).
> > When during normal start mdadm enters reshape_array() metadata is in
> > reshape state already (reshape_active == 1)
> > This means that in case raid0, raid4 is reported but in md raid0 is
> > still present.
> > Due to this 'takeover' has to be executed.
> >
> > >
> > > > b.Decision about backward takeover cannot be always taken by
> > mdadm.
> > > mdadm can think that original level is raid4
> > > > Resolution: use level read from md as orig_level (not from
> > > metadata)
> > >
> > > I see that is a problem. We should record '0' as the 'new_level'
> and
> > > get mdadm to make use of that information.
> >
> > This is true for success path. In error case mdadm exits via 'release'
> > label, where
> > md is switched to orig_level (not new_level). As I remember for
> > 'succeess' path it works as you describes already.
> >
> >
> > >
> > > >
> > > > 2.On the begin of reshape_array() array structure is read from md
> > (via
> > > ioctrl). Md knows nothing about raid4 working level, so
> spares_needed
> > is
> > > wrongly computed
> > > > Resolution: use info->array.raid_disks instead array.raid_disks
> to
> > > compute spares_needed.
> > >
> > > md certainly should know about RAID4 working level because it is
> > > assembled
> > > as RAID4.... hopefully some testing will show me what you really
> mean.
> >
> >
> >
> > During assembly - yes, but for regular reshape start no.
> > Regular reshape start:
> > 1. md knows raid0
> > 2. metadata reports raid4 (metadata has reshape_active == 1 already)
> >
> > Reshape restart
> > 1. md knows raid4
> > 2. metadata reports raid4
> >
> >
> > >
> > > > 3.Analyse_changes() delta_parity concept doesn't match information
> > > currently metadata reports. Result of this is wrongly computed
> > > reshape.after.data_disks field.
> > > > Resolution: do not use delta_parity, base on information from
> > > metadata handle
> > >
> > > You cannot just remove delta_parity like that. It is needed when we
> > > start
> > > a growth. Maybe it is confusing when we restart a growth - I will
> > look
> > > in to in.
> >
> > It looks for me that when metadata handler takes responsibility for
> > reporting
> > Different raid level and increasing raid disks by 'delta parity' we
> > should not
> > Count delta parity in different place.
> > If you think that analyse_change() should support both cases:
> > 1. reshape handler increases raid_disks by delta parity
> > 2. reshape handler doesn't increase raid_disks by delta parity
> >
> > I'll fix analyse_changes() for this.
>
> I've have a look at this and problem is as follow:
> Configuration during reshape start:
> 1. md: raid0, n disks
> 2. metadata: raid4, n+1 disks
> 3. new_level: raid0, delta_disks = 1
>
> analyse_change() detects transition raid4->raid0 so:
> 1. delta_parity = -1
> 2. before.data_disks = n
> 3. after.data_disks = before.data_disks + delta_disks - delta_parity
>
> This gives: after.raid_disks = n + 2
>
> 1. What parity disk has to do with data disks?
> 2. we have transition raid0->raid0, so data disks has to increase by
> delta_disks only
> How to differentiate cases:
> a) raid0->raid0
> b) raid4->raid0
> For both:
> Array.level == 4 and new_level == 0
> Pass md level to analyse changes() to make difference or add field to
> mdinfo (orig_level)?
>
> 3. If we set delta_parity to '-1' then I think it should be added (we
> removing it twice -> this means adding)?
>
> Please let me know your opinion.
>
> BR
> Adam
>
> >
> >
> >
> >
> > >
> > > > 4.Defects/potential defects
> > > > a.delta_disks should be checked in sysfs_set_array()
> > >
> > > No it shouldn't. If delta_disks is UnSet, then the metadata handler
> > has
> > > done the wrong thing.
> > >
> > > > b.array assembly problem
> > > > reshape_active flag has to be evaluated. Existing second map
> doesn't
> > > means reshape always (rebuild, initialization)
> > >
> > > There is no problem here. I agree with your second sentence, but
> that
> > > code already does this.
> > > That first patch in your series makes *no* change to the meaning
> > > of the code.
> > > Before the patch it is
> > >
> > > if (condition) {
> > > variable = 1;
> > > stuff;
> > > ...
> > >
> > > After the patch it is
> > >
> > > variable = condition;
> > > if (variable) {
> > > stuff;
> > > ...
> > >
> > >
> > > which has exactly the same effect.
> >
> > Of course you are right !!!, I don't know, how I've look at this
> > condition yesterday.
> >
> > BR
> > Adam
> >
> > >
> > > So I haven't applied any of these matches as none of them make any
> > sense
> > > to
> > > me - sorry.
> > >
> > > NeilBrown
> > >
> > >
> > >
> > > >
> > > > I've tried to follow your idea in this series. Generally it seems
> ok
> > > (proved by UT). I like putting more array configuration in
> > > sysfs_set_array().
> > > >
> > > > Now I'm going to look at patches at the reshape restart point of
> > view.
> > > > I'll let you know results soon.
> > > >
> > > > BR
> > > > Adam
> > > >
> > > >
> > > > ---
> > > >
> > > > Adam Kwolek (5):
> > > > FIX: Compute spares_needed basing on metadata info
> > > > FIX: Remove delta_parity concept
> > > > FIX: Allow for takeover
> > > > FIX: Set new raid disks when delta_disks is valid
> > > > fix: array is reassembled inactive if stopped during resync
> > > >
> > > >
> > > > Grow.c | 22 +++++-----------------
> > > > super-intel.c | 5 +++--
> > > > sysfs.c | 4 +++-
> > > > 3 files changed, 11 insertions(+), 20 deletions(-)
> > > >
^ permalink raw reply [flat|nested] 12+ messages in thread