lkml.org 
[lkml]   [2006]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sector_t overflow in block layer
On May 18, 2006  15:41 -0700, Linus Torvalds wrote:
> On Thu, 18 May 2006, Anton Altaparmakov wrote:
> > I think you missed that Andreas said he is worried about 64-bit overflows
> > as well.
>
> Ahh. Ok. However, then the test _really_ should be something like
>
> sector_t maxsector, sector;
> int sector_shift = get_sector_shift(bh->b_size);
>
> maxsector = (~(sector_t)0) >> sector_shift;
> if (unshifted_value > maxsector)
> return -EIO;
> sector = (sector_t) unshifted_value << sector_shift;
>
> which is a lot clearer, and likely faster too, with a proper
> get_sector_shift.

I looked at that also, but it isn't clear from the use of "b_size" here
that there is any constraint that b_size is a power of two, only that it
is a multiple of 512. Granted, I don't know whether there are any users
of such a crazy thing, but the fact that this is in bytes instead of a
shift made me think twice.

> Something like this:
>
> /*
> * What it the shift required to turn a bh of size
> * "size" into a 512-byte sector count?
> */
> static inline int get_sector_shift(unsigned int size)
> {
> int shift = -1;
> unsigned int n = 256;
>
> do {
> shift++;
> } while ((n += n) < size);
> return shift;
> }
>
> which should generate good code on just about any architecture (it avoids
> actually using shifts on purpose), and I think the end result will end up
> being more readable (I'm not claiming that the "get_sector_shift()"
> implementation is readable, I'm claiming that the _users_ will be more
> readable).

This in fact exists virtually identically in blkdev.h::blksize_bits()
which I had a look at, but worried a bit about b_size != 2^n and also
the fact that this has branches and/or loop unwinding vs. the fixed
shift operations.

> Of course, even better would be to not have "b_size" at all, but use
> "b_shift", but we don't have that.

I was thinking exactly the same thing myself.

> And the sector shift calculation might be fast enough that it's even
> a win (turning a 64-bit multiply into a shift _tends_ to be better)

My thought was that the gratuitous 64-bit multiply in the 32-bit case
was offset by the fact that the comparison is easy. In the 64-bit case
we are already doing a 64-bit multiply so the goal is to make the
comparison as cheap as possible.

In the end, I don't really care about the exact mechanics of the check...

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-05-19 01:25    [W:0.055 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site