Re: [PATCH v4] DM RAID: Add support for MD RAID10

[NeilBrown <neilb@suse.de>]

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

On Mon, 23 Jul 2012 14:49:22 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> Neil,
> 
> Updated 'sectors_per_dev' calculation and integrated your other
> suggestions.

Mostly ...

> 
>  brassow
> 
> dm raid: add md raid10 support
> 
> Support the MD RAID10 personality through dm-raid.c
> 
> Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>
> 
> Index: linux-upstream/drivers/md/dm-raid.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/dm-raid.c
> +++ linux-upstream/drivers/md/dm-raid.c
> @@ -11,6 +11,7 @@
>  #include "md.h"
>  #include "raid1.h"
>  #include "raid5.h"
> +#include "raid10.h"
>  #include "bitmap.h"
>  
>  #include <linux/device-mapper.h>
> @@ -52,7 +53,10 @@ struct raid_dev {
>  #define DMPF_MAX_RECOVERY_RATE 0x20
>  #define DMPF_MAX_WRITE_BEHIND  0x40
>  #define DMPF_STRIPE_CACHE      0x80
> -#define DMPF_REGION_SIZE       0X100
> +#define DMPF_REGION_SIZE       0x100
> +#define DMPF_RAID10_COPIES     0x200
> +#define DMPF_RAID10_FORMAT     0x400
> +
>  struct raid_set {
>  	struct dm_target *ti;
>  
> @@ -76,6 +80,7 @@ static struct raid_type {
>  	const unsigned algorithm;	/* RAID algorithm. */
>  } raid_types[] = {
>  	{"raid1",    "RAID1 (mirroring)",               0, 2, 1, 0 /* NONE */},
> +	{"raid10",   "RAID10 (striped mirrors)",        0, 2, 10, UINT_MAX /* Varies */},
>  	{"raid4",    "RAID4 (dedicated parity disk)",	1, 2, 5, ALGORITHM_PARITY_0},
>  	{"raid5_la", "RAID5 (left asymmetric)",		1, 2, 5, ALGORITHM_LEFT_ASYMMETRIC},
>  	{"raid5_ra", "RAID5 (right asymmetric)",	1, 2, 5, ALGORITHM_RIGHT_ASYMMETRIC},
> @@ -86,6 +91,17 @@ static struct raid_type {
>  	{"raid6_nc", "RAID6 (N continue)",		2, 4, 6, ALGORITHM_ROTATING_N_CONTINUE}
>  };
>  
> +static unsigned raid10_md_layout_to_copies(int layout)
> +{
> +	return layout & 0xFF;
> +}
> +
> +static int raid10_format_to_md_layout(char *format, unsigned copies)
> +{
> +	/* 1 "far" copy, and 'copies' "near" copies */
> +	return (1 << 8) | (copies & 0xFF);
> +}
> +
>  static struct raid_type *get_raid_type(char *name)
>  {
>  	int i;
> @@ -339,10 +355,16 @@ static int validate_region_size(struct r
>   *    [max_write_behind <sectors>]	See '-write-behind=' (man mdadm)
>   *    [stripe_cache <sectors>]		Stripe cache size for higher RAIDs
>   *    [region_size <sectors>]           Defines granularity of bitmap
> + *
> + * RAID10-only options:
> + *    [raid10_copies <# copies>]        Number of copies.  (Default: 2)
> + *    [raid10_format <near>]            Layout algorithm.  (Default: near)
>   */
>  static int parse_raid_params(struct raid_set *rs, char **argv,
>  			     unsigned num_raid_params)
>  {
> +	char *raid10_format = "near";
> +	unsigned raid10_copies = 2;
>  	unsigned i, rebuild_cnt = 0;
>  	unsigned long value, region_size = 0;
>  	sector_t sectors_per_dev = rs->ti->len;
> @@ -416,11 +438,28 @@ static int parse_raid_params(struct raid
>  		}
>  
>  		key = argv[i++];
> +
> +		/* Parameters that take a string value are checked here. */
> +		if (!strcasecmp(key, "raid10_format")) {
> +			if (rs->raid_type->level != 10) {
> +				rs->ti->error = "'raid10_format' is an invalid parameter for this RAID type";
> +				return -EINVAL;
> +			}
> +			if (strcmp("near", argv[i])) {
> +				rs->ti->error = "Invalid 'raid10_format' value given";
> +				return -EINVAL;
> +			}
> +			raid10_format = argv[i];
> +			rs->print_flags |= DMPF_RAID10_FORMAT;
> +			continue;
> +		}
> +
>  		if (strict_strtoul(argv[i], 10, &value) < 0) {
>  			rs->ti->error = "Bad numerical argument given in raid params";
>  			return -EINVAL;
>  		}
>  
> +		/* Parameters that take a numeric value are checked here */
>  		if (!strcasecmp(key, "rebuild")) {
>  			rebuild_cnt++;
>  			rs->ti->error = NULL;
> @@ -436,6 +475,7 @@ static int parse_raid_params(struct raid
>  				if (rebuild_cnt > rs->raid_type->parity_devs)
>  					rs->ti->error = "Too many rebuild devices specified for given RAID type";
>  				break;
> +			case 10:
>  			default:
>  				DMERR("The rebuild parameter is not supported for %s", rs->raid_type->name);
>  				rs->ti->error = "Rebuild not supported for this RAID type";

This hunk doesn't apply for me, or against 3.5.  Is there some patch I'm
missing.
I do vaguely recall you changing this to a switch statement I think, but I
still have an if statement here.
If I'm missing a patch - could you resend it please?



>> @@ -536,8 +585,30 @@ static int parse_raid_params(struct raid
>  	if (dm_set_target_max_io_len(rs->ti, max_io_len))
>  		return -EINVAL;
>  
> -	if ((rs->raid_type->level > 1) &&
> -	    sector_div(sectors_per_dev, (rs->md.raid_disks - rs->raid_type->parity_devs))) {
> +	if (rs->raid_type->level == 10) {
> +		if (raid10_copies > rs->md.raid_disks) {
> +			rs->ti->error = "Not enough devices to satisfy specification";
> +			return -EINVAL;
> +		}
> +
> +		/* (Len * #mirrors) / #devices */
> +		sectors_per_dev = rs->ti->len * raid10_copies;
> +		if (sector_div(sectors_per_dev, rs->md.raid_disks)) {
> +			rs->ti->error = "Target length not evenly divisible by number of stripes";
> +			return -EINVAL;
> +		}

This test is still completely pointless, and putting an extra test for chunk
alignment after it doesn't make it any less pointless. 
And putting an important division inside the condition of an if(), hides it a
bit more than I like.
But it probably isn't worth arguing about it any more so once I can get a
patch to apply I'll take it.

Thanks,
NeilBrown


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