LinuxLists.cc - [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp

[permalink] [raw]

Subject: 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

2007-07-03 10:41:15

[permalink] [raw]

Subject: 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_ino > + 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
If the timestamp is set to before epoch i.e. a negative timestamp
Index: linux-2.6.21/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.

\
\
\
\
\
\
\
\
\
de)->xtime); \
\
\
\
is required for http://bugzilla.kernel.org/show_bug.cgi?id=5079
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.
/> \
\
\
\
\
\
\
\
\
\
\
\
\
\
\
\
\
\

2007-07-03 10:28:26

[permalink] [raw]

Subject: 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.

2007-07-04 06:32:54

[permalink] [raw]

Subject: 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

2007-07-04 06:36:10

[permalink] [raw]

Subject: 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

2007-07-04 07:24:18

[permalink] [raw]

Subject: 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

2007-07-04 16:44:32

by Andreas Dilger

[permalink] [raw]

Subject: 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.

2007-07-10 23:30:42

by Andrew Morton

[permalink] [raw]

Subject: 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 does
one cause that to come about?

> --- linux-2.6.22-rc4.orig/include/linux/ext4_fs_i.h 2007-06-11 17:22:15.000000000 -0700
> +++ linux-2.6.22-rc4/include/linux/ext4_fs_i.h 2007-06-11 17:39:05.000000000 -0700
> @@ -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.

I thought checkpatch was going to have a little whine about that but the
version I have here doesn't.

>
> #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.h 2007-06-11 17:28:15.000000000 -0700
> +++ linux-2.6.22-rc4/include/linux/ext4_fs_sb.h 2007-06-11 17:39:05.000000000 -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 */
>
> #ifdef EXTENTS_STATS

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.

2007-07-11 02:00:41

[permalink] [raw]

Subject: Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp

2007-07-11 11:14:49

by Andreas Dilger

[permalink] [raw]

Subject: 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.

2007-07-11 11:28:54

by Andreas Dilger

[permalink] [raw]

Subject: 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_reserved1 osd1.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.h 2007-06-11 17:28:15.000000000 -0700
> > +++ linux-2.6.22-rc4/include/linux/ext4_fs_sb.h 2007-06-11 17:39:05.000000000 -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_ISIZE is set
(ro-compat so that older kernels can't create inodes with a smaller
extra_isize). That feature is only enabled if requested by the sysadmin.

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.

2007-07-12 12:57:51

[permalink] [raw]

Subject: Re: [EXT4 set 3][PATCH 1/1] ext4 nanosecond timestamp

Attachments:

ext4_nanosecond_correction.patch (3.34 kB)

2007-07-13 04:30:06

[permalink] [raw]

Subject: 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

2007-07-13 07:04:27

[permalink] [raw]

Subject: 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

2007-07-13 21:46:58

[permalink] [raw]

Subject: 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
>

2007-07-17 00:49:55

[permalink] [raw]

Subject: 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__) || 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 does
> one cause that to come about?
>
> > --- linux-2.6.22-rc4.orig/include/linux/ext4_fs_i.h 2007-06-11 17:22:15.000000000 -0700
> > +++ linux-2.6.22-rc4/include/linux/ext4_fs_i.h 2007-06-11 17:39:05.000000000 -0700
> > @@ -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.
>
> I thought checkpatch was going to have a little whine about that but the
> version I have here doesn't.
>
> >
> > #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.h 2007-06-11 17:28:15.000000000 -0700
> > +++ linux-2.6.22-rc4/include/linux/ext4_fs_sb.h 2007-06-11 17:39:05.000000000 -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 */
> >
> > #ifdef EXTENTS_STATS
>
> 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.

2007-07-17 10:04:09

[permalink] [raw]

Subject: 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.

2007-07-17 19:08:43

[permalink] [raw]

Subject: 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.
>

2007-07-17 20:44:39