[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20070207203946.GB6565@schatzie.adilger.int>
Date: Wed, 7 Feb 2007 13:39:46 -0700
From: Andreas Dilger <adilger@...sterfs.com>
To: Johann Lombardi <johann.lombardi@...l.net>
Cc: Kalpak Shah <kalpak@...sterfs.com>,
linux-ext4 <linux-ext4@...r.kernel.org>, tytso <tytso@....edu>,
sct <sct@...hat.com>
Subject: Re: [RFC] [PATCH 1/1] Nanosecond timestamps
On Feb 06, 2007 16:12 +0100, Johann Lombardi wrote:
> > + if (sbi->s_inode_size > EXT3_GOOD_OLD_INODE_SIZE) {
> > + EXT3_SB(sb)->s_want_extra_isize = sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
>
> Maybe EXT3_SB(sb)-> could be replaced by sbi-> here and in the lines below.
Yes, this should definitely be done. It also increases clarity between
sbi->s_want_extra_isize and es->s_want_extra_isize.
> > + if (EXT3_SB(sb)->s_want_extra_isize <
> > + le32_to_cpu(es->s_min_extra_isize))
> ^^
> > + EXT3_SB(sb)->s_want_extra_isize =
> > + le32_to_cpu(es->s_min_extra_isize);
> ^^
> Since es->s_{min,want}_extra_isize are both __u16 (BTW, shouldn't it be
> __le16?), I think you should use le16_to_cpu() instead of le32_to_cpu().
You are right - this works fine on little endian systems, but fails on
big endian systems where you will get the other half of the word.
This has been a bug in several places already, and I wonder if the
le*_to_cpu() and cpu_to_le*() macros shouldn't do some type checking
instead of just casting the variable to the specified type?
The only problem is if casting constants it would be a bit of a pain
to have to cast them explicitly, though we could have something like:
#define le16_to_cpu(var) (__builtin_constant(var) || !typecheck(__u16, var) ? \
__constant_cpu_to_le16(var) : __le16_to_cpu(var))
The only question is whether "typecheck" adds extra variables on the stack
or if the compiler will always optimize them away.
> > + /* Check if enough inode space is available */
> > + if (EXT3_GOOD_OLD_INODE_SIZE + EXT3_SB(sb)->s_want_extra_isize >
> > + sbi->s_inode_size) {
> > + EXT3_SB(sb)->s_want_extra_isize = sizeof(struct ext3_inode) - EXT3_GOOD_OLD_INODE_SIZE;
> > + printk(KERN_INFO "EXT3-fs: required extra inode space not"
> > + "available.\n");
> > + }
>
> If the inode size is EXT3_GOOD_OLD_INODE_SIZE, sbi->s_want_extra_isize won't
> be initialized. However, it should not be an issue because the ext3_sb_info
> is set to zero in ext3_fill_super().
So I'm not sure I understand if you have an objection or if this is just a
comment. sbi->s_want_extra_isize will be zero and it is not possible for
sbi->s_inode_size < EXT3_GOOD_OLD_INODE_SIZE so this case won't be hit.
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists