[PATCH 0/6] A few fixes connected with raid0<->raid10 takeover

[PATCH 0/6] A few fixes connected with raid0<->raid10 takeover

[Krzysztof Wojcik]

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

[PATCH 1/6] Limit no-restriping operations only for raid5

[Krzysztof Wojcik]

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.

Re: [PATCH 1/6] Limit no-restriping operations only for raid5

[Neil Brown]

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.

RE: [PATCH 1/6] Limit no-restriping operations only for raid5

[Wojcik, Krzysztof]

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

[PATCH 2/6] FIX: Ping manager after level change

[Krzysztof Wojcik]

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

[PATCH 3/6] Check number of failed disks durig raid10->raid0 takeover

[Krzysztof Wojcik]

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

[PATCH 4/6] FIX: Remove disks in mdmon for external metadata

[Krzysztof Wojcik]

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;

Re: [PATCH 4/6] FIX: Remove disks in mdmon for external metadata

[Neil Brown]

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;

RE: [PATCH 4/6] FIX: Remove disks in mdmon for external metadata

[Wojcik, Krzysztof]

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

[PATCH 5/6] FIX: Reset disk state if disk is missing

[Krzysztof Wojcik]

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

[PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked.

[Krzysztof Wojcik]

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;

Re: [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked.

[Neil Brown]

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;

RE: [PATCH 6/6] FIX: Unblock disk in kernel only if it is blocked.

[Wojcik, Krzysztof]

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