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

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

[Adam Kwolek]

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 <adam.kwolek@intel.com>
---

 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;

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

[Adam Kwolek]

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 <adam.kwolek@intel.com>
---

 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;

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

[NeilBrown]

On Mon, 31 Jan 2011 09:59:22 +0100 Adam Kwolek <adam.kwolek@intel.com> 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 <adam.kwolek@intel.com>
> ---
> 
>  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;

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

[NeilBrown]

On Mon, 31 Jan 2011 09:59:30 +0100 Adam Kwolek <adam.kwolek@intel.com> 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 <adam.kwolek@intel.com>
> ---
> 
>  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

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

[Kwolek, Adam]

> -----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 <adam.kwolek@intel.com>
> 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 <adam.kwolek@intel.com>
> > ---
> >
> >  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

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

[Kwolek, Adam]

> -----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 <adam.kwolek@intel.com>
> 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 <adam.kwolek@intel.com>
> > ---
> >
> >  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;

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

[Kwolek, Adam]

> -----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
> <adam.kwolek@intel.com>
> > 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 <adam.kwolek@intel.com>
> > > ---
> > >
> > >  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

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

[NeilBrown]

On Mon, 31 Jan 2011 10:10:30 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
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
> > <adam.kwolek@intel.com>
> > > 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 <adam.kwolek@intel.com>
> > > > ---
> > > >
> > > >  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 

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

[NeilBrown]

On Mon, 31 Jan 2011 09:28:54 +0000 "Kwolek, Adam" <adam.kwolek@intel.com>
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 <adam.kwolek@intel.com>
> > 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 <adam.kwolek@intel.com>
> > > ---
> > >
> > >  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