On 14:10, NeilBrown wrote:

-static int stripe_to_pdidx(sector_t stripe, raid5_conf_t *conf, int previous);
+static int stripe_to_pdidx(sector_t stripe, raid5_conf_t *conf, int previous,
+			   int *qd_idx);

That's a bit ugly, isn't it? The function computes both the p index
and the q index which is not obvious from its name. Also, the p index
is the return value and the q index is returned via a pointer which
is a bit unsymmetric.

static void get_parity_indices(sector_t stripe, raid5_conf_t *conf, int previous,
				 int *pd_idx, int *qd_idx);

perhaps?

+	/* Note that unlike RAID-5, the ordering of the disks matters greatly.*/
+	/* FIX: Is this ordering of drives even remotely optimal? */
+	count = 0;
+	i = d0_idx;
+	do {
+		if (i == sh->pd_idx)
+			ptrs[disks-2] = page_address(sh->dev[i].page);
+		else if (i == sh->qd_idx)
+			ptrs[disks-1] = page_address(sh->dev[i].page);
+		else {
 			ptrs[count++] = page_address(sh->dev[i].page);
-			if (count <= disks-2 && !test_bit(R5_UPTODATE, &sh->dev[i].flags))
+			if (!test_bit(R5_UPTODATE, &sh->dev[i].flags))
 				printk("block %d/%d not uptodate on parity calc\n", i,count);
-			i = raid6_next_disk(i, disks);
-		} while ( i != d0_idx );
-//		break;
-//	}
+		}
+		i = raid6_next_disk(i, disks);
+	} while (i != d0_idx);
+	BUG_ON(count+2 != disks);

Isn't it really dangerous to ignore a dirty stripe head during parity
calculation? I understand that compute_parity6() does not have a
return value and that dirty stripe heads have always been ignored by
compute_parity6(). But it looks much saner to fail in this case.

BTW, the printk lacks a KERN_ERR.

+	/**** FIX THIS: This could be very bad if disks is close to 256 ****/
+	void *ptrs[disks];

How serious is this? It's probably not a problem for version 0.90
superblocks as one can't have more than 26 disks. OTOH, for version
1 superblocks we might end up using up to 2K of stack space on 64
bit machines.

Would it be a reasonable to always allocate 26 pointers, say, and
fall back to malloc() if this turns out to be too small?

+	count = 0;
+	i = d0_idx;
+	do {
+		int slot;
+		if (i == sh->pd_idx)
+			slot = disks-2;
+		else if (i == sh->qd_idx)
+			slot = disks-1;
+		else
+			slot = count++;
+		ptrs[slot] = page_address(sh->dev[i].page);
+		if (i == dd_idx1)
+			faila = slot;
+		if (i == dd_idx2)
+			failb = slot;
+		i = raid6_next_disk(i, disks);
+	} while (i != d0_idx);
+	BUG_ON(count+2 != disks);

Deja vu. How about using a helper function like

static inline int is_parity_idx(int idx, struct stripe_head_sh)
{
	if (idx == sh->pd_idx)
		return sh->disks - 2;
	if (idx == sh->qd_idx)
		return sh->disks - 1;
	return 0;
}

Then the above becomes

	int slot = is_parity_idx(i, sh);
	if (!slot)
		slot = count++;

And in compute_parity6() we could write

	count = 0;
	i = d0_idx;
	do {
		int slot = is_parity_idx(i, sh);
		if (!slot)
			slot = count++;
		ptrs[slot] = page_address(sh->dev[i].page);


Regards
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe