[PATCH 0/6] raid6check fixes

[PATCH 0/6] raid6check fixes

[Bernd Schubert]

Here are a few raid6check fixes. This series also includes a patch for
Makefile to allow to build with optimizations. I only fixed raid6check
to build with -O3, but I don't have time left today to fix mdadm 
(e.g. Grow.c:2237:41: error: ‘info2.data_offset’ may be used uninitialized in this function [-Werror=uninitialized])

I wanted to send the memleak-fixes much earlier, but then
I had been too busy. This patch is lightly tested, patches
from today are only build-tested.
--
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/6] raid6check: Fix build of raid6check

[Bernd Schubert]

After recent git pull 'make raid6check' did not work anymore, as
sysfs_read() was called with a wrong argument and as check_env()
was used by use_udev(), but not defined.


Replace sysfs_read(..., -1, ...) by sysfs_read(..., NULL, ...)

Move check_env() from util.c to lib.c

Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
---
 lib.c        |   10 ++++++++++
 raid6check.c |    2 +-
 util.c       |   10 ----------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib.c b/lib.c
index 1958029..2c3d936 100644
--- a/lib.c
+++ b/lib.c
@@ -378,6 +378,16 @@ void print_escape(char *str)
 	}
 }
 
+int check_env(char *name)
+{
+	char *val = getenv(name);
+
+	if (val && atoi(val) == 1)
+		return 1;
+
+	return 0;
+}
+
 int use_udev(void)
 {
 	static int use = -1;
diff --git a/raid6check.c b/raid6check.c
index e9a17a7..f5aeee4 100644
--- a/raid6check.c
+++ b/raid6check.c
@@ -386,7 +386,7 @@ int main(int argc, char *argv[])
 		goto exitHere;
 	}
 
-	info = sysfs_read(mdfd, -1,
+	info = sysfs_read(mdfd, NULL,
 			  GET_LEVEL|
 			  GET_LAYOUT|
 			  GET_DISKS|
diff --git a/util.c b/util.c
index 3ac63e3..7da69ac 100644
--- a/util.c
+++ b/util.c
@@ -1798,16 +1798,6 @@ int start_mdmon(char *devnm)
 	return 0;
 }
 
-int check_env(char *name)
-{
-	char *val = getenv(name);
-
-	if (val && atoi(val) == 1)
-		return 1;
-
-	return 0;
-}
-
 __u32 random32(void)
 {
 	__u32 rv;

[PATCH 2/6] Makefile: Allow the user to pass EXTRA_CFLAGS

[Bernd Schubert]

Users might want to compile with optimization levels and
CXFLAGS as suggested in the Makefile comment does not work.

Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
---
 Makefile |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 8cb8d51..09e08fa 100644
--- a/Makefile
+++ b/Makefile
@@ -27,8 +27,8 @@
 #           Australia
 #
 
-# define "CXFLAGS" to give extra flags to CC.
-# e.g.  make CXFLAGS=-O to optimise
+# define "EXTRA_CFLAGS" to give extra flags to CC.
+# e.g.  make EXTRA_CFLAGS=-O to optimise
 TCC = tcc
 UCLIBC_GCC = $(shell for nm in i386-uclibc-linux-gcc i386-uclibc-gcc; do which $$nm > /dev/null && { echo $$nm ; exit; } ; done; echo false No uclibc found )
 #DIET_GCC = diet gcc
@@ -41,7 +41,7 @@ KLIBC=/home/src/klibc/klibc-0.77
 KLIBC_GCC = gcc -nostdinc -iwithprefix include -I$(KLIBC)/klibc/include -I$(KLIBC)/linux/include -I$(KLIBC)/klibc/arch/i386/include -I$(KLIBC)/klibc/include/bits32
 
 CC = $(CROSS_COMPILE)gcc
-CXFLAGS = -ggdb
+CXFLAGS = -ggdb $(EXTRA_CFLAGS)
 CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter
 ifdef WARN_UNUSED
 CWFLAGS += -Wp,-D_FORTIFY_SOURCE=2 -O

Re: [PATCH 2/6] Makefile: Allow the user to pass EXTRA_CFLAGS

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

On Tue, 18 Jun 2013 11:09:21 +0200 Bernd Schubert
<bernd.schubert@itwm.fraunhofer.de> wrote:

> Users might want to compile with optimization levels and
> CXFLAGS as suggested in the Makefile comment does not work.

Doesn't it?  It works for me.

   make CXFLAGS=-O3

compiled with "-O3".


I haven't applied this patch (feel free to try to be more convincing) but has
applied all the others (after removing a little bit of trailing white space).

Thanks,
NeilBrown


> 
> Signed-off-by: Bernd Schubert <bernd.schubert@itwm.fraunhofer.de>
> ---
>  Makefile |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 8cb8d51..09e08fa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -27,8 +27,8 @@
>  #           Australia
>  #
>  
> -# define "CXFLAGS" to give extra flags to CC.
> -# e.g.  make CXFLAGS=-O to optimise
> +# define "EXTRA_CFLAGS" to give extra flags to CC.
> +# e.g.  make EXTRA_CFLAGS=-O to optimise
>  TCC = tcc
>  UCLIBC_GCC = $(shell for nm in i386-uclibc-linux-gcc i386-uclibc-gcc; do which $$nm > /dev/null && { echo $$nm ; exit; } ; done; echo false No uclibc found )
>  #DIET_GCC = diet gcc
> @@ -41,7 +41,7 @@ KLIBC=/home/src/klibc/klibc-0.77
>  KLIBC_GCC = gcc -nostdinc -iwithprefix include -I$(KLIBC)/klibc/include -I$(KLIBC)/linux/include -I$(KLIBC)/klibc/arch/i386/include -I$(KLIBC)/klibc/include/bits32
>  
>  CC = $(CROSS_COMPILE)gcc
> -CXFLAGS = -ggdb
> +CXFLAGS = -ggdb $(EXTRA_CFLAGS)
>  CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter
>  ifdef WARN_UNUSED
>  CWFLAGS += -Wp,-D_FORTIFY_SOURCE=2 -O


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/6] Makefile: Allow the user to pass EXTRA_CFLAGS

[Bernd Schubert]

Hello Neil,

On 06/19/2013 02:07 AM, NeilBrown wrote:
> On Tue, 18 Jun 2013 11:09:21 +0200 Bernd Schubert
> <bernd.schubert@itwm.fraunhofer.de> wrote:
>
>> Users might want to compile with optimization levels and
>> CXFLAGS as suggested in the Makefile comment does not work.
>
> Doesn't it?  It works for me.
>
>     make CXFLAGS=-O3
>
> compiled with "-O3".
>
>
> I haven't applied this patch (feel free to try to be more convincing) but has
> applied all the others (after removing a little bit of trailing white space).
>


I'm sorry for the extra work with the white space. Next time I'm first 
going to run linux/scripts/checkpatch.pl

Hmm, indeed

make CXFLAGS=-O3

works, but the other way around it does not:

CXFLAGS=-O3 make

I still wonder how it works, as Makefile sets "CXFLAGS = -ggdb".
Anyway, whichever exception 'make' has for that, can we change it to

-CXFLAGS = -ggdb
+CXFLAGS ?= -ggdb

which would make it work with either way CXFLAGS is specified.


Thanks,
Bernd

Re: [PATCH 2/6] Makefile: Allow the user to pass EXTRA_CFLAGS

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

On Wed, 19 Jun 2013 11:48:47 +0200 Bernd Schubert
<bernd.schubert@itwm.fraunhofer.de> wrote:

> Hello Neil,
> 
> On 06/19/2013 02:07 AM, NeilBrown wrote:
> > On Tue, 18 Jun 2013 11:09:21 +0200 Bernd Schubert
> > <bernd.schubert@itwm.fraunhofer.de> wrote:
> >
> >> Users might want to compile with optimization levels and
> >> CXFLAGS as suggested in the Makefile comment does not work.
> >
> > Doesn't it?  It works for me.
> >
> >     make CXFLAGS=-O3
> >
> > compiled with "-O3".
> >
> >
> > I haven't applied this patch (feel free to try to be more convincing) but has
> > applied all the others (after removing a little bit of trailing white space).
> >
> 
> 
> I'm sorry for the extra work with the white space. Next time I'm first 
> going to run linux/scripts/checkpatch.pl
> 
> Hmm, indeed
> 
> make CXFLAGS=-O3
> 
> works, but the other way around it does not:
> 
> CXFLAGS=-O3 make
> 
> I still wonder how it works, as Makefile sets "CXFLAGS = -ggdb".
> Anyway, whichever exception 'make' has for that, can we change it to
> 
> -CXFLAGS = -ggdb
> +CXFLAGS ?= -ggdb
> 
> which would make it work with either way CXFLAGS is specified.
> 
> 
> Thanks,
> Bernd


Sounds fair.  I've made that change.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 3/6] raid6check: Fix memory leaks detected by valgrind

[Bernd Schubert]

==2389947== 24 bytes in 1 blocks are definitely lost in loss record 1 of 10
==2389947==    at 0x4C2B3F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2389947==    by 0x408067: xmalloc (xmalloc.c:36)
==2389947==    by 0x401B19: check_stripes (raid6check.c:151)
==2389947==    by 0x4030C6: main (raid6check.c:521)
==2389947==
==2389947== 24 bytes in 1 blocks are definitely lost in loss record 2 of 10
==2389947==    at 0x4C2B3F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2389947==    by 0x408067: xmalloc (xmalloc.c:36)
==2389947==    by 0x401B67: check_stripes (raid6check.c:155)
==2389947==    by 0x4030C6: main (raid6check.c:521)
==2389947==

Signed-off-by: Bernd Schubert <bernd.schubert@fastmail.fm>
---
 raid6check.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/raid6check.c b/raid6check.c
index f5aeee4..17f7430 100644
--- a/raid6check.c
+++ b/raid6check.c
@@ -325,9 +325,11 @@ exitCheck:
 	free(stripe_buf);
 	free(stripes);
 	free(blocks);
+	free(block_index_for_slot);
 	free(p);
 	free(q);
 	free(results);
+	free(sig);
 
 	return err;
 }

[PATCH 4/6] raid6check: Use enums for repair type

[Bernd Schubert]

Using hard coded numbers is error prone and hard to read by humans.


Signed-off-by: Bernd Schubert <bernd.schubert@fastmail.fm>
---
 raid6check.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/raid6check.c b/raid6check.c
index 17f7430..480422d 100644
--- a/raid6check.c
+++ b/raid6check.c
@@ -27,6 +27,12 @@
 #include <signal.h>
 #include <sys/mman.h>
 
+enum repair {
+	NO_REPAIR = 0,
+	MANUAL_REPAIR,
+	AUTO_REPAIR
+};
+
 int geo_map(int block, unsigned long long stripe, int raid_disks,
 	    int level, int layout);
 void qsyndrome(uint8_t *p, uint8_t *q, uint8_t **sources, int disks, int size);
@@ -142,7 +148,7 @@ int unlock_all_stripes(struct mdinfo *info, sighandler_t *sig) {
 int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 		  int raid_disks, int chunk_size, int level, int layout,
 		  unsigned long long start, unsigned long long length, char *name[],
-		  int repair, int failed_disk1, int failed_disk2)
+		  enum repair repair, int failed_disk1, int failed_disk2)
 {
 	/* read the data and p and q blocks, and check we got them right */
 	char *stripe_buf = xmalloc(raid_disks * chunk_size);
@@ -220,7 +226,7 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 		if(disk == -65535) {
 			printf("Error detected at %llu: disk slot unknown\n", start);
 		}
-		if(repair == 1) {
+		if(repair == MANUAL_REPAIR) {
 			printf("Repairing stripe %llu\n", start);
 			printf("Assuming slots %d (%s) and %d (%s) are incorrect\n",
 			       failed_disk1, name[failed_disk1],
@@ -284,7 +290,7 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 			err = unlock_all_stripes(info, sig);
 			if(err != 0)
 				goto exitCheck;
-		} else if (disk >= 0 && repair == 2) {
+		} else if (disk >= 0 && repair == AUTO_REPAIR) {
 			printf("Auto-repairing slot %d (%s)\n", disk, name[disk]);
 			if (disk == diskQ) {
 				qsyndrome(p, (uint8_t*)stripes[diskQ], (uint8_t**)blocks, data_disks, chunk_size);
@@ -357,7 +363,7 @@ int main(int argc, char *argv[])
 	int chunk_size = 0;
 	int layout = -1;
 	int level = 6;
-	int repair = 0;
+	enum repair repair = NO_REPAIR;
 	int failed_disk1, failed_disk2;
 	unsigned long long start, length;
 	int i;
@@ -447,7 +453,7 @@ int main(int argc, char *argv[])
 			exit_err = 1;
 			goto exitHere;
 		}
-		repair = 1;
+		repair = MANUAL_REPAIR;
 		start = getnum(argv[3], &err);
 		length = 1;
 		failed_disk1 = getnum(argv[4], &err);
@@ -473,7 +479,7 @@ int main(int argc, char *argv[])
 		start = getnum(argv[2], &err);
 		length = getnum(argv[3], &err);
 		if (argc >= 5 && strcmp(argv[4], "autorepair")==0)
-			repair = 2;
+			repair = AUTO_REPAIR;
 	}
 
 	if (err) {

[PATCH 5/6] raid6check: Fix compiler warnings.

[Bernd Schubert]

Fix some compiler warnings appearing with optimization levels.

Signed-off-by: Bernd Schubert <bernd.schubert@fastmail.fm>
---
 raid6check.c |   32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/raid6check.c b/raid6check.c
index 480422d..90a7fd3 100644
--- a/raid6check.c
+++ b/raid6check.c
@@ -186,7 +186,12 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 		}
 		for (i = 0 ; i < raid_disks ; i++) {
 			lseek64(source[i], offsets[i] + start * chunk_size, 0);
-			read(source[i], stripes[i], chunk_size);
+			int read_res = read(source[i], stripes[i], chunk_size);
+			if (read_res < chunk_size) {
+				fprintf(stderr, "Failed to read complete chunk disk %d, aborting\n", i);
+				unlock_all_stripes(info, sig);
+				goto exitCheck;
+			}
 		}
 		err = unlock_all_stripes(info, sig);
 		if(err != 0)
@@ -282,14 +287,23 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 				goto exitCheck;
 			}
 
+			int write_res1, write_res2;
+
 			lseek64(source[failed_disk1], offsets[failed_disk1] + start * chunk_size, 0);
-			write(source[failed_disk1], stripes[failed_disk1], chunk_size);
+			write_res1 = write(source[failed_disk1], stripes[failed_disk1], chunk_size);
 			lseek64(source[failed_disk2], offsets[failed_disk2] + start * chunk_size, 0);
-			write(source[failed_disk2], stripes[failed_disk2], chunk_size);
+			write_res2 = write(source[failed_disk2], stripes[failed_disk2], chunk_size);
 
 			err = unlock_all_stripes(info, sig);
 			if(err != 0)
 				goto exitCheck;
+
+
+			if (write_res1 != chunk_size || write_res2 != chunk_size) {
+				fprintf(stderr, "Failed to write a complete chunk.\n");
+				goto exitCheck;
+			}
+
 		} else if (disk >= 0 && repair == AUTO_REPAIR) {
 			printf("Auto-repairing slot %d (%s)\n", disk, name[disk]);
 			if (disk == diskQ) {
@@ -314,11 +328,16 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 			}
 
 			lseek64(source[disk], offsets[disk] + start * chunk_size, 0);
-			write(source[disk], stripes[disk], chunk_size);
+			int write_res = write(source[disk], stripes[disk], chunk_size);
 
 			err = unlock_all_stripes(info, sig);
-			if(err != 0)
+			if(err != 0 || write_res != chunk_size)
 				goto exitCheck;
+
+			if (write_res != chunk_size) {
+				fprintf(stderr, "Failed to write a full chunk.\n");
+				goto exitCheck;
+			}
 		}
 
 
@@ -364,7 +383,8 @@ int main(int argc, char *argv[])
 	int layout = -1;
 	int level = 6;
 	enum repair repair = NO_REPAIR;
-	int failed_disk1, failed_disk2;
+	int failed_disk1 = -1; 
+	int failed_disk2 = -1;
 	unsigned long long start, length;
 	int i;
 	int mdfd;

[PATCH 6/6] raid6check: Check return value of lseek64()

[Bernd Schubert]

If lseek64() failed it was still writing to the disks, which would introduce
data corruption.

Signed-off-by: Bernd Schubert <bernd.schubert@fastmail.fm>
---
 raid6check.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/raid6check.c b/raid6check.c
index 90a7fd3..ee0de2f 100644
--- a/raid6check.c
+++ b/raid6check.c
@@ -185,11 +185,19 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 			goto exitCheck;
 		}
 		for (i = 0 ; i < raid_disks ; i++) {
-			lseek64(source[i], offsets[i] + start * chunk_size, 0);
+			off64_t seek_res = lseek64(source[i], offsets[i] + start * chunk_size, 
+						   SEEK_SET);
+			if (seek_res < 0) {
+				fprintf(stderr, "lseek to source %d failed\n", i);
+				unlock_all_stripes(info, sig);
+				err = -1;
+				goto exitCheck;
+			}
 			int read_res = read(source[i], stripes[i], chunk_size);
 			if (read_res < chunk_size) {
 				fprintf(stderr, "Failed to read complete chunk disk %d, aborting\n", i);
 				unlock_all_stripes(info, sig);
+				err = -1;
 				goto exitCheck;
 			}
 		}
@@ -288,17 +296,33 @@ int check_stripes(struct mdinfo *info, int *source, unsigned long long *offsets,
 			}
 
 			int write_res1, write_res2;
+			off64_t seek_res;
 
-			lseek64(source[failed_disk1], offsets[failed_disk1] + start * chunk_size, 0);
+			seek_res = lseek64(source[failed_disk1], 
+					   offsets[failed_disk1] + start * chunk_size, SEEK_SET);
+			if (seek_res < 0) {
+				fprintf(stderr, "lseek failed for failed_disk1\n");
+				unlock_all_stripes(info, sig);
+				err = -1;
+				goto exitCheck;
+			}
 			write_res1 = write(source[failed_disk1], stripes[failed_disk1], chunk_size);
-			lseek64(source[failed_disk2], offsets[failed_disk2] + start * chunk_size, 0);
+
+
+			seek_res = lseek64(source[failed_disk2], 
+					   offsets[failed_disk2] + start * chunk_size, SEEK_SET);
+			if (seek_res < 0) {
+				fprintf(stderr, "lseek failed for failed_disk1\n");
+				unlock_all_stripes(info, sig);
+				err = -1;
+				goto exitCheck;
+			}
 			write_res2 = write(source[failed_disk2], stripes[failed_disk2], chunk_size);
 
 			err = unlock_all_stripes(info, sig);
 			if(err != 0)
 				goto exitCheck;
 
-
 			if (write_res1 != chunk_size || write_res2 != chunk_size) {
 				fprintf(stderr, "Failed to write a complete chunk.\n");
 				goto exitCheck;

Re: [PATCH 0/6] raid6check fixes

[Piergiorgio Sartor]

On Tue, Jun 18, 2013 at 11:09:10AM +0200, Bernd Schubert wrote:
> Here are a few raid6check fixes. This series also includes a patch for
> Makefile to allow to build with optimizations. I only fixed raid6check
> to build with -O3, but I don't have time left today to fix mdadm 
> (e.g. Grow.c:2237:41: error: ‘info2.data_offset’ may be used uninitialized in this function [-Werror=uninitialized])
> 
> I wanted to send the memleak-fixes much earlier, but then
> I had been too busy. This patch is lightly tested, patches
> from today are only build-tested.

Hi Bernd,

cool!!!

Good you're keeping it up!

bye,

-- 

piergiorgio
--
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/6] raid6check fixes

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Tue, 18 Jun 2013 11:09:10 +0200 Bernd Schubert
<bernd.schubert@itwm.fraunhofer.de> wrote:

> Here are a few raid6check fixes. This series also includes a patch for
> Makefile to allow to build with optimizations. I only fixed raid6check
> to build with -O3, but I don't have time left today to fix mdadm 
> (e.g. Grow.c:2237:41: error: ‘info2.data_offset’ may be used uninitialized in this function [-Werror=uninitialized])
> 

Thanks for mentioning that.  I've fix that problem.

NeilBrown


> I wanted to send the memleak-fixes much earlier, but then
> I had been too busy. This patch is lightly tested, patches
> from today are only build-tested.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/6] raid6check fixes

[Piergiorgio Sartor]

On Wed, Jun 19, 2013 at 10:08:03AM +1000, NeilBrown wrote:
> On Tue, 18 Jun 2013 11:09:10 +0200 Bernd Schubert
> <bernd.schubert@itwm.fraunhofer.de> wrote:
> 
> > Here are a few raid6check fixes. This series also includes a patch for
> > Makefile to allow to build with optimizations. I only fixed raid6check
> > to build with -O3, but I don't have time left today to fix mdadm 
> > (e.g. Grow.c:2237:41: error: ‘info2.data_offset’ may be used uninitialized in this function [-Werror=uninitialized])
> > 
> 
> Thanks for mentioning that.  I've fix that problem.
> 
> NeilBrown
> 
> 
> > I wanted to send the memleak-fixes much earlier, but then
> > I had been too busy. This patch is lightly tested, patches
> > from today are only build-tested.
> 

Hi Neil,

BTW, what's your plan of including "raid6check"
into a main release of "mdadm" package?

Do you have "deadline" in mind?

Thanks,

bye,

-- 

piergiorgio
--
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/6] raid6check fixes

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

On Thu, 20 Jun 2013 18:16:39 +0200 Piergiorgio Sartor
<piergiorgio.sartor@nexgo.de> wrote:

> On Wed, Jun 19, 2013 at 10:08:03AM +1000, NeilBrown wrote:
> > On Tue, 18 Jun 2013 11:09:10 +0200 Bernd Schubert
> > <bernd.schubert@itwm.fraunhofer.de> wrote:
> > 
> > > Here are a few raid6check fixes. This series also includes a patch for
> > > Makefile to allow to build with optimizations. I only fixed raid6check
> > > to build with -O3, but I don't have time left today to fix mdadm 
> > > (e.g. Grow.c:2237:41: error: ‘info2.data_offset’ may be used uninitialized in this function [-Werror=uninitialized])
> > > 
> > 
> > Thanks for mentioning that.  I've fix that problem.
> > 
> > NeilBrown
> > 
> > 
> > > I wanted to send the memleak-fixes much earlier, but then
> > > I had been too busy. This patch is lightly tested, patches
> > > from today are only build-tested.
> > 
> 
> Hi Neil,
> 
> BTW, what's your plan of including "raid6check"
> into a main release of "mdadm" package?
> 
> Do you have "deadline" in mind?

What exactly do you mean by that?
raid6check.c is already in the upstream git tree and was in mdadm-3.2.5.

Do you mean when will it be included by distros?  That is really up to them.
I could get "make install" to install it I guess.
I haven't really thought about what would be required for that.

I'll add it to my list of things to think about.

NeilBrown

> 
> Thanks,
> 
> bye,
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/6] raid6check fixes

[Piergiorgio Sartor]

On Mon, Jun 24, 2013 at 05:04:58PM +1000, NeilBrown wrote:
[...]
> > Hi Neil,
> > 
> > BTW, what's your plan of including "raid6check"
> > into a main release of "mdadm" package?
> > 
> > Do you have "deadline" in mind?
> 
> What exactly do you mean by that?
> raid6check.c is already in the upstream git tree and was in mdadm-3.2.5.

Hi Neil, I was under the impression you were
holding back the patches from Robert (Buchholz).

Probably I was wrong.
 
> Do you mean when will it be included by distros?  That is really up to them.
> I could get "make install" to install it I guess.
> I haven't really thought about what would be required for that.
> 
> I'll add it to my list of things to think about.

Well, that could be some thinking for 3.3, couldn't it? :-)

In any case, yes, it would be nice to force a bit
the hand of the distros by installing "raid6check" too.

Thanks,

bye,

pg

> NeilBrown
> 
> > 
> > Thanks,
> > 
> > bye,
> > 
> 



-- 

piergiorgio

Re: [PATCH 0/6] raid6check fixes

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1559 bytes --]

On Mon, 24 Jun 2013 19:10:17 +0200 Piergiorgio Sartor
<piergiorgio.sartor@nexgo.de> wrote:

> On Mon, Jun 24, 2013 at 05:04:58PM +1000, NeilBrown wrote:
> [...]
> > > Hi Neil,
> > > 
> > > BTW, what's your plan of including "raid6check"
> > > into a main release of "mdadm" package?
> > > 
> > > Do you have "deadline" in mind?
> > 
> > What exactly do you mean by that?
> > raid6check.c is already in the upstream git tree and was in mdadm-3.2.5.
> 
> Hi Neil, I was under the impression you were
> holding back the patches from Robert (Buchholz).
> 
> Probably I was wrong.

I don't remember holding back on any raid6check patches, but then I tend to
forget such things - usually after I've replied with why, but sometimes
before.

If there are patches that you (or anyone else) think should be in but aren't,
please point them out to me.  I'll either apply them or explain why I don't
want to.

>  
> > Do you mean when will it be included by distros?  That is really up to them.
> > I could get "make install" to install it I guess.
> > I haven't really thought about what would be required for that.
> > 
> > I'll add it to my list of things to think about.
> 
> Well, that could be some thinking for 3.3, couldn't it? :-)

It could indeed.

Thanks,
NeilBrown


> 
> In any case, yes, it would be nice to force a bit
> the hand of the distros by installing "raid6check" too.
> 
> Thanks,
> 
> bye,
> 
> pg
> 
> > NeilBrown
> > 
> > > 
> > > Thanks,
> > > 
> > > bye,
> > > 
> > 
> 
> 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/6] raid6check fixes

[Piergiorgio Sartor]

On Tue, Jun 25, 2013 at 09:54:22AM +1000, NeilBrown wrote:
[...]
> I don't remember holding back on any raid6check patches, but then I tend to
> forget such things - usually after I've replied with why, but sometimes
> before.
> 
> If there are patches that you (or anyone else) think should be in but aren't,
> please point them out to me.  I'll either apply them or explain why I don't
> want to.

Hi Neil,

no, you did not seem to have forgot anything.
I quickly checked mdadm-3.3-rc1 source and it
seems raid6check.c is up to date.

> > > Do you mean when will it be included by distros?  That is really up to them.
> > > I could get "make install" to install it I guess.
> > > I haven't really thought about what would be required for that.
> > > 
> > > I'll add it to my list of things to think about.
> > 
> > Well, that could be some thinking for 3.3, couldn't it? :-)
> 
> It could indeed.

Well, do not forget... :-)

Thanks,

bye,

pg

> Thanks,
> NeilBrown
> 
> 
> > 
> > In any case, yes, it would be nice to force a bit
> > the hand of the distros by installing "raid6check" too.
> > 
> > Thanks,
> > 
> > bye,
> > 
> > pg
> > 
> > > NeilBrown
> > > 
> > > > 
> > > > Thanks,
> > > > 
> > > > bye,
> > > > 
> > > 
> > 
> > 
> > 
> 



-- 

piergiorgio

Re: [PATCH 0/6] raid6check fixes

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

On Tue, 25 Jun 2013 18:46:28 +0200 Piergiorgio Sartor
<piergiorgio.sartor@nexgo.de> wrote:

> On Tue, Jun 25, 2013 at 09:54:22AM +1000, NeilBrown wrote:
> [...]
> > I don't remember holding back on any raid6check patches, but then I tend to
> > forget such things - usually after I've replied with why, but sometimes
> > before.
> > 
> > If there are patches that you (or anyone else) think should be in but aren't,
> > please point them out to me.  I'll either apply them or explain why I don't
> > want to.
> 
> Hi Neil,
> 
> no, you did not seem to have forgot anything.
> I quickly checked mdadm-3.3-rc1 source and it
> seems raid6check.c is up to date.
> 
> > > > Do you mean when will it be included by distros?  That is really up to them.
> > > > I could get "make install" to install it I guess.
> > > > I haven't really thought about what would be required for that.
> > > > 
> > > > I'll add it to my list of things to think about.
> > > 
> > > Well, that could be some thinking for 3.3, couldn't it? :-)
> > 
> > It could indeed.
> 
> Well, do not forget... :-)
> 

I've just had a little look at raid6test - because some of the selftests were
failing.

The default output is rather verbose.  Verbose output can be good, but not as
the default I think.

However, more importantly, it pays too much heed to the chunksize.

If the start of one chunk on drive X is bad, and the end of a corresponding
chunk on drive Y is bad, then it will complain that it cannot figure out the
problem.
It shouldn't do that.  It shouldn't even look at whole chunks at a time.

It should look at blocks.  Maybe 512bytes or 1K or 4K any of those would do.
Then for each block it should figure out if there is a problem, and maybe
auto-fix it.

Having two blocks on different drives being bad might be cause for extra
warnings, but it doesn't make any difference if they are in the same stripe
or in different stripes.

i.e. the chunksize should be used only as part of the calculation to work out
the layout at any particular place in the array.  It should not be used as a
unit for reading and writing data.

In anyone feels like addressing that issue, I would be very receptive to
patches :-)

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/6] raid6check fixes

[Piergiorgio Sartor]

On Thu, Jun 27, 2013 at 02:27:19PM +1000, NeilBrown wrote:
[...]
> I've just had a little look at raid6test - because some of the selftests were
> failing.
> 
> The default output is rather verbose.  Verbose output can be good, but not as
> the default I think.
> 
> However, more importantly, it pays too much heed to the chunksize.
> 
> If the start of one chunk on drive X is bad, and the end of a corresponding
> chunk on drive Y is bad, then it will complain that it cannot figure out the
> problem.
> It shouldn't do that.  It shouldn't even look at whole chunks at a time.
> 
> It should look at blocks.  Maybe 512bytes or 1K or 4K any of those would do.
> Then for each block it should figure out if there is a problem, and maybe
> auto-fix it.

Hi Neil,

thanks for the feedback.

I checked "mdadm" man page and it states that the chunk
size is always a multiple of 4K (and a power of 2).
I assume this is correct, so I would suggest 4K as
block size. Also because of SSD and many HDD which
are already 4K.

What do you think?

Does it seem reasonable to you?

"raid6check" is processing and collecting statistics
"per byte", I assume it should not need a big architectural
change in order to fit 4K.

About the verbosity, I know, this was already discussed.
My idea was to reduce it, once the other code stabilize.

Thanks,

bye,

pg

> Having two blocks on different drives being bad might be cause for extra
> warnings, but it doesn't make any difference if they are in the same stripe
> or in different stripes.
> 
> i.e. the chunksize should be used only as part of the calculation to work out
> the layout at any particular place in the array.  It should not be used as a
> unit for reading and writing data.
> 
> In anyone feels like addressing that issue, I would be very receptive to
> patches :-)
> 
> NeilBrown



-- 

piergiorgio

Re: [PATCH 0/6] raid6check fixes

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

On Thu, 27 Jun 2013 22:49:00 +0200 Piergiorgio Sartor
<piergiorgio.sartor@nexgo.de> wrote:

> On Thu, Jun 27, 2013 at 02:27:19PM +1000, NeilBrown wrote:
> [...]
> > I've just had a little look at raid6test - because some of the selftests were
> > failing.
> > 
> > The default output is rather verbose.  Verbose output can be good, but not as
> > the default I think.
> > 
> > However, more importantly, it pays too much heed to the chunksize.
> > 
> > If the start of one chunk on drive X is bad, and the end of a corresponding
> > chunk on drive Y is bad, then it will complain that it cannot figure out the
> > problem.
> > It shouldn't do that.  It shouldn't even look at whole chunks at a time.
> > 
> > It should look at blocks.  Maybe 512bytes or 1K or 4K any of those would do.
> > Then for each block it should figure out if there is a problem, and maybe
> > auto-fix it.
> 
> Hi Neil,
> 
> thanks for the feedback.
> 
> I checked "mdadm" man page and it states that the chunk
> size is always a multiple of 4K (and a power of 2).
> I assume this is correct, so I would suggest 4K as
> block size. Also because of SSD and many HDD which
> are already 4K.
> 
> What do you think?
> 
> Does it seem reasonable to you?

Yes, using 4K blocks is perfectly reasonable.

> 
> "raid6check" is processing and collecting statistics
> "per byte", I assume it should not need a big architectural
> change in order to fit 4K.
> 
> About the verbosity, I know, this was already discussed.
> My idea was to reduce it, once the other code stabilize.

Fair enough.  I was thinking in the context of making raid6check part of the
default install.  I wouldn't want to do that until it was more "user
friendly".  I also wouldn't want to do it until he code had stabilized.  So
your idea fits perfectly.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/6] raid6check fixes

[Piergiorgio Sartor]

On Fri, Jun 28, 2013 at 07:12:50AM +1000, NeilBrown wrote:
[...]
> Fair enough.  I was thinking in the context of making raid6check part of the
> default install.  I wouldn't want to do that until it was more "user
> friendly".  I also wouldn't want to do it until he code had stabilized.  So
> your idea fits perfectly.

Hi Neil,

seems reasonable, this discussion was exactly
triggered in order to understand what is missing
and what should be done for "raid6check" in
order to be part of the official "mdadm" binary set.

So, to sum it up:

1) 4K block size resolution for processing
2) Verbosity under controll

I guess these are the open points.

Thanks,

bye,

pg

> Thanks,
> NeilBrown
> 



-- 

piergiorgio