Re: [PATCH 2/9] sector_t format string

Mingming Cao
2006-08-11T00:59:09Z

On Wed, 2006-08-09 at 23:40 -0700, Andrew Morton wrote:
> On Wed, 09 Aug 2006 18:20:43 -0700
> Mingming Cao <cmm@us.ibm.com> wrote:
>
> >
> > Define SECTOR_FMT to print sector_t in proper format
> >
> > ...
> >
> > #define HAVE_SECTOR_T
> > typedef u64 sector_t;
> > +#define SECTOR_FMT "%llu"
>
> We've thus-far avoided doing this. In fact a similar construct in
> device-mapper was recently removed.
>
> Unlike many other attempts, this one appears to be correct (people usually
> get powerpc wrong, due to its u64=unsigned long).
>
> That being said, I'm not really sure we want to add this. It produces
> rather nasty-looking source code and thus far we've just used %llu and we've
> typecasted the sector_t to `unsigned long long'. That happens in a lot of
> places in the kernel and perhaps we don't want to start innovating in ext4
> ;)
>
> That also being said... does a 32-bit sector_t make any sense on a
> 48-bit-blocknumber filesystem? I'd have thought that we'd just make ext4
> depend on 64-bit sector_t and be done with it.
>
> Consequently, sector_t should largely vanish from ext4 and JBD2, except for
> those places where it interfaces with the VFS and the block layer.
> Internally it should just use 64-bit quantities. That could be u64, but
> I'd suggest that the fs simply open-code `unsigned long long' so that we
> don't need to play any gams at all when passing these things into printk.
>

I am fine with unsigned long long -- it does makes the printk a lot
simplier--- I think we had a debate about whether to use sector_t or
just unsigned long long on ext2-devel before, the argument at that time
is to avoid unnecssary memory for in-kernel block variables on 32 bit
machine ..I think that's the concern about using unsigned long long.

I intend to agree with you that the benefit is not so obvious. And since
the on-disk data block number and some metadata are 48bit all the time,
we do have to check the bits of the sector_t to make sure the in-kernel
block variable have enough room to store the blocks when read it from
disk. Not very pretty.

> Finally, perhaps the code is printing block numbers too much ;)
>

> <Notices E3FSBLK, wonders how that snuck through>
>
When we cleanup ext3 code to fix some "int" type block numbers to
"unsigned long" type to able to truely support 2^32 bit ext3 (otherwise
ext3 is limited to 2^31 blocks (8TB)), we had to go through a lot of
printk to modify the format string from %d to %lu. It's a pain.

At that time we are planning to have 48 bit ext3 (now ext4) too, so we
need to go through the same pain again: replace all %lu to %llu in all
the places where the blocks are being print out. Another huge chunk of
patch.

So the decision at that time is to do it once: identify all the printk
cases and use a micro to replace the format string. That is the reason
behind the E3FSBLK

I agree with you that this makes the code hard to read -- I'm fine to
remove it. Fortunately with the previous work, removing it is not so
hard, just simplely search/replace.

The only thing is that we need to type cast every block numbers to be
printed, if the block type is sector_t -- that's a pain. For this reason
I do like to use unsigned long long instead of sector_t.

> I'd suggest that "[patch] ext3: remove E3FSBLK" be written and merged
> before we clone ext4, too...
>

Mingming