[PATCH 0/6] md: Remove the hash tables from raid0.

[PATCH 0/6] md: Remove the hash tables from raid0.

[Andre Noll]

As mentioned by Neil, the raid0 hash table code does probably not
add any value. Moreover, it contains some rather strange sector_t
manipulations which are needed to setup and maintain the table.

This patch series against Neil's for-next tree as of yesterday removes
the hash table from the raid0 code.

Patch #1 replaces the hash table lookup by a simple function that
loops over all strip zones to find the zone that holds a given sector.
This change allows to get rid of the hash table itself (patch #2)
and of related fields of struct raid0_private_data (patch #3).

Patch #4 makes raid0 return a proper error code rather than -ENOMEM
in case the array could not be started for reasons other than memory
shortage.

The remaining two patches are simple cleanups that further simplify
the raid0 code a bit.

The patched kernel has been tested with a smallish raid0 array
consisting of five devices of varying sizes (created and filled with
contents by an unpatched kernel) and seems to work just fine. It
passes the raid0 tests of the mdadm test suite.

Please consider for inclusion.

 drivers/md/raid0.c |  150 ++++++++++++++--------------------------------------
 drivers/md/raid0.h |    4 --
 2 files changed, 39 insertions(+), 115 deletions(-)

Thanks
Andre

[PATCH 1/6] md: raid0: Replace hash table lookup by looping over all strip_zones.

[Andre Noll]

The number of strip_zones of a raid0 array is bounded by the number of
drives in the array and is in fact much smaller for typical setups. For
example, any raid0 array containing identical disks will have only
a single strip_zone.

Therefore, the hash tables which are used for quickly finding the
strip_zone that holds a particular sector are of questionable value
and add quite a bit of unnecessary complexity.

This patch replaces the hash table lookup by equivalent code which
simply loops over all strip zones to find the zone that holds the
given sector.

Subsequent cleanup patches will remove the hash table structure.

Signed-off-by: Andre Noll <maan@systemlinux.org>

 drivers/md/raid0.c |   32 +++++++++++++++++++-------------
 1 files changed, 19 insertions(+), 13 deletions(-)
-- 
1.5.4.3

[PATCH 2/6] md: raid0: Remove hash table.

[Andre Noll]

The raid0 hash table has become unused due to the changes in the
previous patch. This patch removes the hash table allocation and
setup code and kills the hash_table field of struct raid0_private_data.

Signed-off-by: Andre Noll <maan@systemlinux.org>

 drivers/md/raid0.c |   12 ------------
 drivers/md/raid0.h |    1 -
 2 files changed, 0 insertions(+), 13 deletions(-)
-- 
1.5.4.3

[PATCH 3/6] md: raid0: Remove hash spacing and sector shift.

[Andre Noll]

The "sector_shift" and "spacing" fields of struct raid0_private_data
were only used for the hash table lookups. So the removal of the
hash table allows get rid of these fields as well which simplifies
create_strip_zones() and raid0_run() quite a bit.

Signed-off-by: Andre Noll <maan@systemlinux.org>

 drivers/md/raid0.c |   63 +---------------------------------------------------
 drivers/md/raid0.h |    3 --
 2 files changed, 1 insertions(+), 65 deletions(-)
-- 
1.5.4.3

[PATCH 4/6] md: raid0: Make raid0_run() return a proper error code.

[Andre Noll]

Currently raid0_run() always returns -ENOMEM on errors. This is
incorrect as running the array might fail for other reasons, for
example because not all component devices were available.

This patch changes create_strip_zones() so that it returns a proper
error code (either -ENOMEM or -EINVAL) rather than 1 on errors and
makes raid0_run(), its single caller, return that value instead
of -ENOMEM.

Signed-off-by: Andre Noll <maan@systemlinux.org>

 drivers/md/raid0.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)
-- 
1.5.4.3

[PATCH 5/6] md: raid0: Kfree() strip_zone and devlist in create_strip_zones().

[Andre Noll]

It's cleaner to kfree() these in the same function where they are
allocated. This allows to simplify raid0_run() a bit which will be
done in the next patch.

Signed-off-by: Andre Noll <maan@systemlinux.org>

 drivers/md/raid0.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
-- 
1.5.4.3

[PATCH 6/6] md: raid0: Simplify raid0_run().

[Andre Noll]

Get rid of the local variable "conf" and of an unnecessary cast.

Signed-off-by: Andre Noll <maan@systemlinux.org>

 drivers/md/raid0.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)
-- 
1.5.4.3

Re: [PATCH 0/6] md: Remove the hash tables from raid0.

[raz ben yehuda]

where are the patches ?

On Thu, 2009-05-14 at 11:30 +0200, Andre Noll wrote:
> As mentioned by Neil, the raid0 hash table code does probably not
> add any value. Moreover, it contains some rather strange sector_t
> manipulations which are needed to setup and maintain the table.
> 
> This patch series against Neil's for-next tree as of yesterday removes
> the hash table from the raid0 code.
> 
> Patch #1 replaces the hash table lookup by a simple function that
> loops over all strip zones to find the zone that holds a given sector.
> This change allows to get rid of the hash table itself (patch #2)
> and of related fields of struct raid0_private_data (patch #3).
> 
> Patch #4 makes raid0 return a proper error code rather than -ENOMEM
> in case the array could not be started for reasons other than memory
> shortage.
> 
> The remaining two patches are simple cleanups that further simplify
> the raid0 code a bit.
> 
> The patched kernel has been tested with a smallish raid0 array
> consisting of five devices of varying sizes (created and filled with
> contents by an unpatched kernel) and seems to work just fine. It
> passes the raid0 tests of the mdadm test suite.
> 
> Please consider for inclusion.
> 
>  drivers/md/raid0.c |  150 ++++++++++++++--------------------------------------
>  drivers/md/raid0.h |    4 --
>  2 files changed, 39 insertions(+), 115 deletions(-)
> 
> Thanks
> Andre

[PATCH 3/6] md: raid0: Remove hash spacing and sector shift.

[Andre Noll]

The "sector_shift" and "spacing" fields of struct raid0_private_data
were only used for the hash table lookups. So the removal of the
hash table allows get rid of these fields as well which simplifies
create_strip_zones() and raid0_run() quite a bit.

Signed-off-by: Andre Noll <maan@systemlinux.org>
---
 drivers/md/raid0.c |   63 +---------------------------------------------------
 drivers/md/raid0.h |    3 --
 2 files changed, 1 insertions(+), 65 deletions(-)

diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index d550396..4b207ab 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -56,7 +56,6 @@ static int create_strip_zones (mddev_t *mddev)
 {
 	int i, c, j;
 	sector_t current_start, curr_zone_start;
-	sector_t min_spacing;
 	raid0_conf_t *conf = mddev_to_conf(mddev);
 	mdk_rdev_t *smallest, *rdev1, *rdev2, *rdev;
 	struct strip_zone *zone;
@@ -204,28 +203,7 @@ static int create_strip_zones (mddev_t *mddev)
 		printk(KERN_INFO "raid0: current zone start: %llu\n",
 			(unsigned long long)current_start);
 	}
-	/* Now find appropriate hash spacing.
-	 * We want a number which causes most hash entries to cover
-	 * at most two strips, but the hash table must be at most
-	 * 1 PAGE.  We choose the smallest strip, or contiguous collection
-	 * of strips, that has big enough size.  We never consider the last
-	 * strip though as it's size has no bearing on the efficacy of the hash
-	 * table.
-	 */
-	conf->spacing = curr_zone_start;
-	min_spacing = curr_zone_start;
-	sector_div(min_spacing, PAGE_SIZE/sizeof(struct strip_zone*));
-	for (i=0; i < conf->nr_strip_zones-1; i++) {
-		sector_t s = 0;
-		for (j = i; j < conf->nr_strip_zones - 1 &&
-				s < min_spacing; j++)
-			s += conf->strip_zone[j].sectors;
-		if (s >= min_spacing && s < conf->spacing)
-			conf->spacing = s;
-	}
-
 	mddev->queue->unplug_fn = raid0_unplug;
-
 	mddev->queue->backing_dev_info.congested_fn = raid0_congested;
 	mddev->queue->backing_dev_info.congested_data = mddev;
 
@@ -275,10 +253,8 @@ static sector_t raid0_size(mddev_t *mddev, sector_t sectors, int raid_disks)
 	return array_sectors;
 }
 
-static int raid0_run (mddev_t *mddev)
+static int raid0_run(mddev_t *mddev)
 {
-	unsigned  cur=0, i=0, nb_zone;
-	s64 sectors;
 	raid0_conf_t *conf;
 
 	if (mddev->chunk_size == 0) {
@@ -308,43 +284,6 @@ static int raid0_run (mddev_t *mddev)
 
 	printk(KERN_INFO "raid0 : md_size is %llu sectors.\n",
 		(unsigned long long)mddev->array_sectors);
-	printk(KERN_INFO "raid0 : conf->spacing is %llu sectors.\n",
-		(unsigned long long)conf->spacing);
-	{
-		sector_t s = raid0_size(mddev, 0, 0);
-		sector_t space = conf->spacing;
-		int round;
-		conf->sector_shift = 0;
-		if (sizeof(sector_t) > sizeof(u32)) {
-			/*shift down space and s so that sector_div will work */
-			while (space > (sector_t) (~(u32)0)) {
-				s >>= 1;
-				space >>= 1;
-				s += 1; /* force round-up */
-				conf->sector_shift++;
-			}
-		}
-		round = sector_div(s, (u32)space) ? 1 : 0;
-		nb_zone = s + round;
-	}
-	printk(KERN_INFO "raid0 : nb_zone is %d.\n", nb_zone);
-	sectors = conf->strip_zone[cur].sectors;
-
-	for (i=1; i< nb_zone; i++) {
-		while (sectors <= conf->spacing) {
-			cur++;
-			sectors += conf->strip_zone[cur].sectors;
-		}
-		sectors -= conf->spacing;
-	}
-	if (conf->sector_shift) {
-		conf->spacing >>= conf->sector_shift;
-		/* round spacing up so when we divide by it, we
-		 * err on the side of too-low, which is safest
-		 */
-		conf->spacing++;
-	}
-
 	/* calculate the max read-ahead size.
 	 * For read-ahead of large files to be effective, we need to
 	 * readahead at least twice a whole stripe. i.e. number of devices
diff --git a/drivers/md/raid0.h b/drivers/md/raid0.h
index a14630a..dbcf1da 100644
--- a/drivers/md/raid0.h
+++ b/drivers/md/raid0.h
@@ -15,9 +15,6 @@ struct raid0_private_data
 	struct strip_zone *strip_zone;
 	mdk_rdev_t **devlist; /* lists of rdevs, pointed to by strip_zone->dev */
 	int nr_strip_zones;
-
-	sector_t spacing;
-	int sector_shift; /* shift this before divide by spacing */
 };
 
 typedef struct raid0_private_data raid0_conf_t;
-- 
1.5.4.3