<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "> <div><div>On Jul 3, 2012, at 8:21 PM, NeilBrown wrote:</div> <blockquote type="cite"><span class="Apple-style-span" style="border-collapse: separate; font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; ">On Tue, 26 Jun 2012 07:03:51 -0500 Jonathan Brassow <<a href="mailto:jbrassow@redhat.com">jbrassow@redhat.com</a>> wrote: <blockquote type="cite">dm raid: add md raid10 support </blockquote><blockquote type="cite"> </blockquote><blockquote type="cite">Support the MD RAID10 personality through dm-raid.c </blockquote><blockquote type="cite"> </blockquote><blockquote type="cite">Signed-off-by: Jonathan Brassow <<a href="mailto:jbrassow@redhat.com">jbrassow@redhat.com</a>> </blockquote><blockquote type="cite"> </blockquote><blockquote type="cite">Index: linux-upstream/drivers/md/dm-raid.c </blockquote><blockquote type="cite">=================================================================== </blockquote><blockquote type="cite">--- linux-upstream.orig/drivers/md/dm-raid.c </blockquote><blockquote type="cite">+++ linux-upstream/drivers/md/dm-raid.c </blockquote><blockquote type="cite">@@ -11,6 +11,7 @@ </blockquote><blockquote type="cite">#include "md.h" </blockquote><blockquote type="cite">#include "raid1.h" </blockquote><blockquote type="cite">#include "raid5.h" </blockquote><blockquote type="cite">+#include "raid10.h" </blockquote><blockquote type="cite">#include "bitmap.h" </blockquote><blockquote type="cite"> </blockquote><blockquote type="cite">#include <linux/device-mapper.h> </blockquote><blockquote type="cite">@@ -52,7 +53,11 @@ struct raid_dev { </blockquote><blockquote type="cite">#define DMPF_MAX_RECOVERY_RATE 0x20 </blockquote><blockquote type="cite">#define DMPF_MAX_WRITE_BEHIND 0x40 </blockquote><blockquote type="cite">#define DMPF_STRIPE_CACHE 0x80 </blockquote><blockquote type="cite">-#define DMPF_REGION_SIZE 0X100 </blockquote><blockquote type="cite">+#define DMPF_REGION_SIZE 0x100 </blockquote><blockquote type="cite">+#define DMPF_RAID10_NEAR_COPIES 0x200 </blockquote><blockquote type="cite">+#define DMPF_RAID10_FAR_COPIES 0x400 </blockquote><blockquote type="cite">+#define DMPF_RAID10_FAR_OFFSET 0x800 </blockquote><blockquote type="cite">+ </blockquote><blockquote type="cite">struct raid_set { </blockquote><blockquote type="cite"> struct dm_target *ti; </blockquote><blockquote type="cite"> </blockquote><blockquote type="cite">@@ -66,6 +71,15 @@ struct raid_set { </blockquote><blockquote type="cite"> struct raid_dev dev[0]; </blockquote><blockquote type="cite">}; </blockquote><blockquote type="cite"> </blockquote><blockquote type="cite">+/* near_copies in first byte */ </blockquote><blockquote type="cite">+/* far_copies in second byte */ </blockquote><blockquote type="cite">+/* far_offset in 17th bit */ </blockquote><blockquote type="cite">+#define ALGORITHM_RAID10(near_copies, far_copies, far_offset) \ </blockquote><blockquote type="cite">+ ((near_copies & 0xFF) | ((far_copies & 0xFF) << 8) | ((!!far_offset) << 16)) </blockquote><blockquote type="cite">+#define RAID10_NC(layout) (layout & 0xFF) </blockquote><blockquote type="cite">+#define RAID10_FC(layout) ((layout >> 8) & 0xFF) </blockquote><blockquote type="cite">+#define RAID10_FO(layout) (layout & 0x10000) </blockquote><blockquote type="cite">+ </blockquote><blockquote type="cite">/* Supported raid types and properties. */ </blockquote><blockquote type="cite">static struct raid_type { </blockquote><blockquote type="cite"> const char *name; /* RAID algorithm. */ </blockquote><blockquote type="cite">@@ -76,6 +90,8 @@ static struct raid_type { </blockquote><blockquote type="cite"> const unsigned algorithm; /* RAID algorithm. */ </blockquote><blockquote type="cite">} raid_types[] = { </blockquote><blockquote type="cite"> {"raid1", "RAID1 (mirroring)", 0, 2, 1, 0 /* NONE */}, </blockquote><blockquote type="cite">+ {"raid10", "RAID10 (striped mirrors)", 0, 2, 10, -1 /* Varies */}, </blockquote><blockquote type="cite">+ {"raid1e", "RAID1E (Enhanced RAID1)", 0, 2, 10, -1 /* Varies */}, </blockquote><blockquote type="cite"> {"raid4", "RAID4 (dedicated parity disk)", 1, 2, 5, ALGORITHM_PARITY_0}, </blockquote><blockquote type="cite"> {"raid5_la", "RAID5 (left asymmetric)", 1, 2, 5, ALGORITHM_LEFT_ASYMMETRIC}, </blockquote><blockquote type="cite"> {"raid5_ra", "RAID5 (right asymmetric)", 1, 2, 5, ALGORITHM_RIGHT_ASYMMETRIC}, </blockquote><blockquote type="cite">@@ -339,10 +355,17 @@ static int validate_region_size(struct r </blockquote><blockquote type="cite"> * [max_write_behind <sectors>] See '-write-behind=' (man mdadm) </blockquote><blockquote type="cite"> * [stripe_cache <sectors>] Stripe cache size for higher RAIDs </blockquote><blockquote type="cite"> * [region_size <sectors>] Defines granularity of bitmap </blockquote><blockquote type="cite">+ * </blockquote><blockquote type="cite">+ * RAID10-only options: </blockquote><blockquote type="cite">+ * [raid10_near_copies <# copies>] Near copies. (Default: 2) </blockquote><blockquote type="cite">+ * [raid10_far_copies <# copies>] Far copies. (Default: 1) </blockquote><blockquote type="cite">+ * [raid10_far_offset <0/1>] Offset is device size(0) or stripe(1). </blockquote> Can I suggest that you don't do it like this? i.e. don't copy the mistakes I made :-) I don't think there is any value in supporting multiple near and far copies. There are two dimensions of the layout: - number of copies. Defaults to 2 - location of the copies: near, far, offset Some day I could implement an alternate version of 'far' or 'offset' which improves redundancy slightly. Instead of A B C D ... D A B C it would be A B C D .... B A D C i.e. treat the devices as pair and swap the device for the second copy. This doesn't generalise to an odd number of devices, but increases the number of pairs of devices that can concurrently fail without losing date. (for 4 devices, there are 6 pairs. With current 'far' mode there are only 2 pair of devices that can concurrently fail (0,2 and 1,3). With the proposed far mode there are 4 (0,2 0,3 1,2, 1,3). Adding this with your current proposal would be messy. Adding it with the two dimensions I suggest would simply involve adding another 'location' - 'farswap' or 'far2' or something. I note you didn't make 'dm-raid1e' a module alias. Was that deliberate? </blockquote></div> <div>The missed module alias was an oversight, thanks for catching that. (Similar - as you can see from this patch - to the oversight I had when introducing raid1. :) I have gone back and forth on whether to include the alternate "raid1e" or not. There are different RAID1E algorithms, as described in the kernel doc. Perhaps there should also be aliases similar to raid5 and raid6 - like raid1e_as (i.e. RAID1E - Adjacent Stripe), etc. However, there are several combinations that don't have a real name. Any thoughts? I would be just fine leaving "raid1e" out as I would leaving it in. These days, RAID10 is really a subset of RAID1E - perhaps I should be considering taking "raid10" out. ;)</div><div> </div><div>I like your suggestion of changing the parameter names. I've found the original names somewhat confusing. ('far_offset' seems to imply to me that the copy would not be the very next stripe, but _offset_ somehow - it seems to have the reverse meaning to me. I think this comes from the fact that it acts as a modifier to 'far_copy'.) I toyed with a couple different ways of doing this but figured it was best to just go along. Anyway, what you are suggesting seems to be:</div><div> raid10_copies <number> (Default: 2)</div><div> raid10_layout <string> (Default: "near"/"adjacent")</div><div>Where <string> could be "near", "far", "offset" and "some-future-thing". That seems nice to me and seems to clear up some of the confusion caused by "far_offset" seeming to be a modifier to "far_copies".</div><div> </div><div>Let me know what you think about the above, and I'll get v2 ready.</div><div> </div><div> brassow</div><div> </div><div>P.S. While it doesn't affect this singular patch, I see people posting their set of patches as a reply to the summary '0 of' patch. This keeps things together better, and I'll assume this is the way I should post from now on.</div></body></html>