Re: [PATCH v3 1/4] lib/string_helpers: change blk_size to u32 for string_get_size() interface

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Thu, Oct 29 2015, James Bottomley <jbottomley@odin.com> wrote:

> On Thu, 2015-10-29 at 17:30 +0100, Vitaly Kuznetsov wrote:
>> string_get_size() can't really handle huge block sizes, especially
>> blk_size > U32_MAX but string_get_size() interface states the opposite.
>> Change blk_size from u64 to u32 to reflect the reality.
>
> What is the actual evidence for this?  The calculation is designed to be
> a symmetric 128 bit multiply.  When I wrote and tested it, it worked
> fine for huge block sizes.
>

May I politely ask how you tested it, and what you mean by "worked"? The
bug I reported last week was particularly concerning block sizes >= 1024
(e.g. the 32768, 1024 pair giving 32.7 MB where the correct output would
be 33.5 MB). Now it turns out that it was actually broken for smaller
block sizes as well. For ~13000 semirandom size,blk_size pairs, the
current code produces the wrong result in ~2100 cases. The new code
reduces that to 122 cases, all of which are off by one in the last
digit.

And I don't buy the symmetry argument either. Mathematically, it should
give the same, but your algorithm produces 2.04 MB for 512,4096 and 2.09
MB for 4096,512.

Maybe the commit message could be better, but I think it makes a lot of
sense to make blk_size u32. Breaking the symmetry between size and
blk_size is good (less likely that the arguments get swapped). It
allows a simpler implementation. It makes the generated code
smaller.

Rasmus