* [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover
@ 2011-01-19 16:23 Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 1/6] Limit no-restriping operations only for raid5 Krzysztof Wojcik
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
The following series is based on my last series.
It include a few fixes connected with raid0<->raid10 takeover
operations for imsm metadata format.
---
Krzysztof Wojcik (6):
Limit no-restriping operations only for raid5
FIX: Ping manager after level change
Check number of failed disks durig raid10->raid0 takeover
FIX: Remove disks in mdmon for external metadata
FIX: Reset disk state if disk is missing
FIX: Unblock disk in kernel only if it is blocked.
Grow.c | 86 +++++++++++++++++++++++++++++++--------------------------
monitor.c | 6 +++-
super-intel.c | 4 +++
3 files changed, 56 insertions(+), 40 deletions(-)
--
Krzysztof Wojcik
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/6] Limit no-restriping operations only for raid5
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
@ 2011-01-19 16:23 ` Krzysztof Wojcik
2011-01-26 1:01 ` Neil Brown
2011-01-19 16:23 ` [PATCH 2/6] FIX: Ping manager after level change Krzysztof Wojcik
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
If no-restriping needed, we might need to impose some more
changes: layout, raid_disks, chunk_size but only for raid5 arrays.
Move this part of code before operations connected with restriping.
Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
Grow.c | 81 ++++++++++++++++++++++++++++++++++------------------------------
1 files changed, 43 insertions(+), 38 deletions(-)
diff --git a/Grow.c b/Grow.c
index e1ec01c..0ce7c17 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1656,6 +1656,49 @@ static int reshape_array(char *container, int cfd, int fd, char *devname,
}
ping_monitor(container);
+ if (reshape.backup_blocks == 0) {
+ /* No restriping needed, but we might need to impose
+ * some more changes: layout, raid_disks, chunk_size
+ */
+ /* Changes possible only for raid5 arrays */
+ if (info->array.level != 5) {
+ unfreeze(st);
+ return rv;
+ }
+ if (info->new_layout != UnSet &&
+ info->new_layout != info->array.layout) {
+ info->array.layout = info->new_layout;
+ if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
+ fprintf(stderr, Name ": failed to set new layout\n");
+ rv = 1;
+ } else if (!quiet)
+ printf("layout for %s set to %d\n",
+ devname, info->array.layout);
+ }
+ if (info->delta_disks != UnSet &&
+ info->delta_disks != 0) {
+ info->array.raid_disks += info->delta_disks;
+ if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
+ fprintf(stderr, Name ": failed to set raid disks\n");
+ rv = 1;
+ } else if (!quiet)
+ printf("raid_disks for %s set to %d\n",
+ devname, info->array.raid_disks);
+ }
+ if (info->new_chunk != 0 &&
+ info->new_chunk != info->array.chunk_size) {
+ if (sysfs_set_num(info, NULL,
+ "chunk_size", info->new_chunk) != 0) {
+ fprintf(stderr, Name ": failed to set chunk size\n");
+ rv = 1;
+ } else if (!quiet)
+ printf("chunk size for %s set to %d\n",
+ devname, info->array.chunk_size);
+ }
+ unfreeze(st);
+ return rv;
+ }
+
/* reload metadat as it is possible to change made by monitor
*/
if ((cfd >= 0) &&
@@ -1718,44 +1761,6 @@ static int reshape_array(char *container, int cfd, int fd, char *devname,
}
}
- if (reshape.backup_blocks == 0) {
- /* No restriping needed, but we might need to impose
- * some more changes: layout, raid_disks, chunk_size
- */
- if (info->new_layout != UnSet &&
- info->new_layout != info->array.layout) {
- info->array.layout = info->new_layout;
- if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
- fprintf(stderr, Name ": failed to set new layout\n");
- rv = 1;
- } else if (!quiet)
- printf("layout for %s set to %d\n",
- devname, info->array.layout);
- }
- if (info->delta_disks != UnSet &&
- info->delta_disks != 0) {
- info->array.raid_disks += info->delta_disks;
- if (ioctl(fd, SET_ARRAY_INFO, &info->array) != 0) {
- fprintf(stderr, Name ": failed to set raid disks\n");
- rv = 1;
- } else if (!quiet)
- printf("raid_disks for %s set to %d\n",
- devname, info->array.raid_disks);
- }
- if (info->new_chunk != 0 &&
- info->new_chunk != info->array.chunk_size) {
- if (sysfs_set_num(info, NULL,
- "chunk_size", info->new_chunk) != 0) {
- fprintf(stderr, Name ": failed to set chunk size\n");
- rv = 1;
- } else if (!quiet)
- printf("chunk size for %s set to %d\n",
- devname, info->array.chunk_size);
- }
- unfreeze(st);
- return rv;
- }
-
/*
* There are three possibilities.
* 1/ The array will shrink.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/6] FIX: Ping manager after level change
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 1/6] Limit no-restriping operations only for raid5 Krzysztof Wojcik
@ 2011-01-19 16:23 ` Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 3/6] Check number of failed disks durig raid10->raid0 takeover Krzysztof Wojcik
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
Manager has to be informed about level change to refresh internal
structures.
Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
Grow.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/Grow.c b/Grow.c
index 0ce7c17..78d5b0e 100644
--- a/Grow.c
+++ b/Grow.c
@@ -1654,6 +1654,7 @@ static int reshape_array(char *container, int cfd, int fd, char *devname,
!mdmon_running(st->container_dev)) {
start_mdmon(st->container_dev);
}
+ ping_manager(container);
ping_monitor(container);
if (reshape.backup_blocks == 0) {
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/6] Check number of failed disks durig raid10->raid0 takeover
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 1/6] Limit no-restriping operations only for raid5 Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 2/6] FIX: Ping manager after level change Krzysztof Wojcik
@ 2011-01-19 16:23 ` Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 4/6] FIX: Remove disks in mdmon for external metadata Krzysztof Wojcik
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
Number of failed disks MUST be half of initial number of disks.
If number of failed disks is different we should not update
metadata- data corruption may occur after array reassemlation.
Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
super-intel.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/super-intel.c b/super-intel.c
index e284a4a..b25d4fb 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -5835,6 +5835,10 @@ static int apply_takeover_update(struct imsm_update_takeover *u,
map = get_imsm_map(dev, 0);
if (u->direction == R10_TO_R0) {
+ /* Number of failed disks must be half of initial disk number */
+ if (imsm_count_failed(super, dev) != (map->num_members / 2))
+ return 0;
+
/* iterate through devices to mark removed disks as spare */
for (dm = super->disks; dm; dm = dm->next) {
if (dm->disk.status & FAILED_DISK) {
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/6] FIX: Remove disks in mdmon for external metadata
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
` (2 preceding siblings ...)
2011-01-19 16:23 ` [PATCH 3/6] Check number of failed disks durig raid10->raid0 takeover Krzysztof Wojcik
@ 2011-01-19 16:23 ` Krzysztof Wojcik
2011-01-26 1:04 ` Neil Brown
2011-01-19 16:23 ` [PATCH 5/6] FIX: Reset disk state if disk is missing Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked Krzysztof Wojcik
5 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
For raid10 -> raid0 takeover operation we shoud reject disks
in mirror by mark them as 'failed' and then remove them from
array.
For external metadata second action should be execuded by
mdmon so additional condition is necessary.
Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
Grow.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/Grow.c b/Grow.c
index 78d5b0e..c5f83a8 100644
--- a/Grow.c
+++ b/Grow.c
@@ -711,7 +711,9 @@ int remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
sysfs_set_str(sra, sd, "state", "faulty");
sysfs_set_str(sra, sd, "slot", "none");
- sysfs_set_str(sra, sd, "state", "remove");
+ /* for external metadata disks should be removed in mdmon */
+ if (!st->ss->external)
+ sysfs_set_str(sra, sd, "state", "remove");
sd->disk.state |= (1<<MD_DISK_REMOVED);
sd->disk.state &= ~(1<<MD_DISK_SYNC);
sd->next = sra->devs;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/6] FIX: Reset disk state if disk is missing
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
` (3 preceding siblings ...)
2011-01-19 16:23 ` [PATCH 4/6] FIX: Remove disks in mdmon for external metadata Krzysztof Wojcik
@ 2011-01-19 16:23 ` Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked Krzysztof Wojcik
5 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
If we can't read actual disk state, it shoud be initiated
to 0.
Overwise it may be out of date value resulting false action
later in code (e.g. set disk to improper state).
Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
monitor.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/monitor.c b/monitor.c
index b7287d8..ed3ce49 100644
--- a/monitor.c
+++ b/monitor.c
@@ -232,6 +232,8 @@ static int read_and_act(struct active_array *a)
if (mdi->state_fd >= 0) {
mdi->recovery_start = read_resync_start(mdi->recovery_fd);
mdi->curr_state = read_dev_state(mdi->state_fd);
+ } else {
+ mdi->curr_state = 0;
}
}
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked.
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
` (4 preceding siblings ...)
2011-01-19 16:23 ` [PATCH 5/6] FIX: Reset disk state if disk is missing Krzysztof Wojcik
@ 2011-01-19 16:23 ` Krzysztof Wojcik
2011-01-26 1:06 ` Neil Brown
5 siblings, 1 reply; 13+ messages in thread
From: Krzysztof Wojcik @ 2011-01-19 16:23 UTC (permalink / raw)
To: neilb
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
Missing condition during setting next state for disk.
Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
---
monitor.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/monitor.c b/monitor.c
index ed3ce49..4fba5fd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -336,7 +336,9 @@ static int read_and_act(struct active_array *a)
a->container->ss->set_disk(a, mdi->disk.raid_disk,
mdi->curr_state);
check_degraded = 1;
- mdi->next_state |= DS_UNBLOCK;
+ /* if '-blocked' wasn't passed to kernel, do it now */
+ if (mdi->curr_state == DS_BLOCKED)
+ mdi->next_state |= DS_UNBLOCK;
if (a->curr_state == read_auto) {
a->container->ss->set_array_state(a, 0);
a->next_state = active;
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] Limit no-restriping operations only for raid5
2011-01-19 16:23 ` [PATCH 1/6] Limit no-restriping operations only for raid5 Krzysztof Wojcik
@ 2011-01-26 1:01 ` Neil Brown
2011-01-26 11:56 ` Wojcik, Krzysztof
0 siblings, 1 reply; 13+ messages in thread
From: Neil Brown @ 2011-01-26 1:01 UTC (permalink / raw)
To: Krzysztof Wojcik
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
On Wed, 19 Jan 2011 17:23:08 +0100
Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
> If no-restriping needed, we might need to impose some more
> changes: layout, raid_disks, chunk_size but only for raid5 arrays.
> Move this part of code before operations connected with restriping.
I haven't applied this.
It is not clear to me that the code is in the wrong place.
Maybe you introduced some code before this that should have gone after?
or something....
Don't know ... maybe if you try again it will make more sense next
time.
NeilBrown
>
> Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> ---
> Grow.c | 81
> ++++++++++++++++++++++++++++++++++------------------------------ 1
> files changed, 43 insertions(+), 38 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index e1ec01c..0ce7c17 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1656,6 +1656,49 @@ static int reshape_array(char *container, int
> cfd, int fd, char *devname, }
> ping_monitor(container);
>
> + if (reshape.backup_blocks == 0) {
> + /* No restriping needed, but we might need to impose
> + * some more changes: layout, raid_disks, chunk_size
> + */
> + /* Changes possible only for raid5 arrays */
> + if (info->array.level != 5) {
> + unfreeze(st);
> + return rv;
> + }
> + if (info->new_layout != UnSet &&
> + info->new_layout != info->array.layout) {
> + info->array.layout = info->new_layout;
> + if (ioctl(fd, SET_ARRAY_INFO,
> &info->array) != 0) {
> + fprintf(stderr, Name ": failed to
> set new layout\n");
> + rv = 1;
> + } else if (!quiet)
> + printf("layout for %s set to %d\n",
> + devname, info->array.layout);
> + }
> + if (info->delta_disks != UnSet &&
> + info->delta_disks != 0) {
> + info->array.raid_disks += info->delta_disks;
> + if (ioctl(fd, SET_ARRAY_INFO,
> &info->array) != 0) {
> + fprintf(stderr, Name ": failed to
> set raid disks\n");
> + rv = 1;
> + } else if (!quiet)
> + printf("raid_disks for %s set to
> %d\n",
> + devname,
> info->array.raid_disks);
> + }
> + if (info->new_chunk != 0 &&
> + info->new_chunk != info->array.chunk_size) {
> + if (sysfs_set_num(info, NULL,
> + "chunk_size",
> info->new_chunk) != 0) {
> + fprintf(stderr, Name ": failed to
> set chunk size\n");
> + rv = 1;
> + } else if (!quiet)
> + printf("chunk size for %s set to
> %d\n",
> + devname,
> info->array.chunk_size);
> + }
> + unfreeze(st);
> + return rv;
> + }
> +
> /* reload metadat as it is possible to change made by monitor
> */
> if ((cfd >= 0) &&
> @@ -1718,44 +1761,6 @@ static int reshape_array(char *container, int
> cfd, int fd, char *devname, }
> }
>
> - if (reshape.backup_blocks == 0) {
> - /* No restriping needed, but we might need to impose
> - * some more changes: layout, raid_disks, chunk_size
> - */
> - if (info->new_layout != UnSet &&
> - info->new_layout != info->array.layout) {
> - info->array.layout = info->new_layout;
> - if (ioctl(fd, SET_ARRAY_INFO,
> &info->array) != 0) {
> - fprintf(stderr, Name ": failed to
> set new layout\n");
> - rv = 1;
> - } else if (!quiet)
> - printf("layout for %s set to %d\n",
> - devname, info->array.layout);
> - }
> - if (info->delta_disks != UnSet &&
> - info->delta_disks != 0) {
> - info->array.raid_disks += info->delta_disks;
> - if (ioctl(fd, SET_ARRAY_INFO,
> &info->array) != 0) {
> - fprintf(stderr, Name ": failed to
> set raid disks\n");
> - rv = 1;
> - } else if (!quiet)
> - printf("raid_disks for %s set to
> %d\n",
> - devname,
> info->array.raid_disks);
> - }
> - if (info->new_chunk != 0 &&
> - info->new_chunk != info->array.chunk_size) {
> - if (sysfs_set_num(info, NULL,
> - "chunk_size",
> info->new_chunk) != 0) {
> - fprintf(stderr, Name ": failed to
> set chunk size\n");
> - rv = 1;
> - } else if (!quiet)
> - printf("chunk size for %s set to
> %d\n",
> - devname,
> info->array.chunk_size);
> - }
> - unfreeze(st);
> - return rv;
> - }
> -
> /*
> * There are three possibilities.
> * 1/ The array will shrink.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/6] FIX: Remove disks in mdmon for external metadata
2011-01-19 16:23 ` [PATCH 4/6] FIX: Remove disks in mdmon for external metadata Krzysztof Wojcik
@ 2011-01-26 1:04 ` Neil Brown
2011-01-26 13:00 ` Wojcik, Krzysztof
0 siblings, 1 reply; 13+ messages in thread
From: Neil Brown @ 2011-01-26 1:04 UTC (permalink / raw)
To: Krzysztof Wojcik
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
On Wed, 19 Jan 2011 17:23:33 +0100
Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
> For raid10 -> raid0 takeover operation we shoud reject disks
> in mirror by mark them as 'failed' and then remove them from
> array.
> For external metadata second action should be execuded by
> mdmon so additional condition is necessary.
I don't understand this..
I think you are saying that removing failed devices should be done by
mdmon rather than mdadm. Is that what you are saying?
I don't think that is correct. WHY do you want to make this change?
What is not working that should.
NeilBrown
>
> Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> ---
> Grow.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/Grow.c b/Grow.c
> index 78d5b0e..c5f83a8 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -711,7 +711,9 @@ int
> remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
> sysfs_set_str(sra, sd, "state", "faulty");
> sysfs_set_str(sra, sd, "slot", "none");
> - sysfs_set_str(sra, sd, "state", "remove");
> + /* for external metadata disks should be removed in
> mdmon */
> + if (!st->ss->external)
> + sysfs_set_str(sra, sd, "state", "remove");
> sd->disk.state |= (1<<MD_DISK_REMOVED);
> sd->disk.state &= ~(1<<MD_DISK_SYNC);
> sd->next = sra->devs;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked.
2011-01-19 16:23 ` [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked Krzysztof Wojcik
@ 2011-01-26 1:06 ` Neil Brown
2011-01-26 14:22 ` Wojcik, Krzysztof
0 siblings, 1 reply; 13+ messages in thread
From: Neil Brown @ 2011-01-26 1:06 UTC (permalink / raw)
To: Krzysztof Wojcik
Cc: linux-raid, wojciech.neubauer, adam.kwolek, dan.j.williams,
ed.ciechanowski
On Wed, 19 Jan 2011 17:23:49 +0100
Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
> Missing condition during setting next state for disk.
Yet again you completely fail to explain why you want to make this
change.
And again it looks wrong.
There is no harm is setting "-blocked" if the device is not actually
blocked is there?
PLEASE PLEASE PLEASE explain exactly the problem you are trying to
fix. Don't waste my time and yours by making have to continually ask
for explanations.
NeilBrown
>
> Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> ---
> monitor.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index ed3ce49..4fba5fd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -336,7 +336,9 @@ static int read_and_act(struct active_array *a)
> a->container->ss->set_disk(a,
> mdi->disk.raid_disk, mdi->curr_state);
> check_degraded = 1;
> - mdi->next_state |= DS_UNBLOCK;
> + /* if '-blocked' wasn't passed to kernel, do
> it now */
> + if (mdi->curr_state == DS_BLOCKED)
> + mdi->next_state |= DS_UNBLOCK;
> if (a->curr_state == read_auto) {
> a->container->ss->set_array_state(a,
> 0); a->next_state = active;
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 1/6] Limit no-restriping operations only for raid5
2011-01-26 1:01 ` Neil Brown
@ 2011-01-26 11:56 ` Wojcik, Krzysztof
0 siblings, 0 replies; 13+ messages in thread
From: Wojcik, Krzysztof @ 2011-01-26 11:56 UTC (permalink / raw)
To: Neil Brown
Cc: linux-raid, Neubauer, Wojciech, Kwolek, Adam, Williams, Dan J,
Ciechanowski, Ed
> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Wednesday, January 26, 2011 2:02 AM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> Williams, Dan J; Ciechanowski, Ed
> Subject: Re: [PATCH 1/6] Limit no-restriping operations only for raid5
>
> On Wed, 19 Jan 2011 17:23:08 +0100
> Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
>
> > If no-restriping needed, we might need to impose some more
> > changes: layout, raid_disks, chunk_size but only for raid5 arrays.
> > Move this part of code before operations connected with restriping.
>
> I haven't applied this.
> It is not clear to me that the code is in the wrong place.
> Maybe you introduced some code before this that should have gone after?
> or something....
>
> Don't know ... maybe if you try again it will make more sense next
> time.
>
> NeilBrown
Hi,
This patch is just cleanup- code connected with non-restriping operations has been moved just after level change and monitor running, before the code used by restriping operations.
The patch is not necessary.
Krzysztof Wojcik
>
>
> >
> > Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> > ---
> > Grow.c | 81
> > ++++++++++++++++++++++++++++++++++------------------------------ 1
> > files changed, 43 insertions(+), 38 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index e1ec01c..0ce7c17 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -1656,6 +1656,49 @@ static int reshape_array(char *container, int
> > cfd, int fd, char *devname, }
> > ping_monitor(container);
> >
> > + if (reshape.backup_blocks == 0) {
> > + /* No restriping needed, but we might need to impose
> > + * some more changes: layout, raid_disks, chunk_size
> > + */
> > + /* Changes possible only for raid5 arrays */
> > + if (info->array.level != 5) {
> > + unfreeze(st);
> > + return rv;
> > + }
> > + if (info->new_layout != UnSet &&
> > + info->new_layout != info->array.layout) {
> > + info->array.layout = info->new_layout;
> > + if (ioctl(fd, SET_ARRAY_INFO,
> > &info->array) != 0) {
> > + fprintf(stderr, Name ": failed to
> > set new layout\n");
> > + rv = 1;
> > + } else if (!quiet)
> > + printf("layout for %s set to %d\n",
> > + devname, info->array.layout);
> > + }
> > + if (info->delta_disks != UnSet &&
> > + info->delta_disks != 0) {
> > + info->array.raid_disks += info->delta_disks;
> > + if (ioctl(fd, SET_ARRAY_INFO,
> > &info->array) != 0) {
> > + fprintf(stderr, Name ": failed to
> > set raid disks\n");
> > + rv = 1;
> > + } else if (!quiet)
> > + printf("raid_disks for %s set to
> > %d\n",
> > + devname,
> > info->array.raid_disks);
> > + }
> > + if (info->new_chunk != 0 &&
> > + info->new_chunk != info->array.chunk_size) {
> > + if (sysfs_set_num(info, NULL,
> > + "chunk_size",
> > info->new_chunk) != 0) {
> > + fprintf(stderr, Name ": failed to
> > set chunk size\n");
> > + rv = 1;
> > + } else if (!quiet)
> > + printf("chunk size for %s set to
> > %d\n",
> > + devname,
> > info->array.chunk_size);
> > + }
> > + unfreeze(st);
> > + return rv;
> > + }
> > +
> > /* reload metadat as it is possible to change made by monitor
> > */
> > if ((cfd >= 0) &&
> > @@ -1718,44 +1761,6 @@ static int reshape_array(char *container, int
> > cfd, int fd, char *devname, }
> > }
> >
> > - if (reshape.backup_blocks == 0) {
> > - /* No restriping needed, but we might need to impose
> > - * some more changes: layout, raid_disks, chunk_size
> > - */
> > - if (info->new_layout != UnSet &&
> > - info->new_layout != info->array.layout) {
> > - info->array.layout = info->new_layout;
> > - if (ioctl(fd, SET_ARRAY_INFO,
> > &info->array) != 0) {
> > - fprintf(stderr, Name ": failed to
> > set new layout\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("layout for %s set to %d\n",
> > - devname, info->array.layout);
> > - }
> > - if (info->delta_disks != UnSet &&
> > - info->delta_disks != 0) {
> > - info->array.raid_disks += info->delta_disks;
> > - if (ioctl(fd, SET_ARRAY_INFO,
> > &info->array) != 0) {
> > - fprintf(stderr, Name ": failed to
> > set raid disks\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("raid_disks for %s set to
> > %d\n",
> > - devname,
> > info->array.raid_disks);
> > - }
> > - if (info->new_chunk != 0 &&
> > - info->new_chunk != info->array.chunk_size) {
> > - if (sysfs_set_num(info, NULL,
> > - "chunk_size",
> > info->new_chunk) != 0) {
> > - fprintf(stderr, Name ": failed to
> > set chunk size\n");
> > - rv = 1;
> > - } else if (!quiet)
> > - printf("chunk size for %s set to
> > %d\n",
> > - devname,
> > info->array.chunk_size);
> > - }
> > - unfreeze(st);
> > - return rv;
> > - }
> > -
> > /*
> > * There are three possibilities.
> > * 1/ The array will shrink.
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 4/6] FIX: Remove disks in mdmon for external metadata
2011-01-26 1:04 ` Neil Brown
@ 2011-01-26 13:00 ` Wojcik, Krzysztof
0 siblings, 0 replies; 13+ messages in thread
From: Wojcik, Krzysztof @ 2011-01-26 13:00 UTC (permalink / raw)
To: Neil Brown
Cc: linux-raid, Neubauer, Wojciech, Kwolek, Adam, Williams, Dan J,
Ciechanowski, Ed
> -----Original Message-----
> From: Neil Brown [mailto:neilb@suse.de]
> Sent: Wednesday, January 26, 2011 2:04 AM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> Williams, Dan J; Ciechanowski, Ed
> Subject: Re: [PATCH 4/6] FIX: Remove disks in mdmon for external
> metadata
>
> On Wed, 19 Jan 2011 17:23:33 +0100
> Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
>
> > For raid10 -> raid0 takeover operation we shoud reject disks
> > in mirror by mark them as 'failed' and then remove them from
> > array.
> > For external metadata second action should be execuded by
> > mdmon so additional condition is necessary.
>
> I don't understand this..
> I think you are saying that removing failed devices should be done by
> mdmon rather than mdadm. Is that what you are saying?
> I don't think that is correct. WHY do you want to make this change?
> What is not working that should.
>
> NeilBrown
Hi,
According the description in monitor.c:175 when monitor detect "faulty" in device state, it blocks the device, mark it as failed in metadata, unblocks the device and finally writes "remove" to device state.
So writing "remove" to device state in mdadm is not necessary- all operations connected with removing disk are supported by monitor.
Moreover write operation in mdadm is not successful because monior is blocking the device.
Krzysztof Wojcik
>
> >
> > Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> > ---
> > Grow.c | 4 +++-
> > 1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 78d5b0e..c5f83a8 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -711,7 +711,9 @@ int
> > remove_disks_on_raid10_to_raid0_takeover(struct supertype *st,
> > sysfs_set_str(sra, sd, "state", "faulty");
> > sysfs_set_str(sra, sd, "slot", "none");
> > - sysfs_set_str(sra, sd, "state", "remove");
> > + /* for external metadata disks should be removed in
> > mdmon */
> > + if (!st->ss->external)
> > + sysfs_set_str(sra, sd, "state", "remove");
> > sd->disk.state |= (1<<MD_DISK_REMOVED);
> > sd->disk.state &= ~(1<<MD_DISK_SYNC);
> > sd->next = sra->devs;
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked.
2011-01-26 1:06 ` Neil Brown
@ 2011-01-26 14:22 ` Wojcik, Krzysztof
0 siblings, 0 replies; 13+ messages in thread
From: Wojcik, Krzysztof @ 2011-01-26 14:22 UTC (permalink / raw)
To: Neil Brown
Cc: linux-raid, Neubauer, Wojciech, Kwolek, Adam, Williams, Dan J,
Ciechanowski, Ed
> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org [mailto:linux-raid-
> owner@vger.kernel.org] On Behalf Of Neil Brown
> Sent: Wednesday, January 26, 2011 2:06 AM
> To: Wojcik, Krzysztof
> Cc: linux-raid@vger.kernel.org; Neubauer, Wojciech; Kwolek, Adam;
> Williams, Dan J; Ciechanowski, Ed
> Subject: Re: [PATCH 6/6] FIX: Unblock disk in kernel only if it is
> blocked.
>
> On Wed, 19 Jan 2011 17:23:49 +0100
> Krzysztof Wojcik <krzysztof.wojcik@intel.com> wrote:
>
> > Missing condition during setting next state for disk.
>
> Yet again you completely fail to explain why you want to make this
> change.
> And again it looks wrong.
>
> There is no harm is setting "-blocked" if the device is not actually
> blocked is there?
>
> PLEASE PLEASE PLEASE explain exactly the problem you are trying to
> fix. Don't waste my time and yours by making have to continually ask
> for explanations.
>
> NeilBrown
>
Hi,
I'm sorry for insufficient explanation. I will try to be more exact in the future :)
This patch is just cleanup. Setting "-blocked" is no harm but it is problem when we use debug information. Monitor sets "-blocked" many times and it is hard to catch particular information- on the screen we get hundreds of lines with "-blocked" information and it is hard to find useful information.
I think unblock should be triggered only if device is blocked.
Krzysztof Wojcik
>
> >
> > Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@intel.com>
> > ---
> > monitor.c | 4 +++-
> > 1 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index ed3ce49..4fba5fd 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -336,7 +336,9 @@ static int read_and_act(struct active_array *a)
> > a->container->ss->set_disk(a,
> > mdi->disk.raid_disk, mdi->curr_state);
> > check_degraded = 1;
> > - mdi->next_state |= DS_UNBLOCK;
> > + /* if '-blocked' wasn't passed to kernel, do
> > it now */
> > + if (mdi->curr_state == DS_BLOCKED)
> > + mdi->next_state |= DS_UNBLOCK;
> > if (a->curr_state == read_auto) {
> > a->container->ss->set_array_state(a,
> > 0); a->next_state = active;
>
> --
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-01-26 14:22 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-19 16:23 [PATCH 0/6] A few fixes connected with raid0<->raid10 takeover Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 1/6] Limit no-restriping operations only for raid5 Krzysztof Wojcik
2011-01-26 1:01 ` Neil Brown
2011-01-26 11:56 ` Wojcik, Krzysztof
2011-01-19 16:23 ` [PATCH 2/6] FIX: Ping manager after level change Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 3/6] Check number of failed disks durig raid10->raid0 takeover Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 4/6] FIX: Remove disks in mdmon for external metadata Krzysztof Wojcik
2011-01-26 1:04 ` Neil Brown
2011-01-26 13:00 ` Wojcik, Krzysztof
2011-01-19 16:23 ` [PATCH 5/6] FIX: Reset disk state if disk is missing Krzysztof Wojcik
2011-01-19 16:23 ` [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked Krzysztof Wojcik
2011-01-26 1:06 ` Neil Brown
2011-01-26 14:22 ` Wojcik, Krzysztof
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.