Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Tue, 2007-07-17 at 15:29 +0530, Kalpak Shah wrote: > On Mon, 2007-07-16 at 17:49 -0700, Mingming Cao wrote: > > On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > > > On Sun, 01 Jul 2007 03:36:56 -0400 > > > Mingming Cao <[EMAIL PROTECTED]> wrote: > > > > +static inline __le32 ext4_encode_extra_time(struct timespec *time) > > > > +{ > > > > + return cpu_to_le32((sizeof(time->tv_sec) > 4 ? > > > > + time->tv_sec >> 32 : 0) | > > > > + ((time->tv_nsec << 2) & EXT4_NSEC_MASK)); > > > > +} > > > > + > > > > +static inline void ext4_decode_extra_time(struct timespec *time, > > > > __le32 extra) > > > > +{ > > > > + if (sizeof(time->tv_sec) > 4) > > > > + time->tv_sec |= (__u64)(le32_to_cpu(extra) & > > > > EXT4_EPOCH_MASK) > > > > + << 32; > > > > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> 2; > > > > +} > > > > > > Consider uninlining these functions. > > > > > I got compile warining after apply Kalpal's update nanosecond patch, > > which makes these two function inline. It complains these functions are > > defined but not used. It's being used only in the following > > micros(EXT4_INODE_SET_XTIME etc). So if the .c file included the > > ext4_fs.h but not using the micros, the compile will think these two > > functions are not used. > > The compile warnings were introduced because the functions were > uninlined. So we can either keep these functions inlined or consider > adding a "__used" attribute to these two functions. > okay for now I keep these functions inlined. > Thanks, > Kalpak. > - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Mon, 2007-07-16 at 17:49 -0700, Mingming Cao wrote: > On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > > On Sun, 01 Jul 2007 03:36:56 -0400 > > Mingming Cao <[EMAIL PROTECTED]> wrote: > > > +static inline __le32 ext4_encode_extra_time(struct timespec *time) > > > +{ > > > + return cpu_to_le32((sizeof(time->tv_sec) > 4 ? > > > +time->tv_sec >> 32 : 0) | > > > +((time->tv_nsec << 2) & EXT4_NSEC_MASK)); > > > +} > > > + > > > +static inline void ext4_decode_extra_time(struct timespec *time, __le32 > > > extra) > > > +{ > > > + if (sizeof(time->tv_sec) > 4) > > > +time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > > > +<< 32; > > > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> 2; > > > +} > > > > Consider uninlining these functions. > > > I got compile warining after apply Kalpal's update nanosecond patch, > which makes these two function inline. It complains these functions are > defined but not used. It's being used only in the following > micros(EXT4_INODE_SET_XTIME etc). So if the .c file included the > ext4_fs.h but not using the micros, the compile will think these two > functions are not used. The compile warnings were introduced because the functions were uninlined. So we can either keep these functions inlined or consider adding a "__used" attribute to these two functions. Thanks, Kalpak. - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > On Sun, 01 Jul 2007 03:36:56 -0400 > Mingming Cao <[EMAIL PROTECTED]> wrote: > > > This patch is a spinoff of the old nanosecond patches. > > I don't know what the "old nanosecond patches" are. A link to a suitable > changlog for those patches would do in a pinch. Preferable would be to > write a proper changelog for this patch. > > > 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. > > > > Signed-off-by: Andreas Dilger <[EMAIL PROTECTED]> > > Signed-off-by: Kalpak Shah <[EMAIL PROTECTED]> > > Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]> > > Signed-off-by: Dave Kleikamp <[EMAIL PROTECTED]> > > Signed-off-by: Mingming Cao <[EMAIL PROTECTED]> > > > > Index: linux-2.6.22-rc4/fs/ext4/ialloc.c > > Please include diffstat output when preparing patches. > > > + > > +#define EXT4_FITS_IN_INODE(ext4_inode, einode, field) \ > > + ((offsetof(typeof(*ext4_inode), field) +\ > > + sizeof((ext4_inode)->field)) \ > > + <= (EXT4_GOOD_OLD_INODE_SIZE + \ > > + (einode)->i_extra_isize)) \ > > Please add explanatory commentary to EXT4_FITS_IN_INODE(): tell readers > under what circumstances something will not fit in an inode and what the > consequences of this are. > > > +static inline __le32 ext4_encode_extra_time(struct timespec *time) > > +{ > > + return cpu_to_le32((sizeof(time->tv_sec) > 4 ? > > + time->tv_sec >> 32 : 0) | > > + ((time->tv_nsec << 2) & EXT4_NSEC_MASK)); > > +} > > + > > +static inline void ext4_decode_extra_time(struct timespec *time, __le32 > > extra) > > +{ > > + if (sizeof(time->tv_sec) > 4) > > + time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > > + << 32; > > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> 2; > > +} > > Consider uninlining these functions. > I got compile warining after apply Kalpal's update nanosecond patch, which makes these two function inline. It complains these functions are defined but not used. It's being used only in the following micros(EXT4_INODE_SET_XTIME etc). So if the .c file included the ext4_fs.h but not using the micros, the compile will think these two functions are not used. Mingming > > +#define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) > >\ > > +do { > >\ > > + (raw_inode)->xtime = cpu_to_le32((inode)->xtime.tv_sec); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > > + (raw_inode)->xtime ## _extra = \ > > + ext4_encode_extra_time(&(inode)->xtime); \ > > +} while (0) > > + > > +#define EXT4_EINODE_SET_XTIME(xtime, einode, raw_inode) > >\ > > +do { > >\ > > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > > + (raw_inode)->xtime = cpu_to_le32((einode)->xtime.tv_sec); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > > + (raw_inode)->xtime ## _extra = \ > > + ext4_encode_extra_time(&(einode)->xtime); \ > > +} while (0) > > + > > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) > >\ > > +do { > >\ > > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > > + ext4_decode_extra_time(&(inode)->xtime,\ > > + raw_inode->xtime ## _extra);\ > > +} while (0) > > + > > +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) > >\ > > +do { > >\ > > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > > + (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > > + ext4_decode_extra_time(&(einode)->xtime, \ > > + raw_inode->xtime ## _extra);\ > > +} while (0) > > Ugly. I expect these could be implemented as plain old C functions. > Caller could pass in the address of the ext4_inode field which the function > is to operate upon. > > > #if defined(__KERNEL__)
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Fri, 2007-07-13 at 12:35 +0530, Kalpak Shah wrote: > On Fri, 2007-07-13 at 09:59 +0530, Aneesh Kumar K.V wrote: > > > > Kalpak Shah wrote: > > > On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > > >> On Sun, 01 Jul 2007 03:36:56 -0400 > > >> Mingming Cao <[EMAIL PROTECTED]> wrote: > > >> > > >>> This patch is a spinoff of the old nanosecond patches. > > >> I don't know what the "old nanosecond patches" are. A link to a suitable > > >> changlog for those patches would do in a pinch. Preferable would be to > > >> write a proper changelog for this patch. > > > > > > The incremental patch contains a proper changelog describing the patch. > > > > > > > > > Instead of putting incremental patches it would be nice if we can have > > replacement patches. > > for the already existing patches with the comments addressed. For example > > if we have a > > review comment on the patch message ( commit log ) then adding an > > incremental patch doesn't help. > > I think that it would be easier to review just the changes that have > been made to the patches instead of having people go through the entire > patch again. I was hoping that someone with write access to ext4-git > would update the commit logs. > > If replacement patches are preferred, then I will send them again. > No need, I already fold your fix patch to the parent patches, so in the updated ext4-patch-queue it saved the updated nanosecond patch. > Thanks, > Kalpak. > > > > > > > -aneesh > > - > > 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 > - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Fri, 2007-07-13 at 09:59 +0530, Aneesh Kumar K.V wrote: > > Kalpak Shah wrote: > > On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > >> On Sun, 01 Jul 2007 03:36:56 -0400 > >> Mingming Cao <[EMAIL PROTECTED]> wrote: > >> > >>> This patch is a spinoff of the old nanosecond patches. > >> I don't know what the "old nanosecond patches" are. A link to a suitable > >> changlog for those patches would do in a pinch. Preferable would be to > >> write a proper changelog for this patch. > > > > The incremental patch contains a proper changelog describing the patch. > > > > > Instead of putting incremental patches it would be nice if we can have > replacement patches. > for the already existing patches with the comments addressed. For example if > we have a > review comment on the patch message ( commit log ) then adding an incremental > patch doesn't help. I think that it would be easier to review just the changes that have been made to the patches instead of having people go through the entire patch again. I was hoping that someone with write access to ext4-git would update the commit logs. If replacement patches are preferred, then I will send them again. Thanks, Kalpak. > > > -aneesh > - > 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 - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
Kalpak Shah wrote: On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: On Sun, 01 Jul 2007 03:36:56 -0400 Mingming Cao <[EMAIL PROTECTED]> wrote: This patch is a spinoff of the old nanosecond patches. I don't know what the "old nanosecond patches" are. A link to a suitable changlog for those patches would do in a pinch. Preferable would be to write a proper changelog for this patch. The incremental patch contains a proper changelog describing the patch. Instead of putting incremental patches it would be nice if we can have replacement patches. for the already existing patches with the comments addressed. For example if we have a review comment on the patch message ( commit log ) then adding an incremental patch doesn't help. -aneesh - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > On Sun, 01 Jul 2007 03:36:56 -0400 > Mingming Cao <[EMAIL PROTECTED]> wrote: > > > This patch is a spinoff of the old nanosecond patches. > > I don't know what the "old nanosecond patches" are. A link to a suitable > changlog for those patches would do in a pinch. Preferable would be to > write a proper changelog for this patch. The incremental patch contains a proper changelog describing the patch. > > +static inline __le32 ext4_encode_extra_time(struct timespec *time) > > +{ > > + return cpu_to_le32((sizeof(time->tv_sec) > 4 ? > > + time->tv_sec >> 32 : 0) | > > + ((time->tv_nsec << 2) & EXT4_NSEC_MASK)); > > +} > > + > > +static inline void ext4_decode_extra_time(struct timespec *time, __le32 > > extra) > > +{ > > + if (sizeof(time->tv_sec) > 4) > > + time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > > + << 32; > > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> 2; > > +} > > Consider uninlining these functions. Done. This patch adds comments, few small corrections and an appropriate changelog entry. Thanks, Kalpak. This patch adds nanosecond timestamps for ext4. This involves adding *time_extra fields to the ext4_inode to extend the timestamps to 64-bits. Creation time is also added by this patch. These extended fields will fit into an inode if the filesystem was formatted with large inodes (-I 256 or larger) and there are currently no EAs consuming all of the available space. For new inodes we always reserve enough space for the kernel's known extended fields, but for inodes created with an old kernel this might not have been the case. So this patch also adds the EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE feature flag(ro-compat so that older kernels can't create inodes with a smaller extra_isize). which indicates if the fields fitting inside s_min_extra_isize are available or not. If the expansion of inodes if unsuccessful then this feature will be disabled. This feature is only enabled if requested by the sysadmin. None of the extended inode fields is critical for correct filesystem operation. Signed-off-by: Andreas Dilger <[EMAIL PROTECTED]> Signed-off-by: Kalpak Shah <[EMAIL PROTECTED]> Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]> Signed-off-by: Dave Kleikamp <[EMAIL PROTECTED]> Signed-off-by: Mingming Cao <[EMAIL PROTECTED]> Index: linux-2.6.22/include/linux/ext4_fs.h === --- linux-2.6.22.orig/include/linux/ext4_fs.h +++ linux-2.6.22/include/linux/ext4_fs.h @@ -350,20 +350,30 @@ struct ext4_inode { #define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1) #define EXT4_NSEC_MASK (~0UL << EXT4_EPOCH_BITS) +/* + * Extended fields will fit into an inode if the filesystem was formatted + * with large inodes (-I 256 or larger) and there are not currently any EAs + * consuming all of the available space. For new inodes we always reserve + * enough space for the kernel's known extended fields, but for inodes + * created with an old kernel this might not have been the case. None of + * the extended inode fields is critical for correct filesystem operation. + * This macro checks if a certain field fits in the inode. Note that + * inode-size = GOOD_OLD_INODE_SIZE + i_extra_isize + */ #define EXT4_FITS_IN_INODE(ext4_inode, einode, field) \ ((offsetof(typeof(*ext4_inode), field) + \ sizeof((ext4_inode)->field)) \ <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ -static inline __le32 ext4_encode_extra_time(struct timespec *time) +static __le32 ext4_encode_extra_time(struct timespec *time) { return cpu_to_le32((sizeof(time->tv_sec) > 4 ? time->tv_sec >> 32 : 0) | ((time->tv_nsec << 2) & EXT4_NSEC_MASK)); } -static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) +static void ext4_decode_extra_time(struct timespec *time, __le32 extra) { if (sizeof(time->tv_sec) > 4) time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) Index: linux-2.6.22/include/linux/ext4_fs_i.h === --- linux-2.6.22.orig/include/linux/ext4_fs_i.h +++ linux-2.6.22/include/linux/ext4_fs_i.h @@ -153,6 +153,10 @@ struct ext4_inode_info { unsigned long i_ext_generation; struct ext4_ext_cache i_cached_extent; + /* + * File creation time. Its function is same as that of + * struct timespec i_{a,c,m}time in the generic inode. + */ struct timespec i_crtime; };
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Jul 10, 2007 16:30 -0700, Andrew Morton wrote: > > +#define EXT4_FITS_IN_INODE(ext4_inode, einode, field) \ > > + ((offsetof(typeof(*ext4_inode), field) +\ > > + sizeof((ext4_inode)->field)) \ > > + <= (EXT4_GOOD_OLD_INODE_SIZE + \ > > + (einode)->i_extra_isize)) \ > > Please add explanatory commentary to EXT4_FITS_IN_INODE(): tell readers > under what circumstances something will not fit in an inode and what the > consequences of this are. /* Extended fields will fit into an inode if the filesystem was formatted * with large inodes (-I 256 or larger) and there are not currently EAs * consuming all of the available space. For new inodes we always reserve * enough space for the kernel's known extended fields, but for inodes * created with an old kernel this might not have been the case. None of * the extended inode fields is critical for correct filesystem operation. */ > > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) > >\ > > +do { > >\ > > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > > + ext4_decode_extra_time(&(inode)->xtime,\ > > + raw_inode->xtime ## _extra);\ > > +} while (0) > > Ugly. I expect these could be implemented as plain old C functions. > Caller could pass in the address of the ext4_inode field which the function > is to operate upon. We thought about that also, but then the caller needs to do all of the pointer gymnastics themselves like: ext4_inode_get_xtime(&inode->i_ctime, &inode->i_ctime_extra, &raw_inode->i_ctime, &raw_inode->i_ctime_extra) instead of the current: EXT4_INODE_GET_XTIME(ctime, inode, raw_inode); IMHO it is preferrable to make the multiple callsites more readable than the macros. > > #if defined(__KERNEL__) || defined(__linux__) > > (What's the __linux__ for?) > > > #define i_reserved1osd1.linux1.l_i_reserved1 > > #define i_frag osd2.linux2.l_i_frag This is actually unrelated to the current patch, just part of the context. AFAIK, this is historical, so that the kernel and e2fsprogs can use the same ext2_fs.h header. I don't think it is really needed, but such cleanup shouldn't be a part of this patch either. > > +static inline struct timespec ext4_current_time(struct inode *inode) > > +{ > > + return (inode->i_sb->s_time_gran < NSEC_PER_SEC) ? > > + current_fs_time(inode->i_sb) : CURRENT_TIME_SEC; > > +} > > Now, I've forgotten how this works. Remind me, please. Can ext4 > filesystems ever have one-second timestamp granularity? If so, how does > one cause that to come about? Yes, this is possible if an ext2/3/4 filesystem is formatted with 128-byte inodes (which is the default for all but ext4) and this fs is mounted as ext4dev. The inodes can never hold the extra time information (FITS_IN_INODE check above) so the superblock limits the timestamp resolution to 1s in that case. > > @@ -153,6 +153,7 @@ > > > > unsigned long i_ext_generation; > > struct ext4_ext_cache i_cached_extent; > > + struct timespec i_crtime; > > }; > > It is unobvious what this field does. Please prefer to add commentary to > _all_ struct fields - it really helps. It is the inode creation time. This is useful for debug/forensic purposes, and at some point there will be an API so that Samba can use it also. > > #endif /* _LINUX_EXT4_FS_I */ > > Index: linux-2.6.22-rc4/include/linux/ext4_fs_sb.h > > === > > --- linux-2.6.22-rc4.orig/include/linux/ext4_fs_sb.h2007-06-11 > > 17:28:15.0 -0700 > > +++ linux-2.6.22-rc4/include/linux/ext4_fs_sb.h 2007-06-11 > > 17:39:05.0 -0700 > > @@ -79,6 +79,7 @@ > > char *s_qf_names[MAXQUOTAS];/* Names of quota files with > > journalled quota */ > > int s_jquota_fmt; /* Format of quota to use */ > > #endif > > + unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */ > > OK, I can kind-of see how this is working, but some overall description of > how the inode sizing design operates would be helpful. It would certainly > make reviewing of this proposed change more fruitful. Perhaps that new > comment over EXT4_FITS_IN_INODE() would be a suitable place. Hmm, I'm sure there were emails on the topic, but they aren't attached to the patch. s_want_extra_isize is just an override for sizeof(ext4_inode) in case the sysadmin wants to reserve more fields in new inodes. There is also s_min_extra_isize which is what the kernel and e2fsck guarantee that will be available in all in-use inodes, if RO_COMPAT_EXTRA_
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Jul 10, 2007 22:00 -0400, Mingming Cao wrote: > On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > > On Sun, 01 Jul 2007 03:36:56 -0400 > > Mingming Cao <[EMAIL PROTECTED]> wrote: > > > > > This patch is a spinoff of the old nanosecond patches. > > > > I don't know what the "old nanosecond patches" are. A link to a suitable > > changlog for those patches would do in a pinch. Preferable would be to > > write a proper changelog for this patch. > > > I found the original patch > http://marc.info/?l=linux-ext4&m=115091699809181&w=2 > > Andreas or Kalpak, is changelog from the original patch is accurate to > apply here? Mostly, yes, but the name of the feature flag has changed. Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Tue, 2007-07-10 at 16:30 -0700, Andrew Morton wrote: > On Sun, 01 Jul 2007 03:36:56 -0400 > Mingming Cao <[EMAIL PROTECTED]> wrote: > > > This patch is a spinoff of the old nanosecond patches. > > I don't know what the "old nanosecond patches" are. A link to a suitable > changlog for those patches would do in a pinch. Preferable would be to > write a proper changelog for this patch. > I found the original patch http://marc.info/?l=linux-ext4&m=115091699809181&w=2 Andreas or Kalpak, is changelog from the original patch is accurate to apply here? Mingming - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Sun, 01 Jul 2007 03:36:56 -0400 Mingming Cao <[EMAIL PROTECTED]> wrote: > This patch is a spinoff of the old nanosecond patches. I don't know what the "old nanosecond patches" are. A link to a suitable changlog for those patches would do in a pinch. Preferable would be to write a proper changelog for this patch. > 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. > > Signed-off-by: Andreas Dilger <[EMAIL PROTECTED]> > Signed-off-by: Kalpak Shah <[EMAIL PROTECTED]> > Signed-off-by: Eric Sandeen <[EMAIL PROTECTED]> > Signed-off-by: Dave Kleikamp <[EMAIL PROTECTED]> > Signed-off-by: Mingming Cao <[EMAIL PROTECTED]> > > Index: linux-2.6.22-rc4/fs/ext4/ialloc.c Please include diffstat output when preparing patches. > + > +#define EXT4_FITS_IN_INODE(ext4_inode, einode, field)\ > + ((offsetof(typeof(*ext4_inode), field) +\ > + sizeof((ext4_inode)->field)) \ > + <= (EXT4_GOOD_OLD_INODE_SIZE + \ > + (einode)->i_extra_isize)) \ Please add explanatory commentary to EXT4_FITS_IN_INODE(): tell readers under what circumstances something will not fit in an inode and what the consequences of this are. > +static inline __le32 ext4_encode_extra_time(struct timespec *time) > +{ > + return cpu_to_le32((sizeof(time->tv_sec) > 4 ? > +time->tv_sec >> 32 : 0) | > +((time->tv_nsec << 2) & EXT4_NSEC_MASK)); > +} > + > +static inline void ext4_decode_extra_time(struct timespec *time, __le32 > extra) > +{ > + if (sizeof(time->tv_sec) > 4) > +time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > +<< 32; > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> 2; > +} Consider uninlining these functions. > +#define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) >\ > +do {\ > + (raw_inode)->xtime = cpu_to_le32((inode)->xtime.tv_sec); \ > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > + (raw_inode)->xtime ## _extra = \ > + ext4_encode_extra_time(&(inode)->xtime); \ > +} while (0) > + > +#define EXT4_EINODE_SET_XTIME(xtime, einode, raw_inode) >\ > +do {\ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > + (raw_inode)->xtime = cpu_to_le32((einode)->xtime.tv_sec); \ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > + (raw_inode)->xtime ## _extra = \ > + ext4_encode_extra_time(&(einode)->xtime); \ > +} while (0) > + > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) >\ > +do {\ > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > + ext4_decode_extra_time(&(inode)->xtime,\ > +raw_inode->xtime ## _extra);\ > +} while (0) > + > +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) >\ > +do {\ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > + (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > + ext4_decode_extra_time(&(einode)->xtime, \ > +raw_inode->xtime ## _extra);\ > +} while (0) Ugly. I expect these could be implemented as plain old C functions. Caller could pass in the address of the ext4_inode field which the function is to operate upon. > #if defined(__KERNEL__) || defined(__linux__) (What's the __linux__ for?) > #define i_reserved1 osd1.linux1.l_i_reserved1 > #define i_frag osd2.linux2.l_i_frag > @@ -539,6 +603,13 @@ > return container_of(inode, struct ext4_inode_info, vfs_inode); > } > > +static inline struct timespec ext4_current_time(struct inode *inode) > +{ > + return (inode->i_sb->s_time_gran < NSEC_PER_SEC) ? > + current_fs_time(inode->i_sb) : CURRENT_TIME_SEC; > +} Now, I've forgotten how this works. Remind me, please. Can ext4 filesystems ever have one-second timestamp granularity? If so, how
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Jul 04, 2007 12:06 +0530, Aneesh Kumar K.V wrote: > Mingming Cao wrote: > >On Tue, 2007-07-03 at 15:58 +0530, Kalpak Shah wrote: > >>On Sun, 2007-07-01 at 03:36 -0400, Mingming Cao wrote: > >>>+ > >>>+#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) \ > >>>+do { \ > >>>+ (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); > >>>\ > >>>+ if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) > >>>\ > >>>+ ext4_decode_extra_time(&(inode)->xtime, > >>>\ > >>>+ raw_inode->xtime ## _extra); > >>>\ > >>>+} while (0) > >>>+ > >>>+#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode)\ > >>>+do { \ > >>>+ if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) > >>>\ > >>>+ (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); > >>>\ > >>>+ if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra)) > >>>\ > >>>+ ext4_decode_extra_time(&(einode)->xtime, > >>>\ > >>>+ raw_inode->xtime ## _extra); > >>>\ > >>>+} while (0) > >>>+ > >>This nanosecond patch seems to be missing the fix below which is > >>required for http://bugzilla.kernel.org/show_bug.cgi?id=5079 > >> > >>If the timestamp is set to before epoch i.e. a negative timestamp then > >>the file may have its date set into the future on 64-bit systems. So > >>when the timestamp is read it must be cast as signed. > > > >Missed this one. > >Thanks. Will update ext4 patch queue tonight with this fix. > > IIRC in the conference call it was decided to not to apply this patch. > Andreas may be able to update better. I wasn't on the most recent concall, and I've forgotten the details of any discussion on a previous concall. Care really needs to be taken here that negative timestamps are handled properly. We can take the sign bit from the inode i_*time, but then we need to change the load/save of the extra time to use a shift of 31 instead of 32. If we overflow the epoch we have to ensure that the high bits of the seconds is handled correctly. Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
Aneesh Kumar K.V wrote: Mingming Cao wrote: On Tue, 2007-07-03 at 15:58 +0530, Kalpak Shah wrote: On Sun, 2007-07-01 at 03:36 -0400, Mingming Cao wrote: + +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) \ +do { \ +(inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ +if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ +ext4_decode_extra_time(&(inode)->xtime, \ + raw_inode->xtime ## _extra); \ +} while (0) + +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) \ +do { \ +if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ +(einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ +if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra)) \ +ext4_decode_extra_time(&(einode)->xtime, \ + raw_inode->xtime ## _extra); \ +} while (0) + This nanosecond patch seems to be missing the fix below which is required for http://bugzilla.kernel.org/show_bug.cgi?id=5079 If the timestamp is set to before epoch i.e. a negative timestamp then the file may have its date set into the future on 64-bit systems. So when the timestamp is read it must be cast as signed. Missed this one. Thanks. Will update ext4 patch queue tonight with this fix. IIRC in the conference call it was decided to not to apply this patch. Andreas may be able to update better. Looking at the git log i understand the core patch got applied to ext4 tree with the comment from Andreas. So may be we can apply this patch also. commit 4d7bf11d649c72621ca31b8ea12b9c94af380e63 Andreas says: This patch is now treating timestamps with the high bit set as negative times (before Jan 1, 1970). This means we lose 1/2 of the possible range of timestamps (lopping off 68 years before unix timestamp overflow - now only 30 years away :-) to handle the extremely rare case of setting timestamps into the distant past. If we are only interested in fixing the underflow case, we could just limit the values to 0 instead of storing negative values. At worst this will skew the timestamp by a few hours for timezones in the far east (files would still show Jan 1, 1970 in "ls -l" output). That said, it seems 32-bit systems (mine at least) allow files to be set into the past (01/01/1907 works fine) so it seems this patch is bringing the x86_64 behaviour into sync with other kernels. On the plus side, we have a patch that is ready to add nanosecond timestamps to ext3 and as an added bonus adds 2 high bits to the on-disk timestamp so this extends the maximum date to 2242. NOTE: The conference call i mentioned above is http://ext4.wiki.kernel.org/index.php/Ext4_Developer%27s_Conference_Call -aneesh - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
Mingming Cao wrote: On Tue, 2007-07-03 at 15:58 +0530, Kalpak Shah wrote: On Sun, 2007-07-01 at 03:36 -0400, Mingming Cao wrote: + +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) \ +do { \ + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ + ext4_decode_extra_time(&(inode)->xtime, \ + raw_inode->xtime ## _extra); \ +} while (0) + +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) \ +do { \ + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ + (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ + ext4_decode_extra_time(&(einode)->xtime,\ + raw_inode->xtime ## _extra); \ +} while (0) + This nanosecond patch seems to be missing the fix below which is required for http://bugzilla.kernel.org/show_bug.cgi?id=5079 If the timestamp is set to before epoch i.e. a negative timestamp then the file may have its date set into the future on 64-bit systems. So when the timestamp is read it must be cast as signed. Missed this one. Thanks. Will update ext4 patch queue tonight with this fix. IIRC in the conference call it was decided to not to apply this patch. Andreas may be able to update better. -aneesh - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Tue, 2007-07-03 at 15:58 +0530, Kalpak Shah wrote: > On Sun, 2007-07-01 at 03:36 -0400, Mingming Cao wrote: > > + > > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) > >\ > > +do { > >\ > > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > > + ext4_decode_extra_time(&(inode)->xtime,\ > > + raw_inode->xtime ## _extra);\ > > +} while (0) > > + > > +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) > >\ > > +do { > >\ > > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > > + (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > > + ext4_decode_extra_time(&(einode)->xtime, \ > > + raw_inode->xtime ## _extra);\ > > +} while (0) > > + > > This nanosecond patch seems to be missing the fix below which is required for > http://bugzilla.kernel.org/show_bug.cgi?id=5079 > > If the timestamp is set to before epoch i.e. a negative timestamp then the > file may have its date set into the future on 64-bit systems. So when the > timestamp is read it must be cast as signed. Missed this one. Thanks. Will update ext4 patch queue tonight with this fix. Mingming - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Sun, 2007-07-01 at 03:36 -0400, Mingming Cao wrote: > + > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) >\ > +do {\ > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > + ext4_decode_extra_time(&(inode)->xtime,\ > +raw_inode->xtime ## _extra);\ > +} while (0) > + > +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) >\ > +do {\ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > + (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > + ext4_decode_extra_time(&(einode)->xtime, \ > +raw_inode->xtime ## _extra);\ > +} while (0) > + This nanosecond patch seems to be missing the fix below which is required for http://bugzilla.kernel.org/show_bug.cgi?id=5079 If the timestamp is set to before epoch i.e. a negative timestamp then the file may have its date set into the future on 64-bit systems. So when the timestamp is read it must be cast as signed. Index: linux-2.6.21/include/linux/ext4_fs.h === --- linux-2.6.21.orig/include/linux/ext4_fs.h +++ linux-2.6.21/include/linux/ext4_fs.h @@ -390,7 +390,7 @@ do { \ #define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) \ do { \ - (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ + (inode)->xtime.tv_sec = (signed)le32_to_cpu((raw_inode)->xtime); \ if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ ext4_decode_extra_time(&(inode)->xtime,\ raw_inode->xtime ## _extra);\ @@ -399,7 +399,8 @@ do { \ #define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) \ do { \ if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ - (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ + (einode)->xtime.tv_sec = \ + (signed)le32_to_cpu((raw_inode)->xtime); \ if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ ext4_decode_extra_time(&(einode)->xtime, \ raw_inode->xtime ## _extra);\ Thanks, Kalpak. - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
On Sun, 2007-07-01 at 03:36 -0400, Mingming Cao wrote: > + > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) >\ > +do {\ > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ > + ext4_decode_extra_time(&(inode)->xtime,\ > +raw_inode->xtime ## _extra);\ > +} while (0) > + > +#define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) >\ > +do {\ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ > + (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ > + if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ > + ext4_decode_extra_time(&(einode)->xtime, \ > +raw_inode->xtime ## _extra);\ > +} while (0) > + This nanosecond patch seems to be missing the fix below which is required for http://bugzilla.kernel.org/show_bug.cgi?id=5079 If the timestamp is set to before epoch i.e. a negative timestamp then the file may have its date set into the future on 64-bit systems. So when the timestamp is read it must be cast as signed. Index: linux-2.6.21/include/linux/ext4_fs.h === --- linux-2.6.21.orig/include/linux/ext4_fs.h +++ linux-2.6.21/include/linux/ext4_fs.h @@ -390,7 +390,7 @@ do { \ #define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) \ do { \ - (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ + (inode)->xtime.tv_sec = (signed)le32_to_cpu((raw_inode)->xtime); \ if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \ ext4_decode_extra_time(&(inode)->xtime,\ raw_inode->xtime ## _extra);\ @@ -399,7 +399,8 @@ do { \ #define EXT4_EINODE_GET_XTIME(xtime, einode, raw_inode) \ do { \ if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime)) \ - (einode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \ + (einode)->xtime.tv_sec = \ + (signed)le32_to_cpu((raw_inode)->xtime); \ if (EXT4_FITS_IN_INODE(raw_inode, einode, xtime ## _extra))\ ext4_decode_extra_time(&(einode)->xtime, \ raw_inode->xtime ## _extra);\ Thanks, Kalpak. - 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
Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp
Mingming Cao 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 Should be EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE -aneesh - 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