Re: [RFC] [PATCH 1/1] nanosecond timestamps
On Feb 26, 2007 17:20 -0600, Dave Kleikamp wrote:
> I wanted to see if I could clean this up, and this is what I came up
> with. I also did some macro magic to make these macros take one less
> argument. I ended up breaking two macros up into three smaller macros
> and two inline functions. Does this look any better? (This is
> incremental against Kalpak's patch.)
>
> Signed-off-by: Dave Kleikamp <[EMAIL PROTECTED]>
Looks like an improvement to me, if only to give a name and clarity to
the "EXT3_FITS_IN_INODE()" part of the function.
One other note - there was a problem with the original patch because
i_crtime is also in the extended part of the inode so it needs to
be validated against i_extra_isize so we need to submit a new patch
for that anyways and may as well include this cleanup in that patch.
Thanks Dave.
> diff -Nurp linux.orig/fs/ext3/inode.c linux/fs/ext3/inode.c
> --- linux.orig/fs/ext3/inode.c2007-02-26 17:10:22.0 -0600
> +++ linux/fs/ext3/inode.c 2007-02-26 17:02:28.0 -0600
> @@ -2674,10 +2674,10 @@ void ext3_read_inode(struct inode * inod
> inode->i_nlink = le16_to_cpu(raw_inode->i_links_count);
> inode->i_size = le32_to_cpu(raw_inode->i_size);
>
> - EXT3_INODE_GET_XTIME(i_ctime, i_ctime_extra, inode, raw_inode);
> - EXT3_INODE_GET_XTIME(i_mtime, i_mtime_extra, inode, raw_inode);
> - EXT3_INODE_GET_XTIME(i_atime, i_atime_extra, inode, raw_inode);
> - EXT3_INODE_GET_XTIME(i_crtime, i_crtime_extra, ei, raw_inode);
> + EXT3_INODE_GET_XTIME(i_ctime, inode, raw_inode);
> + EXT3_INODE_GET_XTIME(i_mtime, inode, raw_inode);
> + EXT3_INODE_GET_XTIME(i_atime, inode, raw_inode);
> + EXT3_INODE_GET_XTIME(i_crtime, ei, raw_inode);
>
> ei->i_state = 0;
> ei->i_dir_start_lookup = 0;
> @@ -2829,10 +2829,10 @@ static int ext3_do_update_inode(handle_t
> }
> raw_inode->i_links_count = cpu_to_le16(inode->i_nlink);
> raw_inode->i_size = cpu_to_le32(ei->i_disksize);
> - EXT3_INODE_SET_XTIME(i_ctime, i_ctime_extra, inode, raw_inode);
> - EXT3_INODE_SET_XTIME(i_mtime, i_mtime_extra, inode, raw_inode);
> - EXT3_INODE_SET_XTIME(i_atime, i_atime_extra, inode, raw_inode);
> - EXT3_INODE_SET_XTIME(i_crtime, i_crtime_extra, ei, raw_inode);
> + EXT3_INODE_SET_XTIME(i_ctime, inode, raw_inode);
> + EXT3_INODE_SET_XTIME(i_mtime, inode, raw_inode);
> + EXT3_INODE_SET_XTIME(i_atime, inode, raw_inode);
> + EXT3_INODE_SET_XTIME(i_crtime, ei, raw_inode);
>
> raw_inode->i_blocks = cpu_to_le32(inode->i_blocks);
> raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
> diff -Nurp linux.orig/include/linux/ext3_fs.h linux/include/linux/ext3_fs.h
> --- linux.orig/include/linux/ext3_fs.h2007-02-26 17:10:22.0
> -0600
> +++ linux/include/linux/ext3_fs.h 2007-02-26 17:07:20.0 -0600
> @@ -331,37 +331,42 @@ struct ext3_inode {
> #define EXT3_EPOCH_MASK ((1 << EXT3_EPOCH_BITS) - 1)
> #define EXT3_NSEC_MASK (~0UL << EXT3_EPOCH_BITS)
>
> +#define EXT3_FITS_IN_INODE(ext3_inode, field)\
> + ((offsetof(typeof(*ext3_inode), field) -\
> + offsetof(typeof(*ext3_inode), i_extra_isize) +\
> + sizeof((ext3_inode)->field)) <= \
> + le16_to_cpu((ext3_inode)->i_extra_isize))
> +
> +static inline __le32 ext3_encode_extra_time(struct timespec *time)
> +{
> + return cpu_to_le32((sizeof(time->tv_sec) > 4 ? time->tv_sec >> 32 : 0) |
> +((time->tv_nsec << 2) & EXT3_NSEC_MASK));
I think this should be:
return cpu_to_le32((sizeof(time->tv_sec) > 4 ?
(time->tv_sec >> 32) & EXT3_EPOCH_MASK : 0) |
(time->tv_nsec << EXT3_EPOCH_BITS));
We should use EXT3_EPOCH_BITS, and don't really need "& EXT3_NSEC_MASK"
unless there is some reason to believe that tv_nsec is > 1e9. Even then
it would be truncated at 32 bits by the (__u32) cast in cpu_to_le32().
> +static inline void ext3_decode_extra_time(struct timespec *time, __le32
> extra) {
> + if (sizeof(time->tv_sec) > 4)
> + time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT3_EPOCH_MASK)
> + << 32;
> + time->tv_nsec = (le32_to_cpu(extra) & EXT3_NSEC_MASK) >> 2;
And this should be:
time->tv_nsec = (le32_to_cpu(extra) & EXT3_NSEC_MASK) >>EXT3_EPOCH_BITS;
We could again skip the "& EXT3_NSEC_MASK" because le32_to_cpu(extra) will
truncate the input at 32 bits, so:
time->tv_nsec = le32_to_cpu(extra) >> EXT3_EPOCH_BITS;
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] nanosecond timestamps
On Mon, 2007-02-26 at 14:42 -0700, Andreas Dilger wrote:
> On Feb 25, 2007 02:36 -0800, Andrew Morton wrote:
> > > On Fri, 02 Feb 2007 20:09:40 +0530 Kalpak Shah <[EMAIL PROTECTED]> wrote:
> > > +#define EXT3_INODE_GET_XTIME(xtime, extra_xtime, inode, raw_inode)
> > > \
> > > +do {
> > > \
> > > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime);
> > > \
> > > + \
> > > + if (offsetof(typeof(*raw_inode), extra_xtime) -
> > > \
> > > + offsetof(typeof(*raw_inode), i_extra_isize) +
> > > \
> > > + sizeof((raw_inode)->extra_xtime) <=
> > > \
> > > + le16_to_cpu((raw_inode)->i_extra_isize)) {
> > > \
> > > + if (sizeof((inode)->xtime.tv_sec) > 4)\
> > > + (inode)->xtime.tv_sec |= \
> > > + (__u64)(le32_to_cpu((raw_inode)->extra_xtime) & \
> > > + EXT3_EPOCH_MASK) << 32; \
> > > + (inode)->xtime.tv_nsec = \
> > > + (le32_to_cpu((raw_inode)->extra_xtime) & \
> > > + EXT3_NSEC_MASK) >> 2; \
> > > + }
> > > \
> > > +} while (0)
> >
> > ow, my eyes. Can we find a way to do this in C rather than in cpp?
>
> The macro is working on the field names of the inode in most places
> (e.g. i_mtime, i_ctime, etc) rather than the values. You _could_ do it
> in C, but it would mean moving all of the "offsetof()" into the caller
> (basically putting the whole macro in-line at the caller) or doing math
> on the pointer addresses at runtime instead of compile time.
>
> It boils down to a check whether the particular nanosecond field is
> inside the reserved space in the inode or not, so it ends up a comparison
> against a constant. For ctime:
>
> if (4 <= le32_to_cpu((raw_inode)->i_extra_isize) {
>
> And the second "if" decides whether to save bits > 32 in the seconds
> field for 64-bit architectures, so it is also evaluated at compile time.
>
> Better to have this in a macro than in the code itself.
I wanted to see if I could clean this up, and this is what I came up
with. I also did some macro magic to make these macros take one less
argument. I ended up breaking two macros up into three smaller macros
and two inline functions. Does this look any better? (This is
incremental against Kalpak's patch.)
Signed-off-by: Dave Kleikamp <[EMAIL PROTECTED]>
diff -Nurp linux.orig/fs/ext3/inode.c linux/fs/ext3/inode.c
--- linux.orig/fs/ext3/inode.c 2007-02-26 17:10:22.0 -0600
+++ linux/fs/ext3/inode.c 2007-02-26 17:02:28.0 -0600
@@ -2674,10 +2674,10 @@ void ext3_read_inode(struct inode * inod
inode->i_nlink = le16_to_cpu(raw_inode->i_links_count);
inode->i_size = le32_to_cpu(raw_inode->i_size);
- EXT3_INODE_GET_XTIME(i_ctime, i_ctime_extra, inode, raw_inode);
- EXT3_INODE_GET_XTIME(i_mtime, i_mtime_extra, inode, raw_inode);
- EXT3_INODE_GET_XTIME(i_atime, i_atime_extra, inode, raw_inode);
- EXT3_INODE_GET_XTIME(i_crtime, i_crtime_extra, ei, raw_inode);
+ EXT3_INODE_GET_XTIME(i_ctime, inode, raw_inode);
+ EXT3_INODE_GET_XTIME(i_mtime, inode, raw_inode);
+ EXT3_INODE_GET_XTIME(i_atime, inode, raw_inode);
+ EXT3_INODE_GET_XTIME(i_crtime, ei, raw_inode);
ei->i_state = 0;
ei->i_dir_start_lookup = 0;
@@ -2829,10 +2829,10 @@ static int ext3_do_update_inode(handle_t
}
raw_inode->i_links_count = cpu_to_le16(inode->i_nlink);
raw_inode->i_size = cpu_to_le32(ei->i_disksize);
- EXT3_INODE_SET_XTIME(i_ctime, i_ctime_extra, inode, raw_inode);
- EXT3_INODE_SET_XTIME(i_mtime, i_mtime_extra, inode, raw_inode);
- EXT3_INODE_SET_XTIME(i_atime, i_atime_extra, inode, raw_inode);
- EXT3_INODE_SET_XTIME(i_crtime, i_crtime_extra, ei, raw_inode);
+ EXT3_INODE_SET_XTIME(i_ctime, inode, raw_inode);
+ EXT3_INODE_SET_XTIME(i_mtime, inode, raw_inode);
+ EXT3_INODE_SET_XTIME(i_atime, inode, raw_inode);
+ EXT3_INODE_SET_XTIME(i_crtime, ei, raw_inode);
raw_inode->i_blocks = cpu_to_le32(inode->i_blocks);
raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
diff -Nurp linux.orig/include/linux/ext3_fs.h linux/include/linux/ext3_fs.h
--- linux.orig/include/linux/ext3_fs.h 2007-02-26 17:10:22.0 -0600
+++ linux/include/linux/ext3_fs.h 2007-02-26 17:07:20.0 -0600
@@ -331,37 +331,42 @@ struct ext3_inode {
#define EXT3_EPOCH_MASK ((1 << EXT3_EPOCH_BITS) - 1)
#define EXT3_NSEC_MASK (~0UL << EXT3_EPOCH_BITS)
-#define EXT3_INODE_SET_XTIME(xtime, extra_xtime, inode, raw_inode) \
-do { \
+#define EXT3_FITS_IN_INODE(ext3_inode, field) \
+ ((offsetof(typeof(*ext3_inode), field) - \
+ offsetof(typeof(*ext3_inode), i_extra_isize) + \
+ sizeof((ext3_inode)->
Re: [RFC] [PATCH 1/1] nanosecond timestamps
On Feb 25, 2007 02:36 -0800, Andrew Morton wrote:
> > On Fri, 02 Feb 2007 20:09:40 +0530 Kalpak Shah <[EMAIL PROTECTED]> wrote:
> > +#define EXT3_INODE_GET_XTIME(xtime, extra_xtime, inode, raw_inode)
> > \
> > +do {
> > \
> > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime);
> > \
> > + \
> > + if (offsetof(typeof(*raw_inode), extra_xtime) -
> > \
> > + offsetof(typeof(*raw_inode), i_extra_isize) +
> > \
> > + sizeof((raw_inode)->extra_xtime) <=
> > \
> > + le16_to_cpu((raw_inode)->i_extra_isize)) {
> > \
> > + if (sizeof((inode)->xtime.tv_sec) > 4)\
> > + (inode)->xtime.tv_sec |= \
> > + (__u64)(le32_to_cpu((raw_inode)->extra_xtime) & \
> > + EXT3_EPOCH_MASK) << 32; \
> > + (inode)->xtime.tv_nsec = \
> > + (le32_to_cpu((raw_inode)->extra_xtime) & \
> > + EXT3_NSEC_MASK) >> 2; \
> > + }
> > \
> > +} while (0)
>
> ow, my eyes. Can we find a way to do this in C rather than in cpp?
The macro is working on the field names of the inode in most places
(e.g. i_mtime, i_ctime, etc) rather than the values. You _could_ do it
in C, but it would mean moving all of the "offsetof()" into the caller
(basically putting the whole macro in-line at the caller) or doing math
on the pointer addresses at runtime instead of compile time.
It boils down to a check whether the particular nanosecond field is
inside the reserved space in the inode or not, so it ends up a comparison
against a constant. For ctime:
if (4 <= le32_to_cpu((raw_inode)->i_extra_isize) {
And the second "if" decides whether to save bits > 32 in the seconds
field for 64-bit architectures, so it is also evaluated at compile time.
Better to have this in a macro than in the code itself.
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] nanosecond timestamps
> On Fri, 02 Feb 2007 20:09:40 +0530 Kalpak Shah <[EMAIL PROTECTED]> wrote:
> +#define EXT3_INODE_GET_XTIME(xtime, extra_xtime, inode, raw_inode)
> \
> +do {
> \
> + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime);
> \
> + \
> + if (offsetof(typeof(*raw_inode), extra_xtime) -
> \
> + offsetof(typeof(*raw_inode), i_extra_isize) +
> \
> + sizeof((raw_inode)->extra_xtime) <=
> \
> + le16_to_cpu((raw_inode)->i_extra_isize)) {
> \
> + if (sizeof((inode)->xtime.tv_sec) > 4)\
> + (inode)->xtime.tv_sec |= \
> + (__u64)(le32_to_cpu((raw_inode)->extra_xtime) & \
> + EXT3_EPOCH_MASK) << 32; \
> + (inode)->xtime.tv_nsec = \
> + (le32_to_cpu((raw_inode)->extra_xtime) & \
> + EXT3_NSEC_MASK) >> 2; \
> + }
> \
> +} while (0)
ow, my eyes. Can we find a way to do this in C rather than in cpp?
> +static inline struct timespec ext3_current_time(struct inode *inode)
> +{
> + return (inode->i_sb->s_time_gran < 10) ?
NSEC_PER_SEC
> + current_fs_time(inode->i_sb) : CURRENT_TIME_SEC;
> +}
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] nanosecond timestamps
On Mon, Feb 05, 2007 at 11:09:16PM -0500, Theodore Tso wrote: > yet, but one quick comment; the patch looks like it got line-wrapped > by your mail agent (looks like you're using Evolution 2.0). Could you > send it as a text/plain attachment, or otherwise fix your mailer to > not wrap your patches? Ping. Could you please resend the nanosecond patches in a non-whitespace damaged format? Thanks, regards, - Ted - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] Nanosecond timestamps
On Wed, Feb 07, 2007 at 03:05:39PM -0600, Dave Kleikamp wrote: > On Wed, 2007-02-07 at 13:39 -0700, Andreas Dilger wrote: > > 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? > > I think that sparse will catch this. To get the endian checks you need > to do something like this: > > make C=2 CF="-D__CHECK_ENDIAN__"' Indeed: CHECK fs/ext3/super.c fs/ext3/super.c:1787:8: warning: cast to restricted type fs/ext3/super.c:1789:6: warning: cast to restricted type fs/ext3/super.c:1791:8: warning: cast to restricted type fs/ext3/super.c:1793:6: warning: cast to restricted type Thanks, Johann - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] Nanosecond timestamps
On Wed, Feb 07, 2007 at 01:39:46PM -0700, Andreas Dilger wrote: > 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? That would be great. > 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)) Very good idea! > The only question is whether "typecheck" adds extra variables on the stack > or if the compiler will always optimize them away. I tend to think it will always be optimized by the compiler. > > 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. Just a useless 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. I agree. Cheers, Johann - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] Nanosecond timestamps
On Wed, 2007-02-07 at 13:39 -0700, Andreas Dilger wrote:
> On Feb 06, 2007 16:12 +0100, Johann Lombardi wrote:
> > > + 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?
I think that sparse will catch this. To get the endian checks you need
to do something like this:
make C=2 CF="-D__CHECK_ENDIAN__"'
--
David Kleikamp
IBM Linux Technology Center
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] Nanosecond timestamps
On Fri, Feb 02, 2007 at 08:19:50PM +0530, Kalpak Shah wrote:
> +#define EXT3_INODE_SET_XTIME(xtime, extra_xtime, inode, raw_inode) \
> +do { \
> + (raw_inode)->xtime = cpu_to_le32((inode)->xtime.tv_sec); \
> + \
> + if (offsetof(typeof(*raw_inode), extra_xtime) - \
> + offsetof(typeof(*raw_inode), i_extra_isize) + \
> + sizeof((raw_inode)->extra_xtime) <= \
> + le16_to_cpu((raw_inode)->i_extra_isize)) \
^
> + (raw_inode)->extra_xtime = \
With 128-byte inodes, raw_inode->i_extra_isize is beyond the inode limit
and the above will corrupt the filesystem.
IMO, i_extra_isize from ext3_inode_info should be used instead of
raw_inode->i_extra_isize.
> +#define EXT3_INODE_GET_XTIME(xtime, extra_xtime, inode, raw_inode) \
> +do { \
> + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \
> + \
> + if (offsetof(typeof(*raw_inode), extra_xtime) - \
> + offsetof(typeof(*raw_inode), i_extra_isize) + \
> + sizeof((raw_inode)->extra_xtime) <= \
> + le16_to_cpu((raw_inode)->i_extra_isize)) { \
^
ditto
Cheers,
Johann
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] nanosecond timestamps
On Feb 05, 2007 23:09 -0500, Theodore Tso wrote:
> On Fri, Feb 02, 2007 at 08:09:40PM +0530, Kalpak Shah wrote:
> > This patch is a spinoff of the old nanosecond patches. It includes some
> > cleanups and addition of a creation timestamp. The
> > EXT3_FEATURE_RO_COMPAT_EXTRA_ISIZE flag has also been added along with
> > s_{min, want}_extra_isize fields in struct ext3_super_block.
>
> Thanks for sending it. I haven't had a chance to go over it in detail
> yet, but one quick comment; the patch looks like it got line-wrapped
> by your mail agent (looks like you're using Evolution 2.0). Could you
> send it as a text/plain attachment, or otherwise fix your mailer to
> not wrap your patches?
>
> It might be nice if the patch had a way of adjusting the granularity
> by masking off bits of the nanoseconds field, so we can benchmark how
> much overhead constantly updating the ctime field is going to cost us.
Can anyone suggest a benchmark which will test this area? Bull had done
some testing with the inode version patch (it also forces an update for
every change to an inode) and reported no noticable performance loss.
That could have been because of CPU headroom available to do repeat copies
of the in-core inode to the on-disk inode, which may hurt in a more CPU
constrained environment (other server code, multiple filesystems, etc).
Before we go to changing the patch, we may as well start by just testing
before vs. after patch (still using large inodes, for consistency).
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] Nanosecond timestamps
On Fri, Feb 02, 2007 at 08:19:50PM +0530, Kalpak Shah wrote:
> Index: linux-2.6.19/fs/ext3/super.c
> ===
> --- linux-2.6.19.orig/fs/ext3/super.c
> +++ linux-2.6.19/fs/ext3/super.c
> @@ -1770,6 +1772,32 @@ static int ext3_fill_super (struct super
> }
>
> ext3_setup_super (sb, es, sb->s_flags & MS_RDONLY);
> +
> + /* determine the minimum size of new large inodes, if present */
> + 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.
> + if (EXT3_HAS_RO_COMPAT_FEATURE(sb,
> + EXT3_FEATURE_RO_COMPAT_EXTRA_ISIZE)) {
> + if (EXT3_SB(sb)->s_want_extra_isize <
> + le32_to_cpu(es->s_want_extra_isize))
^^
> + EXT3_SB(sb)->s_want_extra_isize =
> + le32_to_cpu(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().
> + }
> + }
> + /* 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().
Johann
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH 1/1] nanosecond timestamps
On Fri, Feb 02, 2007 at 08:09:40PM +0530, Kalpak Shah wrote:
> Hi,
>
> This patch is a spinoff of the old nanosecond patches. It includes some
> cleanups and addition of a creation timestamp. The
> EXT3_FEATURE_RO_COMPAT_EXTRA_ISIZE flag has also been added along with
> s_{min, want}_extra_isize fields in struct ext3_super_block.
Thanks for sending it. I haven't had a chance to go over it in detail
yet, but one quick comment; the patch looks like it got line-wrapped
by your mail agent (looks like you're using Evolution 2.0). Could you
send it as a text/plain attachment, or otherwise fix your mailer to
not wrap your patches?
It might be nice if the patch had a way of adjusting the granularity
by masking off bits of the nanoseconds field, so we can benchmark how
much overhead constantly updating the ctime field is going to cost us.
Regards,
- Ted
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
