Re: [PATCH 00/34] OLCE for external metadata (devel3.2)

[NeilBrown <neilb@suse.de>]

On Tue, 04 Jan 2011 15:34:38 +0100 Adam Kwolek <adam.kwolek@intel.com> wrote:

> This patch series implements Online Capacity Expansion (OLCE) feature in mdadm devel3.2. for Raid5 and Raid0.
> This is implemented as reshape of whole container (all arrays that belongs to container are reshaped).
> Raid0 reshape is implemented using takeover to raid4 transition.
>
> ---

Thanks.
I have reviewed all of these and applied several, changed other, rejected the
rest as detailed below.

Before doing that I finished the refactoring that I was doing so devel-3.2
compiles again.

My next step with this is to do some testing.

As I say below, feel free to resend anything that you find that you still
need.  Maybe I'll understand it better the second time around, or will be
able to suggest a better way of doing it.

In particular, there is no ->manage_reshape method (I didn't take that patch)
so any attempt to reshape an imsm array will crash.  We need to write a
manage_reshape based on the new child_monitor and using the new
progress_reshape.

BTW, when you split a string e.g.
   "this is a long string"
to
   "this is a"
   " long string"

you do *not* need a '\' at the end of the first line.


Thanks,
NeilBrown


> 
> Adam Kwolek (31):
>       Raid0: detect reshape on array start
Applied, but I'm not sure that it is needed or that set_array_state
does what is expected any more...
>       Raid0: Reload disk list on 'next' raid0 array
Not applied - probably because I am too tired to think clearly
about it.  If you still need this, feel free to resubmit it
and I'll think about it then.
>       Raid0: Run 'next' reshapes without meta update
Not applied.  I don't think the fix is right, but I'm not
100% sure, and it might be fixed by other changes that I have
made.  So feel free to resubmit something like this if it is
still a problem.
>       imsm: FIX: do not repair raid4 arrays
Applied
>       Raid0: execute backward takeover
Applied, but I used a different test which is more generic.
>       Detect level change
Applied
>       Take in mind takeover during disk add
Not applied.  This patch is irrelevant because of an earlier patch
which I didn't apply.
>       imsm: Update raid0 metadata for reshape
Applied, though reshape_super should *both* queue and update and
change the metadata in-place, so I changed it to do that.
>       imsm: Move reshape update processing to function
Applied, though I had to do it by hand - hope I didn't break anything.
However the little extra bit in imsm_reshape_super didn't make sense,
and wasn't documented, so I left it out.
>       Add spares to raid0 in mdadm
Applied with changes.  The Assemble.c part of this was OK.
The super-intel.c was not.  That logic needed to be in Assemble.c
I have put it there (untested).
>       imsm: Update metadata for second array
Applied with a few changes.  I broke the new code out into
a separate function, and then called that function from a
different place - I think a more correct place but you might
like to check.
>       Set array size after adding new disks
Applied, with a few changes.  e.g. 'array_size' can read as 'default'.
And we might want to change array_size when raid_disks decreases as well.
>       imsm: update array size information in metadata
Applied.
>       FIX: Initialize subarray variable in reshape_array
No applied - I fixed this a different way already.
>       imsm: FIX: Division by 0
Applied
>       imsm: Finalize reshape in metadata
Applied
>       Finalize reshape after adding disks to array
Not applied.  This should not be needed as the code near the comment:
		/* Reshape has progressed or completed so we need to
		 * update the array state - and possibly the array size
		 */
should handle this case.  If it doesn't it should be fixed.
>       imsm: FIX: Fill sys_name in container_content()
Not applied.  container_content should not set sys_name as the volume could
easily not have a sys_name yet.  Rather some other code should call
sysfs_init() at an appropriate time.  I have done that.
>       FIX: Use sysfs to change array parameters
Applied
>       FIX: added disks are not used by reshape process /md/
Not applied.  Definitely the wrong fix.  Need more details of the
problem.
>       FIX: change adding disks criteria
Not applied.  I'm not convinced this is the correct fix.
For devices already in the array, disk.state should not be zero.
So if it is, then something else is wrong.   I would need to know more
about that problem you are seeing.
>       FIX: Get array information in reshape_array()
Applied
>       imsm: FIX: support general migration by getinfo_super_imsm_volume
Applied.
>       FIX: Arrays cannot be opened exclusively
Why do you say that arrays cannot be opened exclusively?  You might be right,
but I need mor explanation before I can apply this patch - it "seems" wrong
to me.
Not applied.
>       imsm: FIX: update first array in container only
Applied.  Note that when starting array that is part-way through a reshape
of the first array we will need to alloc space just like we allocate
spare here.
>       FIX: get updated information from metadata
Applied, but with substantial changes.
reshape_super reloads the metadata, not reshape_container, so that
it can know if the array size needs to be changed.
>       imsm: FIX: Do not update anchor directly
Not applied.  I don't think this is correct.  The metadata *should* be
updated directly.
When converting a RAID0 to a RAID5 there is no mdadm running so we should:
  ->reshape_super
  ->sync_super
  start mdmon
  proceed with reshape.

>       imsm: FIX: Perform first metadata update for container operation
Applied.
>       imsm: FIX: display error message
Applied ... and I change the other dprintfs to fprintfs as I think
reshape_super should give an message any time it doesn't succeed.
>       imsm: FIX: display correct information for '-E' option
Applied
>       mdadm: second_map enhancement for imsm_get_map()
Applied.
> 
> Krzysztof Wojcik (3):
>       FIX: Position calculation in mdstat_by_subdev
Applied.
>       FIX: Change size condition in imsm_reshape_is_allowed_on_container
Not applied.  Size of '-1' means 'not change was requested'.  Size of '0'
means 'set to maximum size'.  So the test requiring '-1' seems correct.
>       Manage reshape process in manage_reshape vector.
Not applied.  Seems pointless, and ->manage_reshape is not allowed to say
  "management of process not supported".  If ->reshape_super allows the
  reshape, then ->manage_reshape *must* support it.

> 
> 
>  Grow.c        |  174 ++++++++++++------
>  Manage.c      |   13 -
>  managemon.c   |   12 +
>  mdstat.c      |    4 
>  monitor.c     |   20 ++
>  super-intel.c |  561 ++++++++++++++++++++++++++++++++++++++++++---------------
>  6 files changed, 576 insertions(+), 208 deletions(-)
>