Re: [PATCH v1 0/8] raid6: support read-modify-write

[NeilBrown <neilb@suse.de>]

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

On Sun, 10 Aug 2014 11:56:23 +0000 <stockhausen@collogia.de> wrote:

> Hello all.
> 
> First of all thanks to an older patch from Kumar Sundararajan and 
> Dan Williams that helped me to understand RAID6 logic inside md 
> better. Everything is based on ideas & discussions that started
> with http://marc.info/?l=linux-raid&m=136624783417452&w=1
> 
> So once again. Implement RMW support for RAID6. This time improve
> syndrome calculation too. The logic was split into very small  
> parts. Each having a detailed explanation about what is going on.

Thanks a lot for these patches.  The size you split them in is good.
The order is ... not quite right.
It is important for each patch to make sense by itself and for everything to
work sensibly at every point.
Yet your first patch allows PARITY_PREFER_RMW to be set for RAID6 even though
RAID6 doesn't support it yet ... that doesn't make sense.

Also I don't like that fact that you added functions with empty definitions,
even if you later filled out some definitions.
I would prefer to not have the 'hasxor' field, but to expect 'xor_syndrome'
to be NULL for some methods.
Then the RAID6 code would only try RMW is xor_syndrome was non-NULL.
Then you just add a complete xor_syndrome function and it becomes usable.

So I'd probably have that patches:
 1/ add declarations for xor_syndrome and the code to
    report benchmarks on boo, but no actuall xor_syndrome code yet.
 2/ improve test program
 3/ xor_syndrome for generic_int
 4/ add logic for improved rmw_syndrome.
    This always uses rmw for RAID6 if xor_syndrome is defined
 5/ xor_syndrome for SSE2
 6/ config option

Maybe swap the order for 4 and 5.

I want the config option to be last because I don't want it to go upstream,
but everything before it should be able to.

The other thing that is missing from this are performance numbers.
Any numbers at all would be a good start.  If we can collect number from
different use case to get a good overview of the improvement (or not) this
brings, then we might have case for including it upstream.
I realise you know this and made it clear in your point 4.  I just want to
emphasis it again.  I might do some testing myself, but we really need
someone with a larger number of devices to tell us how much better this makes
things work for them.

Thanks for your effort so far.  If you can revise the patches so:
 - config option is last
 - 'hasxor' is not used
I'll put them all in my git tree to make it a little easier for people to
test, and probably do a little myself.

Thanks
NeilBrown

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