[PATCH 1/2] imsm:FIX: change arrays reshape order

Reshape is started from second array, so it causes imsm incompatibility
and problems during second array start.

Reshape should be started in arrays metadata order.

Signed-off-by: Adam Kwolek
---

super-intel.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index f578057..8484df6 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5848,6 +5848,7 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
int devices_to_reshape = 1;
struct imsm_super *mpb = super->anchor;
int ret_val = 0;
+ unsigned int dev_id;

dprintf("imsm: imsm_process_update() for update_reshape\n");

@@ -5877,11 +5878,17 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
" mpb->num_raid_devs = %i\n", mpb->num_raid_devs);
/* manage changes in volume
*/
- for (id = super->devlist ; id; id = id->next) {
+ for (dev_id = 0; dev_id < mpb->num_raid_devs; dev_id++) {
void **sp = *space_list;
struct imsm_dev *newdev;
struct imsm_map *newmap, *oldmap;

+ for (id = super->devlist ; id; id = id->next) {
+ if (id->index == dev_id)
+ break;
+ }
+ if (id == NULL)
+ break;
if (!sp)
continue;
*space_list = *sp;

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

[PATCH 2/2] imsm: Update metadata for second array

When second array reshape is about to start external metadata should be updated
by mdmon in imsm_set_array_state().
For this purposes imsm_progress_container_reshape() is reused.

Signed-off-by: Adam Kwolek
---

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

diff --git a/super-intel.c b/super-intel.c
index 8484df6..0ab4355 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5249,13 +5249,17 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
super->updates_pending++;
}

- /* finalize online capacity expansion/reshape */
+ /* manage online capacity expansion/reshape */
if ((a->curr_action != reshape) &&
(a->prev_action == reshape)) {
struct mdinfo *mdi;

+ /* finalize online capacity expansion/reshape */
for (mdi = a->info.devs; mdi; mdi = mdi->next)
imsm_set_disk(a, mdi->disk.raid_disk, mdi->curr_state);
+
+ /* check next volume reshape */
+ imsm_progress_container_reshape(super);
}

return consistent;

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

Re: [PATCH 1/2] imsm:FIX: change arrays reshape order

On Mon, 31 Jan 2011 09:59:22 +0100 Adam Kwolek wrote:

> Reshape is started from second array, so it causes imsm incompatibility
> and problems during second array start.
>
> Reshape should be started in arrays metadata order.
>
> Signed-off-by: Adam Kwolek
> ---
>
> super-intel.c | 9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index f578057..8484df6 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5848,6 +5848,7 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> int devices_to_reshape = 1;
> struct imsm_super *mpb = super->anchor;
> int ret_val = 0;
> + unsigned int dev_id;
>
> dprintf("imsm: imsm_process_update() for update_reshape\n");
>
> @@ -5877,11 +5878,17 @@ static int apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> " mpb->num_raid_devs = %i\n", mpb->num_raid_devs);
> /* manage changes in volume
> */
> - for (id = super->devlist ; id; id = id->next) {
> + for (dev_id = 0; dev_id < mpb->num_raid_devs; dev_id++) {
> void **sp = *space_list;
> struct imsm_dev *newdev;
> struct imsm_map *newmap, *oldmap;
>
> + for (id = super->devlist ; id; id = id->next) {
> + if (id->index == dev_id)
> + break;
> + }

Could you replace that loop with a call to get_imsm_dev please?

Otherwise, this look good.

Thanks,
NeilBrown

> + if (id == NULL)
> + break;
> if (!sp)
> continue;
> *space_list = *sp;

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

Re: [PATCH 2/2] imsm: Update metadata for second array

On Mon, 31 Jan 2011 09:59:30 +0100 Adam Kwolek wrote:

> When second array reshape is about to start external metadata should be updated
> by mdmon in imsm_set_array_state().
> For this purposes imsm_progress_container_reshape() is reused.
>
> Signed-off-by: Adam Kwolek
> ---
>
> super-intel.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 8484df6..0ab4355 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5249,13 +5249,17 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
> super->updates_pending++;
> }
>
> - /* finalize online capacity expansion/reshape */
> + /* manage online capacity expansion/reshape */
> if ((a->curr_action != reshape) &&
> (a->prev_action == reshape)) {
> struct mdinfo *mdi;
>
> + /* finalize online capacity expansion/reshape */
> for (mdi = a->info.devs; mdi; mdi = mdi->next)
> imsm_set_disk(a, mdi->disk.raid_disk, mdi->curr_state);
> +
> + /* check next volume reshape */
> + imsm_progress_container_reshape(super);
> }
>
> return consistent;

You still haven't explained why you need this extra call to
imsm_progress_container_reshape.
Does the other call never get reached? or does it do the wrong thing? or is
it called too early? or too late or .....

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

RE: [PATCH 2/2] imsm: Update metadata for second array

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of NeilBrown
> Sent: Monday, January 31, 2011 10:19 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 2/2] imsm: Update metadata for second array
>
> On Mon, 31 Jan 2011 09:59:30 +0100 Adam Kwolek
> wrote:
>
> > When second array reshape is about to start external metadata should
> be updated
> > by mdmon in imsm_set_array_state().
> > For this purposes imsm_progress_container_reshape() is reused.
> >
> > Signed-off-by: Adam Kwolek
> > ---
> >
> > super-intel.c | 6 +++++-
> > 1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 8484df6..0ab4355 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5249,13 +5249,17 @@ static int imsm_set_array_state(struct
> active_array *a, int consistent)
> > super->updates_pending++;
> > }
> >
> > - /* finalize online capacity expansion/reshape */
> > + /* manage online capacity expansion/reshape */
> > if ((a->curr_action != reshape) &&
> > (a->prev_action == reshape)) {
> > struct mdinfo *mdi;
> >
> > + /* finalize online capacity expansion/reshape */
> > for (mdi = a->info.devs; mdi; mdi = mdi->next)
> > imsm_set_disk(a, mdi->disk.raid_disk, mdi-
> >curr_state);
> > +
> > + /* check next volume reshape */
> > + imsm_progress_container_reshape(super);
> > }
> >
> > return consistent;
>
> You still haven't explained why you need this extra call to
> imsm_progress_container_reshape.
> Does the other call never get reached? or does it do the wrong thing?
> or is
> it called too early? or too late or .....
>
> NeilBrown

Call to imsm_progress_container_reshape() placed above (super-intel.c:5186), cannot be used as it is called for currently migrated volume only.
If we finish migration it will be never be called (guard for migration in progress: super-intel.c:5133)

Answer is: It is never called for no migration in progress (not too early and not too late: never).

This a reason I've add second call (after finalizing migration) for next array reshape initiation .

BR
Adam


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

RE: [PATCH 1/2] imsm:FIX: change arrays reshape order

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Monday, January 31, 2011 10:18 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 1/2] imsm:FIX: change arrays reshape order
>
> On Mon, 31 Jan 2011 09:59:22 +0100 Adam Kwolek
> wrote:
>
> > Reshape is started from second array, so it causes imsm
> incompatibility
> > and problems during second array start.
> >
> > Reshape should be started in arrays metadata order.
> >
> > Signed-off-by: Adam Kwolek
> > ---
> >
> > super-intel.c | 9 ++++++++-
> > 1 files changed, 8 insertions(+), 1 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index f578057..8484df6 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5848,6 +5848,7 @@ static int
> apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> > int devices_to_reshape = 1;
> > struct imsm_super *mpb = super->anchor;
> > int ret_val = 0;
> > + unsigned int dev_id;
> >
> > dprintf("imsm: imsm_process_update() for update_reshape\n");
> >
> > @@ -5877,11 +5878,17 @@ static int
> apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> > " mpb->num_raid_devs = %i\n", mpb->num_raid_devs);
> > /* manage changes in volume
> > */
> > - for (id = super->devlist ; id; id = id->next) {
> > + for (dev_id = 0; dev_id < mpb->num_raid_devs; dev_id++) {
> > void **sp = *space_list;
> > struct imsm_dev *newdev;
> > struct imsm_map *newmap, *oldmap;
> >
> > + for (id = super->devlist ; id; id = id->next) {
> > + if (id->index == dev_id)
> > + break;
> > + }
>
> Could you replace that loop with a call to get_imsm_dev please?
>
> Otherwise, this look good.
>
> Thanks,
> NeilBrown

In a while ;)
BR
Adam

>
> > + if (id == NULL)
> > + break;
> > if (!sp)
> > continue;
> > *space_list = *sp;

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

RE: [PATCH 1/2] imsm:FIX: change arrays reshape order

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Kwolek, Adam
> Sent: Monday, January 31, 2011 10:30 AM
> To: NeilBrown
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: RE: [PATCH 1/2] imsm:FIX: change arrays reshape order
>
>
>
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Monday, January 31, 2011 10:18 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 1/2] imsm:FIX: change arrays reshape order
> >
> > On Mon, 31 Jan 2011 09:59:22 +0100 Adam Kwolek
>
> > wrote:
> >
> > > Reshape is started from second array, so it causes imsm
> > incompatibility
> > > and problems during second array start.
> > >
> > > Reshape should be started in arrays metadata order.
> > >
> > > Signed-off-by: Adam Kwolek
> > > ---
> > >
> > > super-intel.c | 9 ++++++++-
> > > 1 files changed, 8 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/super-intel.c b/super-intel.c
> > > index f578057..8484df6 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -5848,6 +5848,7 @@ static int
> > apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> > > int devices_to_reshape = 1;
> > > struct imsm_super *mpb = super->anchor;
> > > int ret_val = 0;
> > > + unsigned int dev_id;
> > >
> > > dprintf("imsm: imsm_process_update() for update_reshape\n");
> > >
> > > @@ -5877,11 +5878,17 @@ static int
> > apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> > > " mpb->num_raid_devs = %i\n", mpb->num_raid_devs);
> > > /* manage changes in volume
> > > */
> > > - for (id = super->devlist ; id; id = id->next) {
> > > + for (dev_id = 0; dev_id < mpb->num_raid_devs; dev_id++) {
> > > void **sp = *space_list;
> > > struct imsm_dev *newdev;
> > > struct imsm_map *newmap, *oldmap;
> > >
> > > + for (id = super->devlist ; id; id = id->next) {
> > > + if (id->index == dev_id)
> > > + break;
> > > + }
> >
> > Could you replace that loop with a call to get_imsm_dev please?
> >
> > Otherwise, this look good.
> >
> > Thanks,
> > NeilBrown
>
> In a while ;)
> BR
> Adam

I think we should leave this patch as I've proposed. I need to work not on imsm_dev but on intel_dev.
intel_dev is required for memory re-linking.
If I use there get_imsm_dev() I have to work on 2 pointers set, and keep them in sync. This can introduce more noise in to code.
Than 'for' loop you want to remove.

Summarizing if I use get_imsm_dev(), I cannot remove this loop (it is required for memory management purposes,
few lines below: id->dev = newdev, I need reference to correct intel_dev).

BR
Adam



> >
> > > + if (id == NULL)
> > > + break;
> > > if (!sp)
> > > continue;
> > > *space_list = *sp;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] imsm:FIX: change arrays reshape order

On Mon, 31 Jan 2011 10:10:30 +0000 "Kwolek, Adam"
wrote:

>
>
> > -----Original Message-----
> > From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> > owner@vger.kernel.org] On Behalf Of Kwolek, Adam
> > Sent: Monday, January 31, 2011 10:30 AM
> > To: NeilBrown
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: RE: [PATCH 1/2] imsm:FIX: change arrays reshape order
> >
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Monday, January 31, 2011 10:18 AM
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > > Neubauer, Wojciech
> > > Subject: Re: [PATCH 1/2] imsm:FIX: change arrays reshape order
> > >
> > > On Mon, 31 Jan 2011 09:59:22 +0100 Adam Kwolek
> >
> > > wrote:
> > >
> > > > Reshape is started from second array, so it causes imsm
> > > incompatibility
> > > > and problems during second array start.
> > > >
> > > > Reshape should be started in arrays metadata order.
> > > >
> > > > Signed-off-by: Adam Kwolek
> > > > ---
> > > >
> > > > super-intel.c | 9 ++++++++-
> > > > 1 files changed, 8 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/super-intel.c b/super-intel.c
> > > > index f578057..8484df6 100644
> > > > --- a/super-intel.c
> > > > +++ b/super-intel.c
> > > > @@ -5848,6 +5848,7 @@ static int
> > > apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> > > > int devices_to_reshape = 1;
> > > > struct imsm_super *mpb = super->anchor;
> > > > int ret_val = 0;
> > > > + unsigned int dev_id;
> > > >
> > > > dprintf("imsm: imsm_process_update() for update_reshape\n");
> > > >
> > > > @@ -5877,11 +5878,17 @@ static int
> > > apply_reshape_container_disks_update(struct imsm_update_reshape *u,
> > > > " mpb->num_raid_devs = %i\n", mpb->num_raid_devs);
> > > > /* manage changes in volume
> > > > */
> > > > - for (id = super->devlist ; id; id = id->next) {
> > > > + for (dev_id = 0; dev_id < mpb->num_raid_devs; dev_id++) {
> > > > void **sp = *space_list;
> > > > struct imsm_dev *newdev;
> > > > struct imsm_map *newmap, *oldmap;
> > > >
> > > > + for (id = super->devlist ; id; id = id->next) {
> > > > + if (id->index == dev_id)
> > > > + break;
> > > > + }
> > >
> > > Could you replace that loop with a call to get_imsm_dev please?
> > >
> > > Otherwise, this look good.
> > >
> > > Thanks,
> > > NeilBrown
> >
> > In a while ;)
> > BR
> > Adam
>
> I think we should leave this patch as I've proposed. I need to work not on imsm_dev but on intel_dev.
> intel_dev is required for memory re-linking.
> If I use there get_imsm_dev() I have to work on 2 pointers set, and keep them in sync. This can introduce more noise in to code.
> Than 'for' loop you want to remove.
>
> Summarizing if I use get_imsm_dev(), I cannot remove this loop (it is required for memory management purposes,
> few lines below: id->dev = newdev, I need reference to correct intel_dev).
>

Fair enough - I've applied the original version (not the v2).

Thanks,
NeilBrown

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

Re: [PATCH 2/2] imsm: Update metadata for second array

On Mon, 31 Jan 2011 09:28:54 +0000 "Kwolek, Adam"
wrote:

>
>
> > -----Original Message-----
> > From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> > owner@vger.kernel.org] On Behalf Of NeilBrown
> > Sent: Monday, January 31, 2011 10:19 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 2/2] imsm: Update metadata for second array
> >
> > On Mon, 31 Jan 2011 09:59:30 +0100 Adam Kwolek
> > wrote:
> >
> > > When second array reshape is about to start external metadata should
> > be updated
> > > by mdmon in imsm_set_array_state().
> > > For this purposes imsm_progress_container_reshape() is reused.
> > >
> > > Signed-off-by: Adam Kwolek
> > > ---
> > >
> > > super-intel.c | 6 +++++-
> > > 1 files changed, 5 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 8484df6..0ab4355 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -5249,13 +5249,17 @@ static int imsm_set_array_state(struct
> > active_array *a, int consistent)
> > > super->updates_pending++;
> > > }
> > >
> > > - /* finalize online capacity expansion/reshape */
> > > + /* manage online capacity expansion/reshape */
> > > if ((a->curr_action != reshape) &&
> > > (a->prev_action == reshape)) {
> > > struct mdinfo *mdi;
> > >
> > > + /* finalize online capacity expansion/reshape */
> > > for (mdi = a->info.devs; mdi; mdi = mdi->next)
> > > imsm_set_disk(a, mdi->disk.raid_disk, mdi-
> > >curr_state);
> > > +
> > > + /* check next volume reshape */
> > > + imsm_progress_container_reshape(super);
> > > }
> > >
> > > return consistent;
> >
> > You still haven't explained why you need this extra call to
> > imsm_progress_container_reshape.
> > Does the other call never get reached? or does it do the wrong thing?
> > or is
> > it called too early? or too late or .....
> >
> > NeilBrown
>
> Call to imsm_progress_container_reshape() placed above (super-intel.c:5186), cannot be used as it is called for currently migrated volume only.
> If we finish migration it will be never be called (guard for migration in progress: super-intel.c:5133)
>
> Answer is: It is never called for no migration in progress (not too early and not too late: never).
>
> This a reason I've add second call (after finalizing migration) for next array reshape initiation .
>

Thanks.
I've applied this for now, but I think something is badly messed up in the
handling of migration state - it is terribly convoluted.
However I don't have time to untangle it just now so I'll just take you patch
and hope it doesn't make anything worse :-)

Thanks,
NeilBrown

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