Re: PATCH md [001:002]: raid0: fix chunk size to 4K*n granularity

[Andre Noll <maan@systemlinux.org>]

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

On 18:58, raz ben yehuda wrote:

>  Andre, Patch is applied on top of your last post. now it is your turn to merge :)

I'll have to rework my patch series and will hopefully send the new
series tomorrow.

If it's clear that moving raid0 to 4K granularity is good thing to do,
I'll add your patches on top of the new series.

>  static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
>  {
>  	sector_t num_sectors = rdev->sb_start;
> -
> -	if (chunk_size)
> -		num_sectors &= ~((sector_t)chunk_size/512 - 1);
> +	if (chunk_size) {
> +		int chunk_sects = chunk_size>>9;

As chunk_size is unsigned, chunk_sects should probably also be unsigned.

> +		num_sectors = (num_sectors/chunk_sects)*chunk_sects;

A ROUND_DOWN macro perhaps? There are only four of them in the kernel
tree :-/

> @@ -3512,7 +3514,7 @@ min_sync_store(mddev_t *mddev, const char *buf, size_t len)
>  
>  	/* Must be a multiple of chunk_size */
>  	if (mddev->chunk_size) {
> -		if (min & (sector_t)((mddev->chunk_size>>9)-1))
> +		if (min % (sector_t)(mddev->chunk_size>>9))
>  			return -EINVAL;
>  	}
>  	mddev->resync_min = min;
> @@ -3549,7 +3551,7 @@ max_sync_store(mddev_t *mddev, const char *buf, size_t len)
>  
>  		/* Must be a multiple of chunk_size */
>  		if (mddev->chunk_size) {
> -			if (max & (sector_t)((mddev->chunk_size>>9)-1))
> +			if (max % (sector_t)((mddev->chunk_size>>9)))

If we decide to clean these up, we might as well do it right and
use ffz()/ffs().

> +		/*
> +		* raid0 chunk size has to divide by a page
> +		*/

Nitpick: I'd prefer to say "must be a multiple of", and the indentation
does not match coding style standards.

> +static int raid0_is_power2_chunk(mddev_t *mddev)
> +{
> +	if ((1 << ffz(~mddev->chunk_size)) == mddev->chunk_size)
> +		return 1;
> +	return 0;
> +}

This could be go to md.h as an inline function or at least be used
everywhere in raid0.c.

> -/* Find the zone which holds a particular offset */
> -static struct strip_zone *find_zone(struct raid0_private_data *conf,
> -		sector_t sector)
> +static int raid0_position_bio(mddev_t *mddev, struct bio *bio, sector_t sector)
>  {
> -	int i;
> -
> -	for (i = 0; i < conf->nr_strip_zones; i++) {
> -		struct strip_zone *z = conf->strip_zone + i;
> -
> -		if (sector < z->zone_start + z->sectors)
> -			return z;
> -	}
> -	BUG();
> -	return NULL;
> +	sector_t sect_in_chunk;
> +	mdk_rdev_t *tmp_dev;
> +	sector_t chunk_in_dev;
> +	sector_t rsect;
> +	sector_t x;
> +	raid0_conf_t *conf = mddev_to_conf(mddev);
> +	sector_t chunk_sects = mddev->chunk_size >> 9;
> +	struct strip_zone *zone = &conf->strip_zone[0];
> +
> +	while (sector >= zone->zone_start + zone->sectors)
> +		zone++;
> +	sect_in_chunk = sector % chunk_sects;
> +	x = (sector - zone->zone_start) / chunk_sects;
> +	sector_div(x, zone->nb_dev);
> +	chunk_in_dev = x;
> +	x = sector / chunk_sects;
> +	tmp_dev = zone->dev[sector_div(x, zone->nb_dev)];
> +	rsect = (chunk_in_dev * chunk_sects) + zone->dev_start + sect_in_chunk;
> +	bio->bi_bdev = tmp_dev->bdev;
> +	bio->bi_sector = rsect + tmp_dev->data_offset;
> +	return 0;
>  }

This clashes of course with the planned changes that extend stripe_zone.

I'll see what I can do.
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]