Re: [PATCH 3 of 4] MD RAID10: Improve redundancy for 'far' and 'offset' algorithms (part 2)

[NeilBrown <neilb@suse.de>]

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


Hi Jon,
 I really like the way you have coded this - it makes for very neat code.

 I'm happy to take  it as it is, however I'll just note a couple of little
 issues that you could change if you like:

> --- linux-upstream.orig/drivers/md/raid10.c
> +++ linux-upstream/drivers/md/raid10.c
> @@ -550,6 +550,13 @@ static void __raid10_find_phys(struct ge
>  	sector_t stripe;
>  	int dev;
>  	int slot = 0;
> +	int last_far_set_start, last_far_set_size;
> +
> +	last_far_set_start = (geo->raid_disks / geo->far_set_size) - 1;
> +	last_far_set_start *= geo->far_set_size;
> +
> +	last_far_set_size = geo->far_set_size;
> +	last_far_set_size += (geo->raid_disks % geo->far_set_size);
>  
>  	/* now calculate first sector/dev */
>  	chunk = r10bio->sector >> geo->chunk_shift;
> @@ -575,9 +582,16 @@ static void __raid10_find_phys(struct ge
>  		for (f = 1; f < geo->far_copies; f++) {
>  			set = d / geo->far_set_size;
>  			d += geo->near_copies;
> -			d %= geo->far_set_size;
> -			d += geo->far_set_size * set;
>  
> +			if ((geo->raid_disks % geo->far_set_size) &&
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This test isn't needed.  If it is false, then last_far_set_size will be the
same as geo->far_set_size, so both branches will do the same thing.


> +			    (d > last_far_set_start)) {
> +				d -= last_far_set_start;
> +				d %= last_far_set_size;
> +				d += last_far_set_start;
> +			} else {
> +				d %= geo->far_set_size;
> +				d += geo->far_set_size * set;
> +			}
>  			s += geo->stride;
>  			r10bio->devs[slot].devnum = d;
>  			r10bio->devs[slot].addr = s;
> @@ -615,6 +629,18 @@ static sector_t raid10_find_virt(struct
>  	struct geom *geo = &conf->geo;
>  	int far_set_start = (dev / geo->far_set_size) * geo->far_set_size;
>  	int far_set_size = geo->far_set_size;
> +	int last_far_set_start;
> +
> +	if (geo->raid_disks % geo->far_set_size) {

Similarly I don't think this test is needed - just perform the following code
in any case.
Maybe you will think the code is more clear the way it is.  If so, I'm fine
to leave it as it is.

Thanks,
NeilBrown



> +		last_far_set_start = (geo->raid_disks / geo->far_set_size) - 1;
> +		last_far_set_start *= geo->far_set_size;
> +
> +		if (dev >= last_far_set_start) {
> +			far_set_size = geo->far_set_size;
> +			far_set_size += (geo->raid_disks % geo->far_set_size);
> +			far_set_start = last_far_set_start;
> +		}
> +	}
>  
>  	offset = sector & geo->chunk_mask;
>  	if (geo->far_offset) {
> 


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