On Tue, Sep 26, 2006 at 01:37:00PM -0400, Theodore Tso wrote:
> On Tue, Sep 26, 2006 at 04:48:32PM +0200, Alexandre Ratchov wrote:
> > convert all 32bit on-disk block number definitions (currently __u32,
> > blk_t, unsigned long, unsigned int...) to pblk_t that is defined as
> > __u32. In this way we are sure that blk_t is used only in memory.
> > Later, this would allow to make blk_t 64bit without disturbing any
> > programs (this would just eat more momory and use 64bit arithmetic).
>
> I *really* dislike this approach, because it makes it very hard to
> prove that we got all of the conversions right --- any mistakes about
> which instances of blk_t need to become pblk_t, and which are supposed
> to stay blk_t, and we end up breaking our ABI compatibility. And
> let's just say I have higher standards than Greg KH has shown with
> udev. :-)
>
> I also don't like pblk_t, because it's not at all obvious what it
> means.
It just means "__u32". It's used in very very few places. Mainly to access
on-disk indirect blocks, typically as follows:
char *ind_block_buf;
pblk_t *pblk;
blk_t blk;
...
pblk = (pblk_t *)ind_block_buf;
for (i = 0, ; i < N; i++) {
blk = *pblk;
...
pblk++;
}
So, the only purpose on the typedef is to "tag" 32bit variables that are
used for such purposes and that have to stay 32bit forever. I expect in the
long term to remove the typedef and to simply use __u32 in such code. For me
this patch is a "temporary stage" in order to avoid confusion between blk_t,
__u32, int, long, etc... and to do the 32bit -> 64bit convertion of block
numbers in a safe manner.
> And what if we want to support 64-bit logicial blocks someday?
> Then it's another painful exercise to figure out which blk_t get
> separated to lblk_t, etc.
>
i fully agree here; personnaly, i would like to see only one type for block
numbers: blk_t (or blk64_t if you prefer). And in some sense this patch
contributes to reduce the number of types we use for block numbers. Indeed,
it replaces most int, long, __u32 etc... by blk_t and pblk_t.
>
> So my plan is to introduce a new type, blk64_t, and create new
> interfaces, such as ext2fs_extent_iterate(), which will use the new
> type --- and which will work with old-style indirect block inodes as
> well (it will translate indirect blocks into extents).
i think we want the same thing, except that i've called blk_t what you call
blk64_t.
In order to get the blk64_t interface working with old-style indirect block
inodes, we have to use __u32 pointers to access indirect blocks and blk64_t
for the rest. If we want to reuse the existing code, we have to walk through
it distinguishing between __u32, long, int, blk_t, and replacing some of them by
blk64_t. That's very close to what this patch does.
Perhaps it's not obvious, but thats my approach to get all of the
conversions right without rewriting everything from scratch ;-). I'm still
open to other approches.
cheers,
-- Alexandre
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
