[PATCH 0/9] Grow_continue() - single array.

The following series implements reshape restart from checkpoint.
It supports single array reshape.
Container operations are not fully supported (some code preparation is introduced only /fork()/).

As workaround for container operation multiple container assembly can be used.
During each assembly one array will be reshaped. mdmon switches next array metadata in to reshape state.
This information will be used during next container assembly process.

This series contains some reviewed patches that you not accept last time asn few new.

1. Calling start_reshape() I've changed, but passed flag calculation has to be a little different
that I've pointed in the emails (external and restart flags cannot be used together).
2. I'm resending patches for fork() and backup file check in Grow_continue().
This is begin of support for container operations. It is somehow similar to reshape_container() idea,
but adapted to assembly code.
Assembled array processing is forked. This will allow later (during detected container operation)
to wait when mdmon set this particular array for reshape.
3. I've added fixes for imsm:
a.Checkpoint calculation is wrong (per disk instead per array units).
It was visible during restart from checkpoint. Md reports it
b.Array during reshape was marked as 'dirty'.
This makes mdadm to start reshape from 0 on restart
4. Code for setting 'new' geometry is currently very simple. It sets new disks number only.
I've got a little problem where to place it. You pointed sysfs_set_array() as place to do this.
Inside sysfs_set_array() is placed a comment to set those parameters outside this function.
I've decided to follow suggestion from comment as they seams reasonable.
This code is narrowed to expansion case,
but I've put 'ToDo' information in to code comments, how/where it should be changed (I hope shortly).

I think code follows direction you give me in last email (except above exclusions).

BR
Adam



---

Adam Kwolek (9):
imsm: FIX: Variables declaration cleanup
FIX: Block array monitoring when assembling reshaped array
FIX: Set 'new' geometry when assembling reshaped array
FIX: Verify Backup file name before reshape
FIX: Continue reshape in the background
imsm: FIX: After checkpoint mark array have to be clean
imsm: FIX: Return blocks_per unit for general migration
FIX: Array during reshape cannot be configured
FIX: Do not configure and start, already started reshape


Assemble.c | 26 +++++++++++++++++++++++++-
Grow.c | 47 +++++++++++++++++++++++++++++++++++++++++------
super-intel.c | 7 +++++--
3 files changed, 71 insertions(+), 9 deletions(-)

--
Signature
--
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 1/9] FIX: Do not configure and start, already started reshape

When Grow_continue() calls reshape_array(), reshape is configured
and it is started with array. We cannot start reshape again,
when reshape is in progress.
/it in fact waits due to array_state == 'readonly'/

Signed-off-by: Adam Kwolek
---

Grow.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Grow.c b/Grow.c
index e321a39..d25b71d 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1641,6 +1641,7 @@ static int reshape_array(char *container, int fd, char *devname,
unsigned long long array_size;
int done;
struct mdinfo *sra = NULL;
+ int already_running;

msg = analyse_change(info, &reshape);
if (msg) {
@@ -1995,10 +1996,15 @@ started:
}
}

- err = start_reshape(sra, (info->reshape_active && !st->ss->external));
+ already_running = info->reshape_active &&
+ restart ? restart : !st->ss->external;
+ dprintf("Reshape will be %s\n",
+ already_running ? "continued" : "started");
+ err = start_reshape(sra, already_running);
if (err) {
- fprintf(stderr, Name ": Cannot start reshape for %s\n",
- devname);
+ fprintf(stderr, Name ": Cannot %s reshape for %s\n",
+ already_running ? "continue" : "start",
+ devname);
goto release;
}
if (restart)

--
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/9] FIX: Array during reshape cannot be configured

If this is reshape restart and array sysfs entry sync_action
is equal to "reshape" already.
This means that any configuration changes will return error (busy).
We cannot configure array here. It has to be configured earlier,
during assembly.

Signed-off-by: Adam Kwolek
---

Grow.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Grow.c b/Grow.c
index d25b71d..ee75352 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1966,7 +1966,7 @@ started:
* metadata, and for kernels before 2.6.38 we can
* fail if we try.
*/
- } else {
+ } else if (!restart) {
/* set them all just in case some old 'new_*' value
* persists from some earlier problem.
* We even set them when restarting in the middle. They will

--
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 3/9] imsm: FIX: Return blocks_per unit for general migration

For general migration, blocks per unit are required for all disks,
not for per-member.

Signed-off-by: Adam Kwolek
---

super-intel.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 1a7be71..11972f3 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1690,6 +1690,8 @@ static __u64 blocks_per_migr_unit(struct imsm_dev *dev)
migr_chunk = migr_strip_blocks_resync(dev);
disks = imsm_num_data_members(dev, 0);
blocks_per_unit = stripes_per_unit * migr_chunk * disks;
+ if (migr_type(dev) == MIGR_GEN_MIGR)
+ return blocks_per_unit;
stripe = __le32_to_cpu(map->blocks_per_strip) * disks;
segment = blocks_per_unit / stripe;
block_rel = blocks_per_unit - segment * stripe;

--
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 4/9] imsm: FIX: After checkpoint mark array have to be clean

When checkpoint is marked set volume as clean.
Reshape on dirty volume cannot be restarted from checkpoint.

Signed-off-by: Adam Kwolek
---

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

diff --git a/super-intel.c b/super-intel.c
index 11972f3..04e32ae 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5218,6 +5218,7 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
__le32_to_cpu(dev->vol.curr_migr_unit)) {
dev->vol.curr_migr_unit =
__cpu_to_le32(unit);
+ dev->vol.dirty = 0;
super->updates_pending++;
}
}

--
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 5/9] FIX: Continue reshape in the background

For external metadada, reshape will be continued in the background.
Reshape_array() makes fork itself, but for container operation support
we need a place to wait for reshape begin when current array reshape
time comes /reshapes has to be run in sequence/.
In such case Grow_continue() is responsible for unfreezing array
after return from reshape_array() function.

Signed-off-by: Adam Kwolek
---

Assemble.c | 4 ++++
Grow.c | 26 ++++++++++++++++++++++++--
2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index ee5fcec..e31462d 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -703,6 +703,10 @@ int Assemble(struct supertype *st, char *mddev,
/* check if reshape of external metadata
* is in progress
* and it is need to be monitored by mdadm
+ * ToDo:
+ * For container operation this simple check:
+ * if (content->reshape_active)
+ * has to be replaced by container operation check
*/
if (content->reshape_active)
err = Grow_continue(mdfd, st, content,
diff --git a/Grow.c b/Grow.c
index ee75352..5176425 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3332,6 +3332,7 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
char buf[40];
char *container = NULL;
int err;
+ int forked = 0;

if (!st->ss->external) {
err = sysfs_set_str(info, NULL, "array_state", "readonly");
@@ -3340,9 +3341,30 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
} else {
fmt_devname(buf, st->container_dev);
container = buf;
+ switch (fork()) {
+ case -1:
+ fprintf(stderr, Name ": Cannot run child to "
+ "monitor reshape: %s\n", strerror(errno));
+ return 1;
+ default:
+ return 0;
+ case 0:
+ dprintf(Name ": Continue bacground reshape "
+ "after assemblation\n");
+ forked = 1;
+ /* ToDo:
+ * Wait here during container operation,
+ * if this array has to be reshaped now
+ */
+ }
}
- return reshape_array(container, mdfd, "array", st, info, 1,
- backup_file, 0, 0, 1);
+
+ err = reshape_array(container, mdfd, "array", st, info, 1,
+ backup_file, 0, forked, 1);
+ if (forked)
+ unblock_subarray(info, 0);
+
+ return err;
}



--
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 6/9] FIX: Verify Backup file name before reshape

When reshape is continued in the background, backup_file is required
for user data safety. reshape_array() makes backup_file verification,
but for container operation it will be done in forked code, so check
result will be unknown for assemble.
Grow_continue() has to verify if any file name is passed in.

Signed-off-by: Adam Kwolek
---

Grow.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Grow.c b/Grow.c
index 5176425..a1e6091 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3339,8 +3339,15 @@ int Grow_continue(int mdfd, struct supertype *st, struct mdinfo *info,
if (err)
return err;
} else {
+ if (backup_file == NULL) {
+ fprintf(stderr, Name ": Backup file name has to be "
+ "specified for reshape continuation.\n");
+ return 1;
+ }
+
fmt_devname(buf, st->container_dev);
container = buf;
+
switch (fork()) {
case -1:
fprintf(stderr, Name ": Cannot run child to "

--
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 7/9] FIX: Set "new" geometry when assembling reshaped array

For proper array configuration the following steps has to be taken:
1. set 'old' geometry and reshape position (done in sysfs_set_array())
2. set 'new' geometry (currently new raid_disks only)
3. add disks
4. start array readonly

Added code addressed p.2

In the one of last email, Neil points that sysfs_set_array()
should do this. Comment in sysfs_set_array() doesn't allow for this
due to required analysis for 'new' geometry parameters.
Currently external metadata supports raid disks increase only,
so for now proposed in patch code should enough.
when more migrations for external metadata will be supported this code
will have to support other cases also.

Signed-off-by: Adam Kwolek
---

Assemble.c | 18 ++++++++++++++++--
1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index e31462d..e65022c 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -1527,10 +1527,24 @@ int assemble_container_content(struct supertype *st, int mdfd,
sysfs_init(content, mdfd, 0);

sra = sysfs_read(mdfd, 0, GET_VERSION);
- if (sra == NULL || strcmp(sra->text_version, content->text_version) != 0)
+ if (sra == NULL || strcmp(sra->text_version,
+ content->text_version) != 0) {
if (sysfs_set_array(content, md_get_version(mdfd)) != 0)
return 1;
-
+ if (content->reshape_active) {
+ /* set 'new' array geometry parameters
+ * to restart reshape
+ * ToDo:
+ * Temporary this is a "simple way".
+ * analyse_change() has to be used
+ * for array setup during reshape,
+ * when more array migrations will be supported
+ */
+ sysfs_set_num(content, NULL, "raid_disks",
+ content->array.raid_disks +
+ content->delta_disks);
+ }
+ }
if (sra)
sysfs_free(sra);


--
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 8/9] FIX: Block array monitoring when assembling reshaped array

To allow for array reconfiguration, mdmon cannot push array in to active
state. Assemble should block monitor for external metadata to allow
for reshape configuration and restart.

Signed-off-by: Adam Kwolek
---

Assemble.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index e65022c..8b4b037 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -1532,6 +1532,11 @@ int assemble_container_content(struct supertype *st, int mdfd,
if (sysfs_set_array(content, md_get_version(mdfd)) != 0)
return 1;
if (content->reshape_active) {
+
+ /* mdmon can activate array, so array has to be blocked
+ */
+ block_subarray(content);
+
/* set 'new' array geometry parameters
* to restart reshape
* ToDo:
@@ -1545,6 +1550,7 @@ int assemble_container_content(struct supertype *st, int mdfd,
content->delta_disks);
}
}
+
if (sra)
sysfs_free(sra);


--
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 9/9] imsm: FIX: Variables declaration cleanup

Variables declaration moved a little bit up,
to not mix declaration and code.

Signed-off-by: Adam Kwolek
---

super-intel.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 04e32ae..c5e459f 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -1803,13 +1803,13 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
case MIGR_GEN_MIGR: {
__u64 blocks_per_unit = blocks_per_migr_unit(dev);
__u64 units = __le32_to_cpu(dev->vol.curr_migr_unit);
+ unsigned long long array_blocks;
+ int used_disks;

info->reshape_progress = blocks_per_unit * units;
dprintf("IMSM: General Migration checkpoint : %llu "
"(%llu) -> read reshape progress : %llu\n",
units, blocks_per_unit, info->reshape_progress);
- unsigned long long array_blocks;
- int used_disks;

used_disks = imsm_num_data_members(dev, 1);
if (used_disks > 0) {

--
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 4/9] imsm: FIX: After checkpoint mark array have to beclean

On Wed, 02 Mar 2011 14:29:27 +0100 Adam Kwolek wrote:

> When checkpoint is marked set volume as clean.
> Reshape on dirty volume cannot be restarted from checkpoint.
>
> Signed-off-by: Adam Kwolek
> ---
>
> super-intel.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 11972f3..04e32ae 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -5218,6 +5218,7 @@ static int imsm_set_array_state(struct active_array *a, int consistent)
> __le32_to_cpu(dev->vol.curr_migr_unit)) {
> dev->vol.curr_migr_unit =
> __cpu_to_le32(unit);
> + dev->vol.dirty = 0;
> super->updates_pending++;
> }
> }
>

hi Adam,
You'll need to explain this one a bit more.

If the array isn't clean, then it is wrong to mark it as clean.
If it is clean, then 'consistent' should be 'true' and it will
be marked clean anyway.

Why cannot a reshape of a dirty volume be restarted from a checkpoint?
I would think it would continue with the reshape and then when that
finished, go back and do the resync.

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 0/9] Grow_continue() - single array.

Hi Adam,
I've reviewed this set of patches, applied a few of them, and
made some patches of my own that should achieve the result that I think you
were aiming for. I haven't tested them so there might still be some
issues...

Two important points.

1/ I now require getinfo_super and container_content to present a
reshape_active array in a form that md can cope with it. So a
RAID0 array that is in the middle of a reshape must appear to be
a RAID4 or RAID5 array.
I have changed super-intel to do this, but I have probably missed
some cases - please check that all supported migrations that cannot
be handled directly are handled by the new code.

With this in place, I have changed sysfs_set_array to set up all
the geometry of the array, both old and new. I think this makes
a lot of things a lot cleaner.

2/ I've revised how to handle the restart of a container-wide
migration.
As there is only one array in such a container that is actually
migrating, the mdadm which assembles it can be the one that forks
and managed the whole container.
This now happens. The metadata informs metadata that a container-wide
reshape is needed by setting ->reshape_active to '2'.


I have added a 'freeze' call where I think it should go but I wouldn't be at
all surprised if I got it wrong.

Please review the patch - which are all in my devel-3.2 branch
http://neil.brown.name/git?p=mdadm;a=log;h=refs/heads/devel- 3.2

and let me know what you think.

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 4/9] imsm: FIX: After checkpoint mark array have to beclean

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, March 08, 2011 6:08 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 4/9] imsm: FIX: After checkpoint mark array have to
> be clean
>
> On Wed, 02 Mar 2011 14:29:27 +0100 Adam Kwolek
> wrote:
>
> > When checkpoint is marked set volume as clean.
> > Reshape on dirty volume cannot be restarted from checkpoint.
> >
> > Signed-off-by: Adam Kwolek
> > ---
> >
> > super-intel.c | 1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/super-intel.c b/super-intel.c
> > index 11972f3..04e32ae 100644
> > --- a/super-intel.c
> > +++ b/super-intel.c
> > @@ -5218,6 +5218,7 @@ static int imsm_set_array_state(struct
> active_array *a, int consistent)
> > __le32_to_cpu(dev->vol.curr_migr_unit)) {
> > dev->vol.curr_migr_unit =
> > __cpu_to_le32(unit);
> > + dev->vol.dirty = 0;
> > super->updates_pending++;
> > }
> > }
> >
>
> hi Adam,
> You'll need to explain this one a bit more.
>
> If the array isn't clean, then it is wrong to mark it as clean.
> If it is clean, then 'consistent' should be 'true' and it will
> be marked clean anyway.
>

This could be true if set_array_state has possibility to execute this code in general migration case.
During reshape after updtate curr_migr_unit set array state exits (super-intel.c: 5225 /return 0/)

To achieve this it can be changed:
1. Consistent flag should be == 1 during reshape
monitor.c:387
- a->container->ss->set_array_state(a, a->curr_state <= clean);

+ a->container->ss->set_array_state(a, a->curr_state <= active);

Array state is active during reshape

2. use consistent flag during reshape for imsm
Super-intel.c: set_array_state()
- remove all code for a->curr_action == reshape (around line 5211)
- instead removed code add 'goto' to calculating blocks_per_unit at the end of this function
This will allow for mark clear/dirty volume also

I'll send 2 patch to make this description more clear.
1. FIX: During reshape array is in active state
2. imsm: FIX: Mark checkpoint and array state clean during reshape


> Why cannot a reshape of a dirty volume be restarted from a checkpoint?
> I would think it would continue with the reshape and then when that
> finished, go back and do the resync.
>
> NeilBrown

If array is dirty and during reshape is not assembled due to i.e:
super-intel.c:1792 for dirty volume info->resync_start = 0; Code for analyzing migration type is not executed.

If you find my 2 patches I mention above ok, for now second case it not urgent.


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 0/9] Grow_continue() - single array.

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, March 08, 2011 7:43 AM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 0/9] Grow_continue() - single array.
>
>
> Hi Adam,
> I've reviewed this set of patches, applied a few of them, and
> made some patches of my own that should achieve the result that I think
> you
> were aiming for. I haven't tested them so there might still be some
> issues...
>
> Two important points.
>
> 1/ I now require getinfo_super and container_content to present a
> reshape_active array in a form that md can cope with it. So a
> RAID0 array that is in the middle of a reshape must appear to be
> a RAID4 or RAID5 array.
> I have changed super-intel to do this, but I have probably missed
> some cases - please check that all supported migrations that cannot
> be handled directly are handled by the new code.
>
> With this in place, I have changed sysfs_set_array to set up all
> the geometry of the array, both old and new. I think this makes
> a lot of things a lot cleaner.
>
> 2/ I've revised how to handle the restart of a container-wide
> migration.
> As there is only one array in such a container that is actually
> migrating, the mdadm which assembles it can be the one that forks
> and managed the whole container.
> This now happens. The metadata informs metadata that a container-wide
> reshape is needed by setting ->reshape_active to '2'.
>
>
> I have added a 'freeze' call where I think it should go but I wouldn't
> be at
> all surprised if I got it wrong.
>
> Please review the patch - which are all in my devel-3.2 branch
> http://neil.brown.name/git?p=mdadm;a=log;h=refs/heads/devel- 3.2
>
> and let me know what you think.
>
> Thanks,
> NeilBrown

Hi Neil,

Thank you for your review.

I've found some problems in patches you sent, so I've decided to send my comments in 2 parts.
1st. Changes/workarounds needed to make OLCE workable
(some workarounds, as there are 2 possible ways of changes).
2nd. Changes for reshape restart that I think are requires
(when I'll send first part I'll go with this)

More I'll describe in covers or/and patch descriptions.

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 4/9] imsm: FIX: After checkpoint mark array have to beclean

On Tue, 8 Mar 2011 08:52:40 +0000 "Kwolek, Adam"
wrote:

>
>
> > -----Original Message-----
> > From: NeilBrown [mailto:neilb@suse.de]
> > Sent: Tuesday, March 08, 2011 6:08 AM
> > To: Kwolek, Adam
> > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > Neubauer, Wojciech
> > Subject: Re: [PATCH 4/9] imsm: FIX: After checkpoint mark array have to
> > be clean
> >
> > On Wed, 02 Mar 2011 14:29:27 +0100 Adam Kwolek
> > wrote:
> >
> > > When checkpoint is marked set volume as clean.
> > > Reshape on dirty volume cannot be restarted from checkpoint.
> > >
> > > Signed-off-by: Adam Kwolek
> > > ---
> > >
> > > super-intel.c | 1 +
> > > 1 files changed, 1 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/super-intel.c b/super-intel.c
> > > index 11972f3..04e32ae 100644
> > > --- a/super-intel.c
> > > +++ b/super-intel.c
> > > @@ -5218,6 +5218,7 @@ static int imsm_set_array_state(struct
> > active_array *a, int consistent)
> > > __le32_to_cpu(dev->vol.curr_migr_unit)) {
> > > dev->vol.curr_migr_unit =
> > > __cpu_to_le32(unit);
> > > + dev->vol.dirty = 0;
> > > super->updates_pending++;
> > > }
> > > }
> > >
> >
> > hi Adam,
> > You'll need to explain this one a bit more.
> >
> > If the array isn't clean, then it is wrong to mark it as clean.
> > If it is clean, then 'consistent' should be 'true' and it will
> > be marked clean anyway.
> >
>
> This could be true if set_array_state has possibility to execute this code in general migration case.
> During reshape after updtate curr_migr_unit set array state exits (super-intel.c: 5225 /return 0/)
>
> To achieve this it can be changed:
> 1. Consistent flag should be == 1 during reshape
> monitor.c:387
> - a->container->ss->set_array_state(a, a->curr_state <= clean);
>
> + a->container->ss->set_array_state(a, a->curr_state <= active);

This change doesn't make any sense at all.
Whether the array is 'clean' or 'active', it completely independent of
whether the array is being reshaped.
While it is being reshaped it could sometimes be clean, and sometime
be active, and the recorded state should reflect this.
Say it is 'clean' (or 'consistent') when it actually isn't would be a lie.

>
> Array state is active during reshape
>
> 2. use consistent flag during reshape for imsm
> Super-intel.c: set_array_state()
> - remove all code for a->curr_action == reshape (around line 5211)
> - instead removed code add 'goto' to calculating blocks_per_unit at the end of this function
> This will allow for mark clear/dirty volume also

I like this patch - nice cleanup of the code. I have applied it, thanks.



>
> I'll send 2 patch to make this description more clear.
> 1. FIX: During reshape array is in active state
> 2. imsm: FIX: Mark checkpoint and array state clean during reshape
>
>
> > Why cannot a reshape of a dirty volume be restarted from a checkpoint?
> > I would think it would continue with the reshape and then when that
> > finished, go back and do the resync.
> >
> > NeilBrown
>
> If array is dirty and during reshape is not assembled due to i.e:
> super-intel.c:1792 for dirty volume info->resync_start = 0; Code for analyzing migration type is not executed.

I see - that code in super-intel.c::getinfo_super_imsm_volume needs to be
fixed. Maybe just remove the 'else' there

if (map_to_analyse->map_state == IMSM_T_STATE_UNINITIALIZED ||
dev->vol.dirty) {
info->resync_start = 0;
} else if (dev->vol.migr_state) {
^^^^^

Not sure though.

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 4/9] imsm: FIX: After checkpoint mark array have to beclean

> -----Original Message-----
> From: NeilBrown [mailto:neilb@suse.de]
> Sent: Tuesday, March 08, 2011 11:14 PM
> To: Kwolek, Adam
> Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> Neubauer, Wojciech
> Subject: Re: [PATCH 4/9] imsm: FIX: After checkpoint mark array have to
> be clean
>
> On Tue, 8 Mar 2011 08:52:40 +0000 "Kwolek, Adam"
> wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: NeilBrown [mailto:neilb@suse.de]
> > > Sent: Tuesday, March 08, 2011 6:08 AM
> > > To: Kwolek, Adam
> > > Cc: linux-raid@vger.kernel.org; Williams, Dan J; Ciechanowski, Ed;
> > > Neubauer, Wojciech
> > > Subject: Re: [PATCH 4/9] imsm: FIX: After checkpoint mark array have
> to
> > > be clean
> > >
> > > On Wed, 02 Mar 2011 14:29:27 +0100 Adam Kwolek
>
> > > wrote:
> > >
> > > > When checkpoint is marked set volume as clean.
> > > > Reshape on dirty volume cannot be restarted from checkpoint.
> > > >
> > > > Signed-off-by: Adam Kwolek
> > > > ---
> > > >
> > > > super-intel.c | 1 +
> > > > 1 files changed, 1 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/super-intel.c b/super-intel.c
> > > > index 11972f3..04e32ae 100644
> > > > --- a/super-intel.c
> > > > +++ b/super-intel.c
> > > > @@ -5218,6 +5218,7 @@ static int imsm_set_array_state(struct
> > > active_array *a, int consistent)
> > > > __le32_to_cpu(dev-
> >vol.curr_migr_unit)) {
> > > > dev->vol.curr_migr_unit =
> > > > __cpu_to_le32(unit);
> > > > + dev->vol.dirty = 0;
> > > > super->updates_pending++;
> > > > }
> > > > }
> > > >
> > >
> > > hi Adam,
> > > You'll need to explain this one a bit more.
> > >
> > > If the array isn't clean, then it is wrong to mark it as clean.
> > > If it is clean, then 'consistent' should be 'true' and it will
> > > be marked clean anyway.
> > >
> >
> > This could be true if set_array_state has possibility to execute this
> code in general migration case.
> > During reshape after updtate curr_migr_unit set array state exits
> (super-intel.c: 5225 /return 0/)
> >
> > To achieve this it can be changed:
> > 1. Consistent flag should be == 1 during reshape
> > monitor.c:387
> > - a->container->ss->set_array_state(a, a->curr_state <= clean);
> >
> > + a->container->ss->set_array_state(a, a->curr_state <= active);
>
> This change doesn't make any sense at all.
> Whether the array is 'clean' or 'active', it completely independent of
> whether the array is being reshaped.
> While it is being reshaped it could sometimes be clean, and sometime
> be active, and the recorded state should reflect this.
> Say it is 'clean' (or 'consistent') when it actually isn't would be a
> lie.

During grow from some point array is always dirty, this can be not true also.
I'll put focus on enabling assembly dirty array during reshape.

>
> >
> > Array state is active during reshape
> >
> > 2. use consistent flag during reshape for imsm
> > Super-intel.c: set_array_state()
> > - remove all code for a->curr_action == reshape (around line 5211)
> > - instead removed code add 'goto' to calculating blocks_per_unit
> at the end of this function
> > This will allow for mark clear/dirty volume also
>
> I like this patch - nice cleanup of the code. I have applied it,
> thanks.
>
>
>
> >
> > I'll send 2 patch to make this description more clear.
> > 1. FIX: During reshape array is in active state
> > 2. imsm: FIX: Mark checkpoint and array state clean during reshape
> >
> >
> > > Why cannot a reshape of a dirty volume be restarted from a
> checkpoint?
> > > I would think it would continue with the reshape and then when that
> > > finished, go back and do the resync.
> > >
> > > NeilBrown
> >
> > If array is dirty and during reshape is not assembled due to i.e:
> > super-intel.c:1792 for dirty volume info->resync_start = 0; Code for
> analyzing migration type is not executed.
>
> I see - that code in super-intel.c::getinfo_super_imsm_volume needs to
> be
> fixed. Maybe just remove the 'else' there
>
> if (map_to_analyse->map_state == IMSM_T_STATE_UNINITIALIZED ||
> dev->vol.dirty) {
> info->resync_start = 0;
> } else if (dev->vol.migr_state) {
> ^^^^^
>
> Not sure though.


I've tried this yesterday at first shot, it is not enough.
I'll fix it.

Thanks
Adam

>
> 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