On Mon, Jan 20, 2014 at 05:11:41PM +0100, Bernd Schubert wrote:
> Hello Piergiorgio,
>
> I'm going to review it in more detail later on this week and I think
> I also still have an outstanding patch. But on quick glance, I think
> your patch introduces a memory leak.
>
> > @@ -152,13 +165,14 @@
> > char *stripe_buf = xmalloc(raid_disks * chunk_size);
> > char **stripes = xmalloc(raid_disks * sizeof(char*));
> > char **blocks = xmalloc(raid_disks * sizeof(char*));
> >+ char **blocks_page = xmalloc(raid_disks * sizeof(char*));
> > int *block_index_for_slot = xmalloc(raid_disks * sizeof(int));
> > uint8_t *p = xmalloc(chunk_size);
> > uint8_t *q = xmalloc(chunk_size);
> > int *results = xmalloc(chunk_size * sizeof(int));
> > sighandler_t *sig = xmalloc(3 * sizeof(sighandler_t));
>
> I don't see free(blocks_page).
>
> Btw, clangs static code checker is rather good to find such issues.
> Actually, it reports several issues with mdadm, I just didn't have
> the time to look into it in more detail.
>
> Btw2: Any chance you could at least add the '-p' argument to your
> diff command? I think if you just would use git, it would do that
> automatically.
Hi Bernd,
good catch, I'm pretty sure there are even more :-)
Here the re-patch, i.e. the same patch, with the free().
This was done, with "diff -uNpr", I hope it fits your needs.
BTW, could you point me to some instruction on how to get
the git tree of mdadm?
--- --- ---
diff -uNrp a/raid6check.c b/raid6check.c
--- a/raid6check.c 2013-09-03 06:47:47.000000000 +0200
+++ b/raid6check.c 2014-01-20 19:18:50.829188974 +0100
@@ -27,6 +27,9 @@
#include <signal.h>
#include <sys/mman.h>
+#define CHECK_PAGE_BITS (12)
+#define CHECK_PAGE_SIZE (1 << CHECK_PAGE_BITS)
+
enum repair {
NO_REPAIR = 0,
MANUAL_REPAIR,
@@ -73,15 +76,15 @@ void raid6_collect(int chunk_size, uint8
}
}
-/* Try to find out if a specific disk has problems */
-int raid6_stats(int *results, int raid_disks, int chunk_size)
+/* Try to find out if a specific disk has problems in a CHECK_PAGE_SIZE page size */
+int raid6_stats_blk(int *results, int raid_disks)
{
int i;
int curr_broken_disk = -255;
int prev_broken_disk = -255;
int broken_status = 0;
- for(i = 0; i < chunk_size; i++) {
+ for(i = 0; i < CHECK_PAGE_SIZE; i++) {
if(results[i] != -255)
curr_broken_disk = results[i];
@@ -112,6 +115,16 @@ int raid6_stats(int *results, int raid_d
return curr_broken_disk;
}
+/* Collect disks status for a strip in CHECK_PAGE_SIZE page size blocks */
+void raid6_stats(int *disk, int *results, int raid_disks, int chunk_size)
+{
+ int i, j;
+
+ for(i = 0, j = 0; i < chunk_size; i += CHECK_PAGE_SIZE, j++) {
+ disk[j] = raid6_stats_blk(&results[i], raid_disks);
+ }
+}
+
int lock_stripe(struct mdinfo *info, unsigned long long start,
int chunk_size, int data_disks, sighandler_t *sig) {
int rv;
@@ -152,13 +165,14 @@ int check_stripes(struct mdinfo *info, i
char *stripe_buf = xmalloc(raid_disks * chunk_size);
char **stripes = xmalloc(raid_disks * sizeof(char*));
char **blocks = xmalloc(raid_disks * sizeof(char*));
+ char **blocks_page = xmalloc(raid_disks * sizeof(char*));
int *block_index_for_slot = xmalloc(raid_disks * sizeof(int));
uint8_t *p = xmalloc(chunk_size);
uint8_t *q = xmalloc(chunk_size);
int *results = xmalloc(chunk_size * sizeof(int));
sighandler_t *sig = xmalloc(3 * sizeof(sighandler_t));
- int i;
+ int i, j;
int diskP, diskQ;
int data_disks = raid_disks - 2;
int err = 0;
@@ -172,7 +186,7 @@ int check_stripes(struct mdinfo *info, i
stripes[i] = stripe_buf + i * chunk_size;
while (length > 0) {
- int disk;
+ int disk[chunk_size >> CHECK_PAGE_BITS];
printf("pos --> %llu\n", start);
@@ -217,26 +231,31 @@ int check_stripes(struct mdinfo *info, i
block_index_for_slot[diskP] = data_disks;
blocks[data_disks+1] = stripes[diskQ];
block_index_for_slot[diskQ] = data_disks+1;
-
+/* Do we really need the code below? */
+#if 0
if (memcmp(p, stripes[diskP], chunk_size) != 0) {
printf("P(%d) wrong at %llu\n", diskP, start);
}
if (memcmp(q, stripes[diskQ], chunk_size) != 0) {
printf("Q(%d) wrong at %llu\n", diskQ, start);
}
+#endif
raid6_collect(chunk_size, p, q, stripes[diskP], stripes[diskQ], results);
- disk = raid6_stats(results, raid_disks, chunk_size);
+ raid6_stats(disk, results, raid_disks, chunk_size);
- if(disk >= -2) {
- disk = geo_map(disk, start, raid_disks, level, layout);
- }
- if(disk >= 0) {
- printf("Error detected at %llu: possible failed disk slot: %d --> %s\n",
- start, disk, name[disk]);
- }
- if(disk == -65535) {
- printf("Error detected at %llu: disk slot unknown\n", start);
+ for(j = 0; i < (chunk_size >> CHECK_PAGE_BITS); j++) {
+ if(disk[j] >= -2) {
+ disk[j] = geo_map(disk[j], start, raid_disks, level, layout);
+ }
+ if(disk[j] >= 0) {
+ printf("Error detected at %llu, page %d: possible failed disk slot: %d --> %s\n",
+ start, j, disk[j], name[disk[j]]);
+ }
+ if(disk[j] == -65535) {
+ printf("Error detected at %llu, page %d: disk slot unknown\n", start, j);
+ }
}
+
if(repair == MANUAL_REPAIR) {
printf("Repairing stripe %llu\n", start);
printf("Assuming slots %d (%s) and %d (%s) are incorrect\n",
@@ -325,21 +344,37 @@ int check_stripes(struct mdinfo *info, i
goto exitCheck;
}
- } 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);
+ }
+
+ int pages_to_write_count = 0;
+ int page_to_write[chunk_size >> CHECK_PAGE_BITS];
+ for(j = 0; i < (chunk_size >> CHECK_PAGE_BITS); j++) {
+ if (disk[j] >= 0 && repair == AUTO_REPAIR) {
+ printf("Auto-repairing slot %d (%s)\n", disk[j], name[disk[j]]);
+ pages_to_write_count++;
+ page_to_write[j] = 1;
+ for(i = 0; i < raid_disks; i++) {
+ blocks_page[i] = blocks[i] + j * CHECK_PAGE_SIZE;
+ }
+ if (disk[j] == diskQ) {
+ qsyndrome(p, (uint8_t*)stripes[diskQ], (uint8_t**)blocks_page, data_disks, CHECK_PAGE_SIZE);
+ } else {
+ char *all_but_failed_blocks[data_disks];
+ int failed_block_index = block_index_for_slot[disk[j]];
+ for (i=0; i < data_disks; i++)
+ if (failed_block_index == i)
+ all_but_failed_blocks[i] = stripes[diskP] + j * CHECK_PAGE_SIZE;
+ else
+ all_but_failed_blocks[i] = blocks_page[i];
+ xor_blocks(stripes[disk[j]] + j * CHECK_PAGE_SIZE,
+ all_but_failed_blocks, data_disks, CHECK_PAGE_SIZE);
+ }
} else {
- char *all_but_failed_blocks[data_disks];
- int failed_block_index = block_index_for_slot[disk];
- for (i=0; i < data_disks; i++)
- if (failed_block_index == i)
- all_but_failed_blocks[i] = stripes[diskP];
- else
- all_but_failed_blocks[i] = blocks[i];
- xor_blocks(stripes[disk],
- all_but_failed_blocks, data_disks, chunk_size);
+ page_to_write[j] = 0;
}
+ }
+
+ if(pages_to_write_count > 0) {
err = lock_stripe(info, start, chunk_size, data_disks, sig);
if(err != 0) {
@@ -348,14 +383,19 @@ int check_stripes(struct mdinfo *info, i
goto exitCheck;
}
- lseek64(source[disk], offsets[disk] + start * chunk_size, 0);
- int write_res = write(source[disk], stripes[disk], chunk_size);
+ int write_res = 0;
+ for(j = 0; j < (chunk_size >> CHECK_PAGE_BITS); j++) {
+ if(page_to_write[j] == 1) {
+ lseek64(source[disk[j]], offsets[disk[j]] + start * chunk_size + j * CHECK_PAGE_SIZE, 0);
+ write_res += write(source[disk[j]], stripes[disk[j]] + j * CHECK_PAGE_SIZE, CHECK_PAGE_SIZE);
+ }
+ }
err = unlock_all_stripes(info, sig);
- if(err != 0 || write_res != chunk_size)
+ if (err != 0 || write_res != (CHECK_PAGE_SIZE * pages_to_write_count))
goto exitCheck;
- if (write_res != chunk_size) {
+ if (write_res != (CHECK_PAGE_SIZE * pages_to_write_count)) {
fprintf(stderr, "Failed to write a full chunk.\n");
goto exitCheck;
}
@@ -370,6 +410,7 @@ exitCheck:
free(stripe_buf);
free(stripes);
free(blocks);
+ free(blocks_page);
free(block_index_for_slot);
free(p);
free(q);
--
piergiorgio
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
