Define SECTOR_FMT to print sector_t in proper format
Signed-off-by: Dave Kleikamp <[email protected]>
Acked-by: Andreas Dilger <[email protected]>
---
linux-2.6.18-rc4-ming/include/asm-h8300/types.h | 1 +
linux-2.6.18-rc4-ming/include/asm-i386/types.h | 1 +
linux-2.6.18-rc4-ming/include/asm-mips/types.h | 5 +++++
linux-2.6.18-rc4-ming/include/asm-powerpc/types.h | 5 +++++
linux-2.6.18-rc4-ming/include/asm-s390/types.h | 5 +++++
linux-2.6.18-rc4-ming/include/asm-sh/types.h | 1 +
linux-2.6.18-rc4-ming/include/asm-x86_64/types.h | 1 +
linux-2.6.18-rc4-ming/include/linux/types.h | 1 +
8 files changed, 20 insertions(+)
diff -puN include/asm-h8300/types.h~sector_fmt include/asm-h8300/types.h
--- linux-2.6.18-rc4/include/asm-h8300/types.h~sector_fmt 2006-08-09 15:41:47.912256609 -0700
+++ linux-2.6.18-rc4-ming/include/asm-h8300/types.h 2006-08-09 15:41:47.936256803 -0700
@@ -57,6 +57,7 @@ typedef u32 dma_addr_t;
#define HAVE_SECTOR_T
typedef u64 sector_t;
+#define SECTOR_FMT "%llu"
#define HAVE_BLKCNT_T
typedef u64 blkcnt_t;
diff -puN include/asm-i386/types.h~sector_fmt include/asm-i386/types.h
--- linux-2.6.18-rc4/include/asm-i386/types.h~sector_fmt 2006-08-09 15:41:47.915256633 -0700
+++ linux-2.6.18-rc4-ming/include/asm-i386/types.h 2006-08-09 15:41:47.937256811 -0700
@@ -59,6 +59,7 @@ typedef u64 dma64_addr_t;
#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FMT "%llu"
#define HAVE_SECTOR_T
#endif
diff -puN include/asm-mips/types.h~sector_fmt include/asm-mips/types.h
--- linux-2.6.18-rc4/include/asm-mips/types.h~sector_fmt 2006-08-09 15:41:47.918256657 -0700
+++ linux-2.6.18-rc4-ming/include/asm-mips/types.h 2006-08-09 15:41:47.938256819 -0700
@@ -95,6 +95,11 @@ typedef unsigned long phys_t;
#ifdef CONFIG_LBD
typedef u64 sector_t;
+#if (_MIPS_SZLONG == 64)
+#define SECTOR_FMT "%lu"
+#else
+#define SECTOR_FMT "%llu"
+#endif
#define HAVE_SECTOR_T
#endif
diff -puN include/asm-powerpc/types.h~sector_fmt include/asm-powerpc/types.h
--- linux-2.6.18-rc4/include/asm-powerpc/types.h~sector_fmt 2006-08-09 15:41:47.921256681 -0700
+++ linux-2.6.18-rc4-ming/include/asm-powerpc/types.h 2006-08-09 15:41:47.938256819 -0700
@@ -99,6 +99,11 @@ typedef struct {
#ifdef CONFIG_LBD
typedef u64 sector_t;
+#ifdef __powerpc64__
+#define SECTOR_FMT "%lu"
+#else
+#define SECTOR_FMT "%llu"
+#endif
#define HAVE_SECTOR_T
#endif
diff -puN include/asm-s390/types.h~sector_fmt include/asm-s390/types.h
--- linux-2.6.18-rc4/include/asm-s390/types.h~sector_fmt 2006-08-09 15:41:47.924256706 -0700
+++ linux-2.6.18-rc4-ming/include/asm-s390/types.h 2006-08-09 15:41:47.939256827 -0700
@@ -89,6 +89,11 @@ typedef union {
#ifdef CONFIG_LBD
typedef u64 sector_t;
+#ifndef __s390x__
+#define SECTOR_FMT "%llu"
+#else
+#define SECTOR_FMT "%lu"
+#endif
#define HAVE_SECTOR_T
#endif
diff -puN include/asm-sh/types.h~sector_fmt include/asm-sh/types.h
--- linux-2.6.18-rc4/include/asm-sh/types.h~sector_fmt 2006-08-09 15:41:47.927256730 -0700
+++ linux-2.6.18-rc4-ming/include/asm-sh/types.h 2006-08-09 15:41:47.939256827 -0700
@@ -54,6 +54,7 @@ typedef u32 dma_addr_t;
#ifdef CONFIG_LBD
typedef u64 sector_t;
+#define SECTOR_FMT "%llu"
#define HAVE_SECTOR_T
#endif
diff -puN include/asm-x86_64/types.h~sector_fmt include/asm-x86_64/types.h
--- linux-2.6.18-rc4/include/asm-x86_64/types.h~sector_fmt 2006-08-09 15:41:47.931256762 -0700
+++ linux-2.6.18-rc4-ming/include/asm-x86_64/types.h 2006-08-09 15:41:47.940256835 -0700
@@ -49,6 +49,7 @@ typedef u64 dma64_addr_t;
typedef u64 dma_addr_t;
typedef u64 sector_t;
+#define SECTOR_FMT "%llu"
#define HAVE_SECTOR_T
#endif /* __ASSEMBLY__ */
diff -puN include/linux/types.h~sector_fmt include/linux/types.h
--- linux-2.6.18-rc4/include/linux/types.h~sector_fmt 2006-08-09 15:41:47.934256787 -0700
+++ linux-2.6.18-rc4-ming/include/linux/types.h 2006-08-09 15:41:47.940256835 -0700
@@ -134,6 +134,7 @@ typedef __s64 int64_t;
*/
#ifndef HAVE_SECTOR_T
typedef unsigned long sector_t;
+#define SECTOR_FMT "%lu"
#endif
#ifndef HAVE_BLKCNT_T
_
On Wed, 09 Aug 2006 18:20:43 -0700
Mingming Cao <[email protected]> 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.
Finally, perhaps the code is printing block numbers too much ;)
<Notices E3FSBLK, wonders how that snuck through>
I'd suggest that "[patch] ext3: remove E3FSBLK" be written and merged
before we clone ext4, too...
Hi,
On Wed, 9 Aug 2006, Andrew Morton wrote:
> 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.
Is this really necessary? There are a few features, which would make ext4
also interesting at the low end (e.g. extents). Storing 64bit values on
disk is fine, but they should be converted to native values as soon as
possible.
bye, Roman
Roman Zippel wrote:
> Hi,
>
> On Wed, 9 Aug 2006, Andrew Morton wrote:
>
>> 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.
>
> Is this really necessary? There are a few features, which would make ext4
> also interesting at the low end (e.g. extents). Storing 64bit values on
> disk is fine, but they should be converted to native values as soon as
> possible.
Consider what that means. "converted to native" means dealing with
truncation issues...
Jeff
Roman Zippel wrote:
> Yes, it does, but I don't think it's that difficult - basically returning
> -EIO, it should be part of the basic error handling. Afterwards you don't
> have to waste cpu/memory on unused data anymore.
Or you could just not bother, and leave everything as u64.
Jeff
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> > On Wed, 9 Aug 2006, Andrew Morton wrote:
> >
> > > 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.
> >
> > Is this really necessary? There are a few features, which would make ext4
> > also interesting at the low end (e.g. extents). Storing 64bit values on disk
> > is fine, but they should be converted to native values as soon as possible.
>
> Consider what that means. "converted to native" means dealing with truncation
> issues...
Yes, it does, but I don't think it's that difficult - basically returning
-EIO, it should be part of the basic error handling. Afterwards you don't
have to waste cpu/memory on unused data anymore.
bye, Roman
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> Roman Zippel wrote:
> > Yes, it does, but I don't think it's that difficult - basically returning
> > -EIO, it should be part of the basic error handling. Afterwards you don't
> > have to waste cpu/memory on unused data anymore.
>
> Or you could just not bother, and leave everything as u64.
Why?
bye, Roman
Roman Zippel wrote:
> Hi,
>
> On Thu, 10 Aug 2006, Jeff Garzik wrote:
>
>> Roman Zippel wrote:
>>> Yes, it does, but I don't think it's that difficult - basically returning
>>> -EIO, it should be part of the basic error handling. Afterwards you don't
>>> have to waste cpu/memory on unused data anymore.
>> Or you could just not bother, and leave everything as u64.
>
> Why?
To eliminate needless complexity and keep things simple and obvious?
Jeff
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> > > Or you could just not bother, and leave everything as u64.
> >
> > Why?
>
> To eliminate needless complexity and keep things simple and obvious?
Considering the amount of complexity we add for the high end, why is it
suddenly a bad thing to add even a _little_ complexity for the other end?
bye, Roman
Roman Zippel wrote:
> On Thu, 10 Aug 2006, Jeff Garzik wrote:
>>>> Or you could just not bother, and leave everything as u64.
>>> Why?
>> To eliminate needless complexity and keep things simple and obvious?
>
> Considering the amount of complexity we add for the high end, why is it
> suddenly a bad thing to add even a _little_ complexity for the other end?
This is ext4 not ext3 we're talking about. The next gen Linux
filesystem should be tuned for modern machines -- 64bit, moving forward
-- while still working just fine on 32bit.
Jeff
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> > Considering the amount of complexity we add for the high end, why is it
> > suddenly a bad thing to add even a _little_ complexity for the other end?
>
> This is ext4 not ext3 we're talking about. The next gen Linux filesystem
> should be tuned for modern machines -- 64bit, moving forward -- while still
> working just fine on 32bit.
If you force everyone to use 64bit sector numbers, I don't understand how
you can claim "still working just fine on 32bit"?
At some point ext4 is probably going to be the de facto standard, which
very many people want to use, because it has all the new features, which
won't be ported to ext2/3. So I still don't understand, what's so wrong
about a little tuning in both directions?
bye, Roman
Roman Zippel wrote:
> If you force everyone to use 64bit sector numbers, I don't understand how
> you can claim "still working just fine on 32bit"?
64bit sector numbers work just fine on 32-bit machines.
> At some point ext4 is probably going to be the de facto standard, which
> very many people want to use, because it has all the new features, which
> won't be ported to ext2/3. So I still don't understand, what's so wrong
> about a little tuning in both directions?
Just seems like wasted effort to me.
Jeff
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> Roman Zippel wrote:
> > If you force everyone to use 64bit sector numbers, I don't understand how
> > you can claim "still working just fine on 32bit"?
>
> 64bit sector numbers work just fine on 32-bit machines.
Depends on the definition of "fine".
> > At some point ext4 is probably going to be the de facto standard, which very
> > many people want to use, because it has all the new features, which won't be
> > ported to ext2/3. So I still don't understand, what's so wrong about a
> > little tuning in both directions?
>
> Just seems like wasted effort to me.
I disagree.
Many developer still brag about how Linux runs on about everything, but
it's little steps like this, which make it more and more a joke.
bye, Roman
Roman Zippel wrote:
> I disagree.
> Many developer still brag about how Linux runs on about everything, but
> it's little steps like this, which make it more and more a joke.
It does still "run" on most everything but it's naive to think that you can do
anything and everything on 10-year-old hardware. Choose ext3 and Linux still
runs just fine. And honestly I doubt that ext4 will show much problem either.
ext4 is being developed primarily to address scaling issues at the high end of
the storage spectrum. If you're concerned about carrying 64-bit containers,
just use ext3, and be happy with your 32-bit, < 16TB filesystems, I'd say.
-Eric
Roman Zippel wrote:
> Hi,
>
> On Thu, 10 Aug 2006, Jeff Garzik wrote:
>
>> Roman Zippel wrote:
>>> If you force everyone to use 64bit sector numbers, I don't understand how
>>> you can claim "still working just fine on 32bit"?
>> 64bit sector numbers work just fine on 32-bit machines.
>
> Depends on the definition of "fine".
>
>>> At some point ext4 is probably going to be the de facto standard, which very
>>> many people want to use, because it has all the new features, which won't be
>>> ported to ext2/3. So I still don't understand, what's so wrong about a
>>> little tuning in both directions?
>> Just seems like wasted effort to me.
>
> I disagree.
> Many developer still brag about how Linux runs on about everything, but
> it's little steps like this, which make it more and more a joke.
What joke? With CONFIG_LBD, 32-bit machines can already support large
block devices.
If you feel that hardcoding u64 as sector numbers will mean ext4
suddenly fails on 32-bit, you misunderstand the situation completely.
Linux (and ext4) will continue to run everywhere.
Jeff
Hi,
On Thu, 10 Aug 2006, Eric Sandeen wrote:
> ext4 is being developed primarily to address scaling issues at the high end of
> the storage spectrum. If you're concerned about carrying 64-bit containers,
> just use ext3, and be happy with your 32-bit, < 16TB filesystems, I'd say.
The problem being that it doesn't _exclusively_ address scaling issues,
some new features may well be interesting to non high end users as well.
If it's supposed to be a high end only fs, then please don't call ext4,
otherwise it would mislead users about what it doesn't is - a general
purpose fs.
bye, Roman
Roman Zippel wrote:
> Hi,
>
> On Thu, 10 Aug 2006, Eric Sandeen wrote:
>
>> ext4 is being developed primarily to address scaling issues at the high end of
>> the storage spectrum. If you're concerned about carrying 64-bit containers,
>> just use ext3, and be happy with your 32-bit, < 16TB filesystems, I'd say.
>
> The problem being that it doesn't _exclusively_ address scaling issues,
> some new features may well be interesting to non high end users as well.
> If it's supposed to be a high end only fs, then please don't call ext4,
> otherwise it would mislead users about what it doesn't is - a general
> purpose fs.
It will work just fine on 32-bit machines.
You're making a mountain out of a molehill.
Jeff
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> With CONFIG_LBD, 32-bit machines can already support large block
> devices.
>
> If you feel that hardcoding u64 as sector numbers will mean ext4 suddenly
> fails on 32-bit, you misunderstand the situation completely.
With CONFIG_LBD disabled you still had the truncation/complexity issues
somewhere else, so you gain nothing, but waste memory in ext4.
bye, Roman
Roman Zippel wrote:
> Hi,
>
> On Thu, 10 Aug 2006, Jeff Garzik wrote:
>
>> With CONFIG_LBD, 32-bit machines can already support large block
>> devices.
>>
>> If you feel that hardcoding u64 as sector numbers will mean ext4 suddenly
>> fails on 32-bit, you misunderstand the situation completely.
>
> With CONFIG_LBD disabled you still had the truncation/complexity issues
> somewhere else, so you gain nothing, but waste memory in ext4.
You gain simplicity and reduced number of code paths.
"waste memory" is hardly a significant argument. I doubt you will
notice a difference.
Jeff
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> > The problem being that it doesn't _exclusively_ address scaling issues, some
> > new features may well be interesting to non high end users as well. If it's
> > supposed to be a high end only fs, then please don't call ext4, otherwise it
> > would mislead users about what it doesn't is - a general purpose fs.
>
> It will work just fine on 32-bit machines.
>
> You're making a mountain out of a molehill.
I'm not sure we're talking about the same thing, my initial concern was
this comment from Andrew: "I'd have thought that we'd just make ext4
depend on 64-bit sector_t and be done with it." This would require LBD and
I don't think this is "just fine" on 32bit machines.
bye, Roman
Hi,
On Thu, 10 Aug 2006, Jeff Garzik wrote:
> > With CONFIG_LBD disabled you still had the truncation/complexity issues
> > somewhere else, so you gain nothing, but waste memory in ext4.
>
> You gain simplicity and reduced number of code paths.
Compared to other things it's almost nothing.
> "waste memory" is hardly a significant argument. I doubt you will notice a
> difference.
Compared to other current waste, that's possible, but why do we have to
make it worse and don't even make an effort to keep it a little under
control?
bye, Roman
On Wed, Aug 09, 2006 at 11:40:19PM -0700, Andrew Morton wrote:
> On Wed, 09 Aug 2006 18:20:43 -0700
> Mingming Cao <[email protected]> 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.
Ext4 will support a 48-bit blocknumber format for extents, but I do
want to make ext4 suitable as a general purpose filesystem, and 32-bit
systems will be around for I fear far longer than people might wish.
So while I agree that we shouldn't go _too_ far out of our way to make
things efficient on 32-bit systems, if it's not that much work to
support a 32-bit sector_t, we ought to do it.
So how about a compromise? We allow for a 32-bit sector_t in ext4,
but we drop the SECTOR_FMT, and rely on %llu and typecasts in
printk's. Then the only other extra hair in the filesystem code will
be a mount-time check to make sure we don't try to mount a large
filesystem on system with a 32-bit sector_t.
- Ted
>>>>> "Roman" == Roman Zippel <[email protected]> writes:
Roman> If you force everyone to use 64bit sector numbers, I don't
Roman> understand how you can claim "still working just fine on
Roman> 32bit"? At some point ext4 is probably going to be the de
Roman> facto standard, which very many people want to use, because it
Roman> has all the new features, which won't be ported to ext2/3. So I
Roman> still don't understand, what's so wrong about a little tuning
Roman> in both directions?
The problem as I see it, is that you want extents, but you don't want
the RAM/DISK/ROM penalty of 64bit blocks, since embedded devices won't
ever go past the existing ext3 sizes, right?
Is this a more clear statement of what you want?
So the next question I have, and this is for the ext3/ext4 developers,
is whether ext4 will have a feature flag to mark whether the
filesystem has 64bit sector numbers or not. If so, then I think Roman
will be fine.
Me, I think going all 64bit is the way to go, since it moves the
limits so high up, that we probably won't hit them in a serious way
for another 20 years in terms of disk space addressing needs, sorta
like how 64bit CPUs have really moved out the RAM constraints as
well.
Now sure, there are people who will hit those limits, but they're in a
very very small minority with the money to pay for a large large
electric bill to just run a system that big. For now.
John
Hi,
On Thu, 10 Aug 2006, Theodore Tso wrote:
> Ext4 will support a 48-bit blocknumber format for extents, but I do
> want to make ext4 suitable as a general purpose filesystem, and 32-bit
> systems will be around for I fear far longer than people might wish.
> So while I agree that we shouldn't go _too_ far out of our way to make
> things efficient on 32-bit systems, if it's not that much work to
> support a 32-bit sector_t, we ought to do it.
>
> So how about a compromise? We allow for a 32-bit sector_t in ext4,
> but we drop the SECTOR_FMT, and rely on %llu and typecasts in
> printk's. Then the only other extra hair in the filesystem code will
> be a mount-time check to make sure we don't try to mount a large
> filesystem on system with a 32-bit sector_t.
Thanks, that's what I was hoping for. :)
Disallowing to mount large fs without CONFIG_LBD is not a real problem and
then also truncation is not an issue anymore (except maybe for e2fsck).
bye, Roman
Hi,
On Thu, 10 Aug 2006, John Stoffel wrote:
> The problem as I see it, is that you want extents, but you don't want
> the RAM/DISK/ROM penalty of 64bit blocks, since embedded devices won't
> ever go past the existing ext3 sizes, right?
>
> Is this a more clear statement of what you want?
This is only about the runtime penalty. I'm less concerned about the disk
structures, as soon as you use extents it's often more efficient than the
current structure anyway.
bye, Roman
On Thu, 10 Aug 2006 12:24:06 -0400
"John Stoffel" <[email protected]> wrote:
> >>>>> "Roman" == Roman Zippel <[email protected]> writes:
>
> Roman> If you force everyone to use 64bit sector numbers, I don't
> Roman> understand how you can claim "still working just fine on
> Roman> 32bit"? At some point ext4 is probably going to be the de
> Roman> facto standard, which very many people want to use, because it
> Roman> has all the new features, which won't be ported to ext2/3. So I
> Roman> still don't understand, what's so wrong about a little tuning
> Roman> in both directions?
>
> The problem as I see it, is that you want extents, but you don't want
> the RAM/DISK/ROM penalty of 64bit blocks, since embedded devices won't
> ever go past the existing ext3 sizes, right?
For ext3 on x86:
CONFIG_LBD=y:
box:/usr/src/25> size fs/jbd/jbd.o fs/ext3/ext3.o
text data bss dec hex filename
51076 8 32 51116 c7ac fs/jbd/jbd.o
87466 1020 4 88490 159aa fs/ext3/ext3.o
CONFIG_LBD=n:
box:/usr/src/25> size fs/jbd/jbd.o fs/ext3/ext3.o
text data bss dec hex filename
51133 8 32 51173 c7e5 fs/jbd/jbd.o
87679 1020 4 88703 15a7f fs/ext3/ext3.o
That's a grand total of 270 bytes of text saved. aka 0.19%.
We'll save four bytes in the inode (unlikely to save anything due to slab
packing).
We'll save 12 bytes against open-for-writing files due to
ext3_block_alloc_info shrinkage (unlikely to save anything due to kmalloc
size roundup).
IOW, unless I've missed something major, we're looking at a total saving of
around a 16th of a page. We can save more than that any day of the week by
going around and deleting some crap from somewhere. We can surely save
more than this by taming ext4's inlining frenzy.
I'd expect any runtime savings to be similarly modest.
Hi,
On Thu, 10 Aug 2006, Andrew Morton wrote:
> For ext3 on x86:
>
> CONFIG_LBD=y:
>
> box:/usr/src/25> size fs/jbd/jbd.o fs/ext3/ext3.o
> text data bss dec hex filename
> 51076 8 32 51116 c7ac fs/jbd/jbd.o
> 87466 1020 4 88490 159aa fs/ext3/ext3.o
>
> CONFIG_LBD=n:
>
> box:/usr/src/25> size fs/jbd/jbd.o fs/ext3/ext3.o
> text data bss dec hex filename
> 51133 8 32 51173 c7e5 fs/jbd/jbd.o
> 87679 1020 4 88703 15a7f fs/ext3/ext3.o
>
> That's a grand total of 270 bytes of text saved. aka 0.19%.
>
> We'll save four bytes in the inode (unlikely to save anything due to slab
> packing).
sector_t is used in multiple structures (bio/request/buffer_head), which
quickly adds up.
ext3 is also currently not a very heavy sector_t user, if you try this
with block/ you get a more than 3% difference.
bye, Roman
On Wed, Aug 09, 2006 at 11:40:19PM -0700, Andrew Morton wrote:
> On Wed, 09 Aug 2006 18:20:43 -0700
> Mingming Cao <[email protected]> wrote:
>
> > Define SECTOR_FMT to print sector_t in proper format
>
> We've thus-far avoided doing this. In fact a similar construct in
> device-mapper was recently removed.
Yeah, OCFS2 had similar formats, and we were asked to change
them to naked casts before inclusion. Seems quite consistent with the
rest of the kernel.
Joel
--
"Can any of you seriously say the Bill of Rights could get through
Congress today? It wouldn't even get out of committee."
- F. Lee Bailey
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Thu, Aug 10, 2006 at 12:17:47PM -0700, Joel Becker wrote:
> On Wed, Aug 09, 2006 at 11:40:19PM -0700, Andrew Morton wrote:
> > On Wed, 09 Aug 2006 18:20:43 -0700
> > Mingming Cao <[email protected]> wrote:
> >
> > > Define SECTOR_FMT to print sector_t in proper format
> >
> > We've thus-far avoided doing this. In fact a similar construct in
> > device-mapper was recently removed.
>
> Yeah, OCFS2 had similar formats, and we were asked to change
> them to naked casts before inclusion. Seems quite consistent with the
> rest of the kernel.
Will
printk("%S", sector_t);
kill at least one kitten?
On Thu, 10 Aug 2006 23:44:40 +0400
Alexey Dobriyan <[email protected]> wrote:
> On Thu, Aug 10, 2006 at 12:17:47PM -0700, Joel Becker wrote:
> > On Wed, Aug 09, 2006 at 11:40:19PM -0700, Andrew Morton wrote:
> > > On Wed, 09 Aug 2006 18:20:43 -0700
> > > Mingming Cao <[email protected]> wrote:
> > >
> > > > Define SECTOR_FMT to print sector_t in proper format
> > >
> > > We've thus-far avoided doing this. In fact a similar construct in
> > > device-mapper was recently removed.
> >
> > Yeah, OCFS2 had similar formats, and we were asked to change
> > them to naked casts before inclusion. Seems quite consistent with the
> > rest of the kernel.
>
> Will
>
> printk("%S", sector_t);
>
> kill at least one kitten?
It would be really nice to be able to define local enhancements like this
to printf. It would solve lots of these problems quite nicely.
Bus alas, there's no way (afaik) to teach __attribute__((format)) about
them, so gcc will warn.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Alexey Dobriyan wrote:
> On Thu, Aug 10, 2006 at 12:17:47PM -0700, Joel Becker wrote:
>> On Wed, Aug 09, 2006 at 11:40:19PM -0700, Andrew Morton wrote:
>>> On Wed, 09 Aug 2006 18:20:43 -0700
>>> Mingming Cao <[email protected]> wrote:
>>>
>>>> Define SECTOR_FMT to print sector_t in proper format
>>> We've thus-far avoided doing this. In fact a similar construct in
>>> device-mapper was recently removed.
>> Yeah, OCFS2 had similar formats, and we were asked to change
>> them to naked casts before inclusion. Seems quite consistent with the
>> rest of the kernel.
>
> Will
>
> printk("%S", sector_t);
>
> kill at least one kitten?
I like the general idea. I think that having to cast every time you want
to print a sector number is pretty gross. I had something more like %Su
in mind, though.
- -Jeff
- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org
iD8DBQFE25RfLPWxlyuTD7IRAshdAJ0dboV4trG1Pgy6P+sqPiV0bWabYQCgiZng
FuaanX+K+jSTwlrapumR1XY=
=jLoe
-----END PGP SIGNATURE-----
On Thu, Aug 10, 2006 at 12:57:47PM -0700, Andrew Morton wrote:
> On Thu, 10 Aug 2006 23:44:40 +0400
> Alexey Dobriyan <[email protected]> wrote:
> > On Thu, Aug 10, 2006 at 12:17:47PM -0700, Joel Becker wrote:
> > > On Wed, Aug 09, 2006 at 11:40:19PM -0700, Andrew Morton wrote:
> > > > On Wed, 09 Aug 2006 18:20:43 -0700
> > > > Mingming Cao <[email protected]> wrote:
> > > >
> > > > > Define SECTOR_FMT to print sector_t in proper format
> > > >
> > > > We've thus-far avoided doing this. In fact a similar construct in
> > > > device-mapper was recently removed.
> > >
> > > Yeah, OCFS2 had similar formats, and we were asked to change
> > > them to naked casts before inclusion. Seems quite consistent with the
> > > rest of the kernel.
> >
> > Will
> >
> > printk("%S", sector_t);
> >
> > kill at least one kitten?
>
> It would be really nice to be able to define local enhancements like this
> to printf. It would solve lots of these problems quite nicely.
>
> Bus alas, there's no way (afaik) to teach __attribute__((format)) about
> them, so gcc will warn.
Arrrgh!
from gcc-local(1)
- gcc recognizes a new format attribute, kprintf, to deal with the ex-
tra format arguments `%b', `%r', and `%z' used in the OpenBSD kernel.
One word: bastards.
On Thu, 2006-08-10 at 18:37 +0200, Roman Zippel wrote:
> Hi,
>
> On Thu, 10 Aug 2006, Theodore Tso wrote:
>
> > Ext4 will support a 48-bit blocknumber format for extents, but I do
> > want to make ext4 suitable as a general purpose filesystem, and 32-bit
> > systems will be around for I fear far longer than people might wish.
> > So while I agree that we shouldn't go _too_ far out of our way to make
> > things efficient on 32-bit systems, if it's not that much work to
> > support a 32-bit sector_t, we ought to do it.
> >
> > So how about a compromise? We allow for a 32-bit sector_t in ext4,
> > but we drop the SECTOR_FMT, and rely on %llu and typecasts in
> > printk's. Then the only other extra hair in the filesystem code will
> > be a mount-time check to make sure we don't try to mount a large
> > filesystem on system with a 32-bit sector_t.
>
> Thanks, that's what I was hoping for. :)
> Disallowing to mount large fs without CONFIG_LBD is not a real problem and
> then also truncation is not an issue anymore (except maybe for e2fsck).
>
Just to clarify, the current code (both ext3/4) already check the
filesystem blocks number before mounting it...it fails to mount the
large device if sector_t is 32bit:
in fs/ext4/super.c:
static int ext4_fill_super (struct super_block *sb, void *data, int
silent)
{
.....
if (EXT4_BLOCKS_COUNT(es) >
(sector_t)(~0ULL) >> (sb->s_blocksize_bits - 9)) {
printk(KERN_ERR "EXT4-fs: filesystem on %s:"
" too large to mount safely\n", sb->s_id);
if (sizeof(sector_t) < 8)
printk(KERN_WARNING "EXT4-fs: CONFIG_LBD not "
"enabled\n");
goto failed_mount;
}
}
> bye, Roman
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2006-08-09 at 23:40 -0700, Andrew Morton wrote:
> On Wed, 09 Aug 2006 18:20:43 -0700
> Mingming Cao <[email protected]> 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
Mingming Cao wrote:
> 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.
<OT>
FWIW, ext3 still isn't 32-bit clean, in my testing. The types are ok but the
code overflows them with arithmetic in some places. Look for patches from me
soon ;-)
</OT>
-eric
>> Will
>>
>> printk("%S", sector_t);
>>
>> kill at least one kitten?
>
>I like the general idea. I think that having to cast every time you want
>to print a sector number is pretty gross. I had something more like %Su
>in mind, though.
What will happen if you run out of %[a-z] ?
Jan Engelhardt
--
On Fri, 2006-08-11 at 07:57 +0200, Jan Engelhardt wrote:
> >> Will
> >>
> >> printk("%S", sector_t);
> >>
> >> kill at least one kitten?
> >
> >I like the general idea. I think that having to cast every time you want
> >to print a sector number is pretty gross. I had something more like %Su
> >in mind, though.
>
> What will happen if you run out of %[a-z] ?
My keyboard can produce a lot more characters (even if I ignore the
non-ASCII ones).
Bernd
--
Firmix Software GmbH http://www.firmix.at/
mobil: +43 664 4416156 fax: +43 1 7890849-55
Embedded Linux Development and Services
>> >> Will
>> >>
>> >> printk("%S", sector_t);
>> >>
>> >> kill at least one kitten?
>> >
>> >I like the general idea. I think that having to cast every time you want
>> >to print a sector number is pretty gross. I had something more like %Su
>> >in mind, though.
>>
>> What will happen if you run out of %[a-z] ?
>
>My keyboard can produce a lot more characters (even if I ignore the
>non-ASCII ones).
Oh yes I know we could use %ö %Д and %私 but that will possibly not amuse
others outside your locale.
(Note, it should have been %[A-Za-z])
Jan Engelhardt
--
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Jan Engelhardt wrote:
>>> Will
>>>
>>> printk("%S", sector_t);
>>>
>>> kill at least one kitten?
>> I like the general idea. I think that having to cast every time you want
>> to print a sector number is pretty gross. I had something more like %Su
>> in mind, though.
>
> What will happen if you run out of %[a-z] ?
Are we really expecting that many global structure members to be
variable width? I only propose adding another option because whenever a
sector is printed, it must be casted to avoid warnings.
Other replies commented on how gcc won't recognize the new option, so
we'd receive warnings anyway. Cleaner code that causes warnings doesn't
sound like a big win after all.
- -Jeff
- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org
iD8DBQFE3Ji1LPWxlyuTD7IRApAEAJ9ApkoyKwmTReZindjJmkuU/0yhbACgk0Uu
zG8eXN3RzU1wKFVrRlr3xO8=
=e6D2
-----END PGP SIGNATURE-----
On Thu, 2006-08-10 at 17:59 -0700, Mingming Cao wrote:
> On Wed, 2006-08-09 at 23:40 -0700, Andrew Morton wrote:
> > On Wed, 09 Aug 2006 18:20:43 -0700
> > Mingming Cao <[email protected]> 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...
> >
Hmm, I am thinking we should do this cleanup after we clone ext4.
As in ext3, all block type are unsigned long, so the format string would
be "%lu" and we don't need to the typecast for all block varaibles to be
printed.
For ext4, the format string would be "%llu" not "%lu". If we do the
remove E2FSBLK before the clone, we will lost the mark of places where
we need to convert the format string from %lu to %llu.
Also, if we keep the sector_t and not goes with unsigned long long for
the filesystem block type, then we need to case the type for the blocks
to be printed. Keeping the E3FSBLK around until we finish the cast will
makes the effort a lot easier. Of course, if we decide to go withes
unsigned long long, this is not a issue.
Mingming