On Wed, 2006-08-09 at 23:40 -0700, Andrew Morton wrote: > On Wed, 09 Aug 2006 18:20:43 -0700 > Mingming Cao <cmm@xxxxxxxxxx> 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 - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
