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

[Andre Noll <maan@systemlinux.org>]

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

On 21:15, NeilBrown wrote:
> > +/* 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;
> > +	}
> 
> Maybe I'm being petty, but I really don't like this...
> 
> I really like
> > -        while (sector >= zone->zone_start + zone->sectors)
> > -                zone++;
> 
> It seems to capture what is really happening.
> But I can see that your code has a defensive aspect to it
> which is hard to argue against.
> 
> It's probably the fact that there are two loop variables (i and z)
> that bothers me....
> Here is a thought.  How about we extend the stripe_zone array
> by one element and put a sentinal at the end, with ->zone_start being
> the size of the drive ... or maybe even "max sector".
> 
> Then the loop could be
>    for (i = 0; i < conf->nr_strip_zones; i++)
>          if (sector < conf->strip_zone[i+1])
>                return conf->strip_zone[i]
> 
> Save our selves an 'add' that way :-)

Saving an addition is a good thing as find_zone() is certainly
performance-critical. After all, that's why the original author used
hash tables.

The drawback is of course that we will happily return the last zone
for (invalid) requests beyond the array size. If that happens, data
corruption on the drive(s) of the last zone will likely be the result,
so I feel a bit uncomfortable in doing this.

How about extending stripe_zone as you propose, storing the array
size in ->zone_start of the last element and still call BUG() after
the loop?  That still saves the additional add and avoids the mentioned
data corruption.

> > +	zone = find_zone(conf, sector);
> > +	if (!zone)
> > +		return 1;
> 
> I don't much like those last two lines either.  zone will
> never be NULL because if find_zone doesn't find anything, it
> calls BUG.  And returning from make_request without doing any IO
> is just wrong.  So those two lines can go.

Yup. Do you want me to rework the patches, or should I follow up with
patches on top of the series?

Thanks
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 --]