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

SandeepKsinha
2009-05-14T11:15:40Z

Hi Andre,

On Thu, May 14, 2009 at 4:13 PM, Andre Noll <maan@systemlinux.org> wrote:
> 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(-)
>
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index c08d755..9fd3c3c 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -398,6 +398,22 @@ static int raid0_stop (mddev_t *mddev)
>        return 0;
>  }
>
> +/* Find the zone which holds a particular offset */
> +static struct strip_zone *find_zone(struct raid0_private_data *conf,
> +               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;
> +}
> +
>  static int raid0_make_request (struct request_queue *q, struct bio *bio)
>  {
>        mddev_t *mddev = q->queuedata;
> @@ -443,20 +459,10 @@ static int raid0_make_request (struct request_queue *q, struct bio *bio)
>                bio_pair_release(bp);
>                return 0;
>        }
> -
> -
> -       {
> -               sector_t x = sector >> conf->sector_shift;
> -               sector_div(x, (u32)conf->spacing);
> -               zone = conf->hash_table[x];
> -       }
> -
> -       while (sector >= zone->zone_start + zone->sectors)
> -               zone++;
> -
> +       zone = find_zone(conf, sector);
> +       if (!zone)
> +               return 1;

Firstly, will find_zone ever return a NULL. You code already asserts
on that situation in find_zone.

But in my opinion,

You should allow NULL to be returned from find_zone( ),
handle NULL in the caller function more gracefully than panicking (BUG).

Also, the above lines mean that if zone==NULL, you return 1, which
will eventually submit your request through generic_make_request.

I might be wrong as I am quite new to this code.
Kindly forgive.
>        sect_in_chunk = bio->bi_sector & (chunk_sects - 1);
> -
> -
>        {
>                sector_t x = (sector - zone->zone_start) >> chunksect_bits;
>
> --
> 1.5.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
Regards,
Sandeep.

“To learn is to change. Education is a process that changes the learner.”
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html