RE: [PATCH 0/5] Make OLCE workeable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: NeilBrown [mailto:neilb@xxxxxxx]
> Sent: Wednesday, March 09, 2011 1:53 AM
> To: Kwolek, Adam
> Cc: linux-raid@xxxxxxxxxxxxxxx; 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@xxxxxxxxx>
> 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(-)
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux RAID Wiki]     [ATA RAID]     [Linux SCSI Target Infrastructure]     [Linux Block]     [Linux IDE]     [Linux SCSI]     [Linux Hams]     [Device Mapper]     [Device Mapper Cryptographics]     [Kernel]     [Linux Admin]     [Linux Net]     [GFS]     [RPM]     [git]     [Yosemite Forum]


  Powered by Linux