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

[James Bottomley]

On Fri, 2015-10-30 at 00:19 +0100, Rasmus Villemoes wrote:
> On Thu, Oct 29 2015, James Bottomley <jbottomley@xxxxxxxx> 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).

The test was basically a userspace version reversing the large size
smaller block size numbers and verifying they produce the same output.

>  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.

I wasn't making the point that there isn't a potential off by a couple
of percent problem in the algorithm I was making the point that it
should work as a multiplier of two u64 numbers, so I can't understand
the rational basis for reducing the block size to u32.

> 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.

That's an off by 2.5%; it means there's a slight error in one of the
carries it doesn't mean there's a fundamental problem in the algorithm.

> 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.

The drive vendors are already pushing huge block size systems for ZBC.
They're already talking about 2GB sectors, which is 31 bits ... they'll
be over the 32 bit limit fairly shortly, I predict, so it makes no sense
to have to have the storage layer do silly bit shifting because we were
short sighted enough to cap block size to a u32.

James

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå