Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1303308 > unrolled thread
| Started by | Deepa Dinamani <deepa.kernel@gmail.com> |
|---|---|
| First post | 2016-01-07 06:50 +0100 |
| Last post | 2016-01-15 06:10 +0100 |
| Articles | 20 on this page of 38 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-07 06:50 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-11 00:10 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-12 06:50 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-12 09:40 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-12 10:30 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-13 07:30 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-13 10:30 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-13 17:40 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-13 22:10 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-14 18:00 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-14 19:10 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-14 22:10 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-14 23:50 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-15 00:00 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-15 03:30 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-15 18:10 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-15 23:50 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-15 04:00 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-15 18:00 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Andreas Dilger <adilger@dilger.ca> - 2016-01-16 20:20 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-17 00:40 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Andreas Dilger <adilger@dilger.ca> - 2016-01-17 03:40 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-18 07:20 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-18 12:00 +0100
[RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-18 18:50 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-18 21:00 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-18 22:20 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-18 22:50 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-19 02:50 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-19 06:30 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-19 22:00 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-19 23:30 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-20 06:20 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-20 16:10 +0100
Re: [RFC 02/15] vfs: Change all structures to support 64 bit time Dave Chinner <david@fromorbit.com> - 2016-01-21 00:10 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Arnd Bergmann <arnd@arndb.de> - 2016-01-21 00:20 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-27 07:30 +0100
Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time Deepa Dinamani <deepa.kernel@gmail.com> - 2016-01-15 06:10 +0100
Page 1 of 2 [1] 2 Next page →
| From | Deepa Dinamani <deepa.kernel@gmail.com> |
|---|---|
| Date | 2016-01-07 06:50 +0100 |
| Subject | [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qObCr-3GI-11@gated-at.bofh.it> |
The current representation of inode times in struct inode, struct iattr,
and struct kstat use struct timespec. timespec is not y2038 safe.
Use scalar data types (seconds and nanoseconds stored separately) to
represent timestamps in struct inode in order to maintain same size for
times across 32 bit and 64 bit architectures.
In addition, lay them out such that they are packed on a naturally
aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
This will help save RAM space as inode structure is cached in memory.
The other structures are transient and do not benefit from these
changes.
Add accessors for inode timestamps.
These provide a way to access the time field members.
Accessors abstract the timestamp representation so that any logic
to convert between the struct inode timestamps and other interfaces
can be placed here.
The plan is to globally change all references to these types through
these accessors only. So when the actual internal representation
changes, it will be transparent to the outside world.
This can be extended to add code to validate the inode times that
are being set.
Macros are chosen as accessors rather than functions because we can
condense 3 {a,c,m} time functions into a single macro. After we agree
on an approach, the implementation could be changed to use static
inline functions if it suits more.
Add inode_timespec aliases to help convert kstat and iattr times to use
64 bit times. These hide the internal data type.
Use uapi exposed data types here to keep minimal timstamp data type
conversions in API's interfacing with vfs.
After the CONFIG_FS_USES_64BIT_TIME is enabled, all inode_timespec
aliases will be removed and timespec64 data types and API's will
be used directly.
Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com>
---
include/linux/fs.h | 55 +++++++++++++++++++++++++++++++++++++++++++-------
include/linux/stat.h | 6 +++---
include/linux/time64.h | 21 +++++++++++++++++++
3 files changed, 72 insertions(+), 10 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 12ba937..b9f3cee 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -245,13 +245,13 @@ typedef void (dax_iodone_t)(struct buffer_head *bh_map, int uptodate);
*/
struct iattr {
unsigned int ia_valid;
- umode_t ia_mode;
- kuid_t ia_uid;
- kgid_t ia_gid;
- loff_t ia_size;
- struct timespec ia_atime;
- struct timespec ia_mtime;
- struct timespec ia_ctime;
+ umode_t ia_mode;
+ kuid_t ia_uid;
+ kgid_t ia_gid;
+ loff_t ia_size;
+ struct inode_timespec ia_atime;
+ struct inode_timespec ia_mtime;
+ struct inode_timespec ia_ctime;
/*
* Not an attribute, but an auxiliary info for filesystems wanting to
@@ -616,9 +616,18 @@ struct inode {
};
dev_t i_rdev;
loff_t i_size;
+#ifdef CONFIG_FS_USES_64BIT_TIME
+ time64_t i_atime_sec;
+ time64_t i_mtime_sec;
+ time64_t i_ctime_sec;
+ s32 i_atime_nsec;
+ s32 i_mtime_nsec;
+ s32 i_ctime_nsec;
+#else
struct timespec i_atime;
struct timespec i_mtime;
struct timespec i_ctime;
+#endif
spinlock_t i_lock; /* i_blocks, i_bytes, maybe i_size */
unsigned short i_bytes;
unsigned int i_blkbits;
@@ -679,6 +688,38 @@ struct inode {
void *i_private; /* fs or device private pointer */
};
+#ifdef CONFIG_FS_USES_64BIT_TIME
+
+#define VFS_INODE_SET_XTIME(xtime, inode, ts64) \
+ do { \
+ struct inode_timespec __ts = (ts64); \
+ (inode)->xtime##_sec = __ts.tv_sec; \
+ (inode)->xtime##_nsec = __ts.tv_nsec; \
+ } while (0)
+
+#define VFS_INODE_GET_XTIME(xtime, inode) \
+ (struct timespec64){.tv_sec = (inode)->xtime##_sec, \
+ .tv_nsec = (inode)->xtime##_nsec}
+
+#else
+
+#define VFS_INODE_SET_XTIME(xtime, inode, ts) \
+ ((inode)->xtime = (ts))
+
+#define VFS_INODE_GET_XTIME(xtime, inode) \
+ ((inode)->xtime)
+
+#endif
+
+#define VFS_INODE_SWAP_XTIME(xtime, inode1, inode2) \
+ do { \
+ struct inode_timespec __ts = \
+ VFS_INODE_GET_XTIME(xtime, inode1); \
+ VFS_INODE_SET_XTIME(xtime, inode1, \
+ VFS_INODE_GET_XTIME(xtime, inode2)); \
+ VFS_INODE_SET_XTIME(xtime, inode2, __ts); \
+ } while (0)
+
static inline int inode_unhashed(struct inode *inode)
{
return hlist_unhashed(&inode->i_hash);
diff --git a/include/linux/stat.h b/include/linux/stat.h
index 075cb0c..559983f 100644
--- a/include/linux/stat.h
+++ b/include/linux/stat.h
@@ -27,9 +27,9 @@ struct kstat {
kgid_t gid;
dev_t rdev;
loff_t size;
- struct timespec atime;
- struct timespec mtime;
- struct timespec ctime;
+ struct inode_timespec atime;
+ struct inode_timespec mtime;
+ struct inode_timespec ctime;
unsigned long blksize;
unsigned long long blocks;
};
diff --git a/include/linux/time64.h b/include/linux/time64.h
index 367d5af..be98201 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -26,6 +26,27 @@ struct itimerspec64 {
#endif
+#ifdef CONFIG_FS_USES_64BIT_TIME
+
+/* Place holder defines until CONFIG_FS_USES_64BIT_TIME
+ * is enabled.
+ * timespec64 data type and functions will be used at that
+ * time directly and these defines will be deleted.
+ */
+#define inode_timespec timespec64
+
+#define inode_timespec_compare timespec64_compare
+#define inode_timespec_equal timespec64_equal
+
+#else
+
+#define inode_timespec timespec
+
+#define inode_timespec_compare timespec_compare
+#define inode_timespec_equal timespec_equal
+
+#endif
+
/* Parameters used to convert the timespec values: */
#define MSEC_PER_SEC 1000L
#define USEC_PER_MSEC 1000L
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-11 00:10 +0100 |
| Message-ID | <qPxhx-2iT-31@gated-at.bofh.it> |
| In reply to | #1303308 |
On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote: > The current representation of inode times in struct inode, struct iattr, > and struct kstat use struct timespec. timespec is not y2038 safe. > > Use scalar data types (seconds and nanoseconds stored separately) to > represent timestamps in struct inode in order to maintain same size for > times across 32 bit and 64 bit architectures. > In addition, lay them out such that they are packed on a naturally > aligned boundary on 64 bit arch as 4 bytes are used to store nsec. > This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes. > This will help save RAM space as inode structure is cached in memory. > The other structures are transient and do not benefit from these > changes. IMO, this decisions sends the patch series immediately down the wrong path. TO me, this is a severe case of premature optimisation because everything gets way more complex just to save those 8 bytes, especially as those holes can be filled simply by changing the variable declaration order in the structure and adding a simple comment. And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at all; you've got to touch lots of code(*), making it all shouty and harder to read. They seem only to exist because of the above structural change requires an abstract timestamp accessor while CONFIG_FS_USES_64BIT_TIME exists. Given that goes away at the end o the series, so should the macro - if we use a struct timespec64 in the first place, it isn't even necessary as a temporary construct. (*) I note you haven't touched XFS, which means you've probably broken lots of other filesystem code. e.g. in XFS, functions like xfs_vn_getattr() and xfs_vn_update_time() access inode->i_[acm]time directly and hence are not going to compile after this patch series. Cheers, Dave. -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Deepa Dinamani <deepa.kernel@gmail.com> |
|---|---|
| Date | 2016-01-12 06:50 +0100 |
| Message-ID | <qQ009-51J-13@gated-at.bofh.it> |
| In reply to | #1305734 |
> On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote: > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote: >> The current representation of inode times in struct inode, struct iattr, >> and struct kstat use struct timespec. timespec is not y2038 safe. >> >> Use scalar data types (seconds and nanoseconds stored separately) to >> represent timestamps in struct inode in order to maintain same size for >> times across 32 bit and 64 bit architectures. >> In addition, lay them out such that they are packed on a naturally >> aligned boundary on 64 bit arch as 4 bytes are used to store nsec. >> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes. >> This will help save RAM space as inode structure is cached in memory. >> The other structures are transient and do not benefit from these >> changes. > > IMO, this decisions sends the patch series immediately down the > wrong path. There are other things the patch does that I would like to get comments on: inode_timespec aliases, range check, individual fs changes etc. These are independent of the inode timestamp representation changes. >TO me, this is a severe case of premature optimisation > because everything gets way more complex just to save those 8 bytes, > especially as those holes can be filled simply by changing the > variable declaration order in the structure and adding a simple > comment. I had tried rearranging the structure and the pahole tool does not show any difference unless you pack and align the struct to 4 bytes on 64 bit arch. The change actually saves 16 bytes on x86_64 and adds 12 bytes on i386. Here is the breakdown for struct inode before and after the patch: x86_64: /* size: 544, cachelines: 9, members: 44 */ | /* size: 528, cachelines: 9, members: 47 */ /* sum members: 534, holes: 3, sum holes: 10 */ | /* sum members: 522, holes: 2, sum holes: 6 */ i386: /* size: 328, cachelines: 6, members: 45 */ | /* size: 340, cachelines: 6, members: 48 */ /* sum members: 326, holes: 1, sum holes: 2 */ | /* sum members: 338, holes: 1, sum holes: 2 */ According to /proc/slabinfo I estimated savings of 4MB on a lightly loaded system. > And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at > all; you've got to touch lots of code(*), making it all shouty and > harder to read. They seem only to exist because of the above > structural change requires an abstract timestamp accessor while > CONFIG_FS_USES_64BIT_TIME exists. > Given that goes away at the end o > the series, so should the macro - if we use a struct timespec64 in > the first place, it isn't even necessary as a temporary construct timespec64 was the first option considered here. The problem with using timespec64 is the long data type to represent nsec. If it were possible to change timespec64 nsec to int data type then it might be okay to use that if we are not worried about holes. I do not see why time stamps should have different representations on a 32 bit vs a 64 bit arch. This left us with the option define a new data type to represent timestamps. I agreed with the concerns on the earlier RFC series that there are already very many data types to represent time in the kernel. So this left me with the option of using scalar types to represent these. The scalar types were not used for optimization. They just happened to serve that purpose as well. This could be in a follow on patch, but as long as we are changing the representation everywhere, I don't see why there should be an intermediate step to change it to timespec64 only to change it to this representation later. As far as accessors are concerned, there already are accessors in the VFS: generic_fillattr() and setattr_copy(). The problem really is that there is more than one way of updating these attributes(timestamps in this particular case). The side effect of this is that we don't always call timespec_trunc() before assigning timestamps which can lead to inconsistencies between on disk and in memory inode timestamps. Also, since these also touch other attributes, these become more restrictive. The accessors were an idea to streamline all accesses to timestamps in inode. Right now the accessor macros also figure out if the timestamps were clamped and then call the registered callback. But, this can be extended to include fs_time_trunc() and then all the end users can just use these and not worry about the right granularity or range. As the commit text says, these can be changed to inline functions to avoid shouty case. > (*) I note you haven't touched XFS, which means you've probably > broken lots of other filesystem code. e.g. in XFS, functions like > xfs_vn_getattr() and xfs_vn_update_time() access inode->i_[acm]time > directly and hence are not going to compile after this patch series. I think I should have explained this more in my cover letter, as this has come up twice now. Patches 1-7 are the only ones that are relevant and compiled and tested. These change three example filesystems as an illustration of the proposed solution. Of course, every filesystem will have to be changed similarly before patches 8-15 and a few more to change additional filesystems to use timespec64 can be merged. Patches 8-15 were included merely to provide a complete picture, as I thought patches explained the concept better than words only. These have not even been compiled, as these are for illustration purposes as noted in the cover letter. -Deepa
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-12 09:40 +0100 |
| Message-ID | <qQ2EG-6Ka-7@gated-at.bofh.it> |
| In reply to | #1306998 |
On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
> > On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote:
> >
> >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> >> The current representation of inode times in struct inode, struct iattr,
> >> and struct kstat use struct timespec. timespec is not y2038 safe.
> >>
> >> Use scalar data types (seconds and nanoseconds stored separately) to
> >> represent timestamps in struct inode in order to maintain same size for
> >> times across 32 bit and 64 bit architectures.
> >> In addition, lay them out such that they are packed on a naturally
> >> aligned boundary on 64 bit arch as 4 bytes are used to store nsec.
> >> This makes each tuple(sec, nscec) use 12 bytes instead of 16 bytes.
> >> This will help save RAM space as inode structure is cached in memory.
> >> The other structures are transient and do not benefit from these
> >> changes.
> >
> > IMO, this decisions sends the patch series immediately down the
> > wrong path.
>
> There are other things the patch does that I would like to get comments
> on: inode_timespec aliases, range check, individual fs changes etc.
> These are independent of the inode timestamp representation changes.
The inode_timespec alias is part of the problem - AFAICT it exists
because you need a representation that is independent of both the
old and new in-memory structures.
The valid timestamp range stuff in the superblock is absolutely
necessary, but that's something that can be done completely
independently to the changes for supporting a differnet time storage
format.
And the fs changes cannot really be commented on until the VFs time
representation is sorted out properly...
> >TO me, this is a severe case of premature optimisation
> > because everything gets way more complex just to save those 8 bytes,
> > especially as those holes can be filled simply by changing the
> > variable declaration order in the structure and adding a simple
> > comment.
>
> I had tried rearranging the structure and the pahole tool does not show
> any difference unless you pack and align the struct to 4 bytes on 64
> bit arch. The change actually saves 16 bytes on x86_64 and adds 12
> bytes on i386.
>
> Here is the breakdown for struct inode before and after the patch:
>
> x86_64:
> /* size: 544, cachelines: 9, members: 44 */ |
> /* size: 528, cachelines: 9, members: 47 */
> /* sum members: 534, holes: 3, sum holes: 10 */ |
> /* sum members: 522, holes: 2, sum holes: 6 */
>
> i386:
> /* size: 328, cachelines: 6, members: 45 */ |
> /* size: 340, cachelines: 6, members: 48 */
> /* sum members: 326, holes: 1, sum holes: 2 */ |
> /* sum members: 338, holes: 1, sum holes: 2 */
>
> According to /proc/slabinfo I estimated savings of 4MB on a lightly
> loaded system.
>
> > And, really, I don't like those VFS_INODE_[GS]ET_XTIME macros at
> > all; you've got to touch lots of code(*), making it all shouty and
> > harder to read. They seem only to exist because of the above
> > structural change requires an abstract timestamp accessor while
> > CONFIG_FS_USES_64BIT_TIME exists.
> > Given that goes away at the end o
> > the series, so should the macro - if we use a struct timespec64 in
> > the first place, it isn't even necessary as a temporary construct
>
> timespec64 was the first option considered here. The problem with using
> timespec64 is the long data type to represent nsec. If it were possible
> to change timespec64 nsec to int data type then it might be okay to use
> that if we are not worried about holes. I do not see why time stamps
> should have different representations on a 32 bit vs a 64 bit arch.
What's it matter? iot's irrelevant to the problem at hand.
Besides, have you looked at the existing timestamp definitions? they
use a struct timespec, which on a 64 bit system:
struct timespec i_atime; /* 88 16 */
struct timespec i_mtime; /* 104 16 */
struct timespec i_ctime; /* 120 16 */
use 2 longs and are identical to a timespec64 in everything but
name. These should just be changed to a timespec64, and we suck up
the fact it increases the size of the 32 bit inode as we have to
increase it's size to support time > y2038 anyway.
This is what I meant about premature optimisation - you've got a
wonderfully complex solution to a problem that we don't need to
solve to support timestamps >y2038. It's also why it goes down the
wrong path at this point - most of the changes are not necessary if
all we need to do is a simple timespec -> timespec64 type change and
the addition timestamp range limiting in the existing truncation
function...
> The problem really is that
> there is more than one way of updating these attributes(timestamps in
> this particular case). The side effect of this is that we don't always
> call timespec_trunc() before assigning timestamps which can lead to
> inconsistencies between on disk and in memory inode timestamps.
That's a problem that can be fixed independently of y2038 support.
Indeed, we can be quite lazy about updating timestamps - by intent
and design we usually have different timestamps in memory compared
to on disk, which is one of the reasons why there are so many
different ways to change and update timestamps....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-12 10:30 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQ3r4-7nB-33@gated-at.bofh.it> |
| In reply to | #1307098 |
On Tuesday 12 January 2016 19:29:57 Dave Chinner wrote: > > This is what I meant about premature optimisation - you've got a > wonderfully complex solution to a problem that we don't need to > solve to support timestamps >y2038. It's also why it goes down the > wrong path at this point - most of the changes are not necessary if > all we need to do is a simple timespec -> timespec64 type change and > the addition timestamp range limiting in the existing truncation > function... I originally suggested doing the split representation because I was worried about the downsides of using timespec64 on 32-bit systems after looking at actual memory consumption on my test box. At this moment, I have a total of 145712700 inodes in memory on a machine with 64GB ram, saving 12 bytes on each amounts to a total of 145MB. I think it was more than that when I first looked, so it's between 0.2% and 0.3% of savings in total memory, which is certainly worth discussing about, given the renewed interest in conserving RAM in general. If we want to save this memory, then doing it at the same time as the timespec64 conversion is the right time so we don't need to touch every file twice. One point that I had not considered though is on the 32-bit systems we are talking about, not only is RAM much smaller, but also there would be a smaller fraction of RAM available to store inodes, so there is not as much to gain. Arnd
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-13 07:30 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQn6q-48g-15@gated-at.bofh.it> |
| In reply to | #1307148 |
On Tue, Jan 12, 2016 at 10:27:07AM +0100, Arnd Bergmann wrote: > On Tuesday 12 January 2016 19:29:57 Dave Chinner wrote: > > > > This is what I meant about premature optimisation - you've got a > > wonderfully complex solution to a problem that we don't need to > > solve to support timestamps >y2038. It's also why it goes down the > > wrong path at this point - most of the changes are not necessary if > > all we need to do is a simple timespec -> timespec64 type change and > > the addition timestamp range limiting in the existing truncation > > function... > > I originally suggested doing the split representation because I > was worried about the downsides of using timespec64 on 32-bit systems > after looking at actual memory consumption on my test box. > > At this moment, I have a total of 145712700 inodes in memory on a machine Is that all? :P > with 64GB ram, saving 12 bytes on each amounts to a total of 145MB. I just posted a patchset that knocks 104 bytes off the XFS inode (~12% reduction in size). We need the changes in that patchset to sanely support >y2038k support in XFS, and it means we now won't need to grow the XFS inode to add that support, either. > I think > it was more than that when I first looked, so it's between 0.2% and 0.3% > of savings in total memory, which is certainly worth discussing about, > given the renewed interest in conserving RAM in general. If we want to > save this memory, then doing it at the same time as the timespec64 conversion > is the right time so we don't need to touch every file twice. You just uttered the key words: "If we want to save this memory" So let's stop conflating two different lines of development because we only actually *need* y2038k support. The fact we haven't made timestamp space optimisations means that nobody has thought it necessary or worthwhile. y2038k support doesn't change the landscape under which we might consider the optimisation, so we need to determine if the benefit outweighs the cost in terms of code complexity and maintainability. So separate the two changes - make the y2038k change simple and obviously correct first by changing everything to timespec64. Then it won't get delayed by bikeshedding about an optimisation of that is of questionable benefit. Cheers, Dave. -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-13 10:30 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQpUD-63W-5@gated-at.bofh.it> |
| In reply to | #1308078 |
On Wednesday 13 January 2016 17:27:16 Dave Chinner wrote: > > I think > > it was more than that when I first looked, so it's between 0.2% and 0.3% > > of savings in total memory, which is certainly worth discussing about, > > given the renewed interest in conserving RAM in general. If we want to > > save this memory, then doing it at the same time as the timespec64 conversion > > is the right time so we don't need to touch every file twice. > > You just uttered the key words: "If we want to save this memory" > > So let's stop conflating two different lines of development because > we only actually *need* y2038k support. > > The fact we haven't made timestamp space optimisations means > that nobody has thought it necessary or worthwhile. y2038k support > doesn't change the landscape under which we might consider the > optimisation, so we need to determine if the benefit outweighs the > cost in terms of code complexity and maintainability. > > So separate the two changes - make the y2038k change simple and > obviously correct first by changing everything to timespec64. Then it > won't get delayed by bikeshedding about an optimisation of that is > of questionable benefit. Fine with me. I think Deepa already started simplifying the series already. I agree that for 64-bit machines, there is no need to optimize that code now, since we are not regressing in terms of memory size. For 32-bit machines, we are regressing anyway, the question is whether it's by 12 or 24 bytes per inode. Let me try to estimate the worse-case scenario here: let's assume that we have 1GB of RAM (anything more on a 32-bit system gets you into trouble, and if you have less, there will be less of a problem). Filling all of system ram with small tmpfs files means a single 4K page plus 280 bytes for the minimum inode, so we need an additional 6MB or 12MB to store the extra timespec bits. Probably not too bad for a worst-case scenario, but there is also the case of storing just the inodes but no pages, and that would be worse. I've added the linux-arm and linux-mips lists to cc, to see if anyone has strong opinions on this matter. We don't have to worry about x86-32 here, because sizeof(struct timespec64) is 12 bytes there anyway, and I don't think there are any other 32-bit architectures that have large-scale deployments or additional requirements we don't already have on ARM. Arnd
[toc] | [prev] | [next] | [standalone]
| From | Deepa Dinamani <deepa.kernel@gmail.com> |
|---|---|
| Date | 2016-01-13 17:40 +0100 |
| Message-ID | <qQwCL-2o0-45@gated-at.bofh.it> |
| In reply to | #1307098 |
On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote: > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote: > > > On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote: > > > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote: Summarizing, here are the open questions that need to be sorted before another series: 1. What should be part of the series? a. y2038 safe data types conversion. b. range check and clamping 2. How to achieve a seamless transition? Is inode_timespec solution agreed upon to achieve 1a? An alternate approach is included in the cover letter. 3. policy for handling out of range timestamps: There was no conclusion on this from the previous series as noted in the cover letter. I think this can be solved by figuring out the answer to question: who should have a say in deciding the course of action if the timestamps are out of range: a. sysadmin through sysctl (Arnd's suggestion) b. have default vfs handlers with an option for individual fs to override. c. clamp and ignore d. disable expired fs at compile time (Arnd's suggestion) > The inode_timespec alias is part of the problem - AFAICT it exists > because you need a representation that is independent of both the > old and new in-memory structures. Maybe you are misunderstanding the fact that only struct inode is changed to have individual fields. Whereas, timespec64 is used everywhere else in the series. inode_timespec is an alias to transition timespec references to timespec64 without breaking anything. This is needed because we don't want to change all references to timespec in a single patch like the cover letter says. The same RFC Patch 2 has more details. > The valid timestamp range stuff in the superblock is absolutely > necessary, but that's something that can be done completely > independently to the changes for supporting a differnet time storage > format. If I'm defining new functions to support new format, then I should at least get the function signatures right before using them. These can be in a different patch, but should be in the same patch series, before they are used anywhere. For instance, struct timespec timespec_trunc(struct timespec t, unsigned gran); should now take superblock as an argument instead of gran. > And the fs changes cannot really be commented on until the VFs time > representation is sorted out properly... Each fs is changed twice in the current approach to transition everything to timespec64. And, there are different ways of doing this. For instance, Arnd had an idea different from mine as to how this can be done: He was suggesting using something like these accessor macros and incorporating timespec64 from the beginning in the individual filesystems rather than inode_timespec. Again, this is independent of how timestamps are stored in struct inode. There are others that are independent of inode timestamp representation as well > Besides, have you looked at the existing timestamp definitions? they > use a struct timespec, which on a 64 bit system: > > struct timespec i_atime; /* 88 16 */ > struct timespec i_mtime; /* 104 16 */ > struct timespec i_ctime; /* 120 16 */ > > use 2 longs and are identical to a timespec64 in everything but > name. These should just be changed to a timespec64, and we suck up > the fact it increases the size of the 32 bit inode as we have to > increase it's size to support time > y2038 anyway. > > This is what I meant about premature optimisation - you've got a > wonderfully complex solution to a problem that we don't need to > solve to support timestamps >y2038. It's also why it goes down the > wrong path at this point - most of the changes are not necessary if > all we need to do is a simple timespec -> timespec64 type change and > the addition timestamp range limiting in the existing truncation > function... The pahole output I pasted in the previous email(on the left) was for timespec. Yes, I do know that timespec64 is same as timespec on 64 bit systems: #if __BITS_PER_LONG == 64 # define timespec64 timespec I think it's been agreed upon now that inode timestamps will be changed to use timespec64 as Arnd mentioned, if I do not hear any objections. The whole purpose of this is to gather comments. > > The problem really is that > > there is more than one way of updating these attributes(timestamps in > > this particular case). The side effect of this is that we don't always > > call timespec_trunc() before assigning timestamps which can lead to > > inconsistencies between on disk and in memory inode timestamps. > > That's a problem that can be fixed independently of y2038 support. > Indeed, we can be quite lazy about updating timestamps - by intent > and design we usually have different timestamps in memory compared > to on disk, which is one of the reasons why there are so many > different ways to change and update timestamps.... This has nothing to do with lazy updates. This is about writing wrong granularities and non clamped values to in-memory inode. -Deepa
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-13 22:10 +0100 |
| Message-ID | <qQAQ1-5ns-9@gated-at.bofh.it> |
| In reply to | #1308584 |
On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote: > On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote: > > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote: > > > > On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote: > > > > > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote: > > Summarizing, here are the open questions that need to be sorted before another > series: > 1. What should be part of the series? > a. y2038 safe data types conversion. > b. range check and clamping Yes. > 2. How to achieve a seamless transition? > Is inode_timespec solution agreed upon to achieve 1a? No. Just convert direct to timespec64. > An alternate approach is included in the cover letter. > 3. policy for handling out of range timestamps: > There was no conclusion on this from the previous series as noted in the > cover letter. > a. sysadmin through sysctl (Arnd's suggestion) > b. have default vfs handlers with an option for individual fs to override. > c. clamp and ignore I think it's a mix - if the timestamps come in from userspace, fail with ERANGE. That could be controlled by sysctl via VFS part of the ->setattr operation, or in each of the individual FS implementations. If they come from the kernel (e.g. atime update) then the generic behvaiour is to warn and continue, filesystems can otherwise select their own policy for kernel updates via ->update_time. > d. disable expired fs at compile time (Arnd's suggestion) Not really an option, because it means we can't use filesystems that interop with other systems (e.g. cameras, etc) because they won't support y2038k timestamps for a long time, if ever (e.g. vfat). > > > The problem really is that > > > there is more than one way of updating these attributes(timestamps in > > > this particular case). The side effect of this is that we don't always > > > call timespec_trunc() before assigning timestamps which can lead to > > > inconsistencies between on disk and in memory inode timestamps. > > > > That's a problem that can be fixed independently of y2038 support. > > Indeed, we can be quite lazy about updating timestamps - by intent > > and design we usually have different timestamps in memory compared > > to on disk, which is one of the reasons why there are so many > > different ways to change and update timestamps.... > > This has nothing to do with lazy updates. > This is about writing wrong granularities and non clamped values to > in-memory inode. Which really shouldn't happen because we should be clamping and/or truncating timestamps at the creation/entry point into the VFS/filesystem. e.g. current_fs_time(sb) is how filesystems grab the current kernel time for timestamp updates. Add an equivalent current_fs_time64(sb) to do return timespec64 and do clamping and limit warning, and now you have a simple vehicle for converting the VFS and filesystems to support y2038k clean date formats. If there are places where filesystems are receiving or using unchecked timestamps then those are bugs that need fixing. Those need to be in separate patches to y2038k support... Cheers, Dave. -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-14 18:00 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQTpD-1kn-1@gated-at.bofh.it> |
| In reply to | #1308815 |
On Thursday 14 January 2016 08:04:36 Dave Chinner wrote: > On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote: > > On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote: > > > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote: > > > > > On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote: > > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote: > > > > 2. How to achieve a seamless transition? > > Is inode_timespec solution agreed upon to achieve 1a? > > No. Just convert direct to timespec64. The hard part here is how to split that change into logical patches per file system. We have already discussed all sorts of ways to do that, but there is no ideal solution, as you usually end up either having some really large patches, or you have to modify the same lines multiple times. The most promising approaches are: a) In Deepa's current patch set, some infrastructure is first introduced by changing the type from timespec to an identical inode_timespec, which lets us convert one file system at a time to inode_timespec and then change the type once they are all done. The downside is then that all file systems have to get touched twice so we end up with timespec64 everywhere. b) A variation of that which I would do is to use have a smaller set of infrastructure first, so we can change one file system at a time to timespec64 while leaving the common structures to use timespec until all file systems are converted. The downside is the use of some conversion macros when accessing the times in the inode. When the common code is changed, those accessor macros get turned into trivial assignments that can be removed up later or changed in the same patch. c) The opposite direction from b) is to first change the common code, but then any direct assignment between a timespec in a file system and the timespec64 in the inode/iattr/kstat/etc first needs a conversion helper so we can build cleanly, and then we do one file system at a time to remove them all again while changing the internal structures in the file system from timespec to timespec64. > > An alternate approach is included in the cover letter. > > 3. policy for handling out of range timestamps: > > There was no conclusion on this from the previous series as noted in the > > cover letter. > > a. sysadmin through sysctl (Arnd's suggestion) > > b. have default vfs handlers with an option for individual fs to override. > > c. clamp and ignore > > I think it's a mix - if the timestamps come in from userspace, > fail with ERANGE. That could be controlled by sysctl via VFS > part of the ->setattr operation, or in each of the individual FS > implementations. If they come from the kernel (e.g. atime update) then > the generic behvaiour is to warn and continue, filesystems can > otherwise select their own policy for kernel updates via > ->update_time. I'd prefer not to have it done by the individual file system implementation, so we get a consistent behavior. Normally you either care about correct time stamps, or you care about interoperability and you don't want to have errors returned here. It could be done per mount, but that seems overly complicated for rather little to be gained. > > d. disable expired fs at compile time (Arnd's suggestion) > > Not really an option, because it means we can't use filesystems that > interop with other systems (e.g. cameras, etc) because they won't > support y2038k timestamps for a long time, if ever (e.g. vfat). Let me clarify what my idea is here: I want a global kernel option that disables all code that has known y2038 issues. If anyone tries to build an embedded system with support beyond 2038, that should disable all of those things, including file systems, drivers and system calls, so we can reasonably assume that everything that works today with that kernel build will keep working in the future and not break in random ways. For a file system, this can be done in a number of ways: * Most file systems today interpret the time as an unsigned 32-bit number (as opposed to signed as ext3, xfs and few others do), so as long as we use timespec64 in the syscalls, we are ok. * Some legacy file systems (maybe hfs) can remain disabled, as nobody cares about them any more. * If we still care about them (e.g. ext2), we can make them support only read-only mode. In ext4, this would mean forbidding write access to file systems that don't have the extended inode format enabled. Normal users that don't care about not breaking in 2038 obviously won't set the option, and have the same level of backwards compatibility support as today. > > > > The problem really is that > > > > there is more than one way of updating these attributes(timestamps in > > > > this particular case). The side effect of this is that we don't always > > > > call timespec_trunc() before assigning timestamps which can lead to > > > > inconsistencies between on disk and in memory inode timestamps. > > > > > > That's a problem that can be fixed independently of y2038 support. > > > Indeed, we can be quite lazy about updating timestamps - by intent > > > and design we usually have different timestamps in memory compared > > > to on disk, which is one of the reasons why there are so many > > > different ways to change and update timestamps.... > > > > This has nothing to do with lazy updates. > > This is about writing wrong granularities and non clamped values to > > in-memory inode. > > Which really shouldn't happen because we should be clamping and/or > truncating timestamps at the creation/entry point into the > VFS/filesystem. > > e.g. current_fs_time(sb) is how filesystems grab the current kernel > time for timestamp updates. Add an equivalent current_fs_time64(sb) > to do return timespec64 and do clamping and limit warning, and now > you have a simple vehicle for converting the VFS and filesystems to > support y2038k clean date formats. I think the current patch series does this already. > If there are places where filesystems are receiving or using > unchecked timestamps then those are bugs that need fixing. Those > need to be in separate patches to y2038k support... Fair enough, but that probably means that patch series will have to come first. This will also reduce the number of places in which a separate type conversion function needs to be added. Arnd
[toc] | [prev] | [next] | [standalone]
| From | Deepa Dinamani <deepa.kernel@gmail.com> |
|---|---|
| Date | 2016-01-14 19:10 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQUvn-2j8-1@gated-at.bofh.it> |
| In reply to | #1309479 |
On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote: > On Thursday 14 January 2016 08:04:36 Dave Chinner wrote: > > On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote: > > > On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote: > > > > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote: > > > > > > On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote: > > > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote: > > > > > > 2. How to achieve a seamless transition? > > > Is inode_timespec solution agreed upon to achieve 1a? > > > > No. Just convert direct to timespec64. > > The hard part here is how to split that change into logical patches > per file system. We have already discussed all sorts of ways to > do that, but there is no ideal solution, as you usually end up > either having some really large patches, or you have to modify > the same lines multiple times. > > The most promising approaches are: > > a) In Deepa's current patch set, some infrastructure is first > introduced by changing the type from timespec to an identical > inode_timespec, which lets us convert one file system at a time > to inode_timespec and then change the type once they are all > done. The downside is then that all file systems have to get > touched twice so we end up with timespec64 everywhere. > > b) A variation of that which I would do is to use have a smaller > set of infrastructure first, so we can change one file system > at a time to timespec64 while leaving the common structures to > use timespec until all file systems are converted. The downside > is the use of some conversion macros when accessing the times > in the inode. > When the common code is changed, those accessor macros get > turned into trivial assignments that can be removed up later > or changed in the same patch. > > c) The opposite direction from b) is to first change the common > code, but then any direct assignment between a timespec in > a file system and the timespec64 in the inode/iattr/kstat/etc > first needs a conversion helper so we can build cleanly, > and then we do one file system at a time to remove them all > again while changing the internal structures in the > file system from timespec to timespec64. Just a clarification here: approaches b and c also need some functions that take times as arguments, including a function pointer in the vfs layer to be supported in both forms: timespec and timespec64 concurrently. As included in the cover letter, these are: generic_update_time(), inode->i_op->update_time(), lease_get_mtime(), fstack_copy_attr_all(), setattr_copy(), generic_fillattr(). -Deepa
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-14 22:10 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQXjA-4eb-11@gated-at.bofh.it> |
| In reply to | #1309479 |
On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote:
> On Thursday 14 January 2016 08:04:36 Dave Chinner wrote:
> > On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote:
> > > On Tue, Jan 12, 2016 at 07:29:57PM +1100, Dave Chinner wrote:
> > > > On Mon, Jan 11, 2016 at 09:42:36PM -0800, Deepa Dinamani wrote:
> > > > > > On Jan 11, 2016, at 04:33, Dave Chinner <david@fromorbit.com> wrote:
> > > > > >> On Wed, Jan 06, 2016 at 09:35:59PM -0800, Deepa Dinamani wrote:
> > >
> > > 2. How to achieve a seamless transition?
> > > Is inode_timespec solution agreed upon to achieve 1a?
> >
> > No. Just convert direct to timespec64.
>
> The hard part here is how to split that change into logical patches
> per file system. We have already discussed all sorts of ways to
> do that, but there is no ideal solution, as you usually end up
> either having some really large patches, or you have to modify
> the same lines multiple times.
>
> The most promising approaches are:
>
> a) In Deepa's current patch set, some infrastructure is first
> introduced by changing the type from timespec to an identical
> inode_timespec, which lets us convert one file system at a time
> to inode_timespec and then change the type once they are all
> done. The downside is then that all file systems have to get
> touched twice so we end up with timespec64 everywhere.
Yes, and the result is not pretty.
> b) A variation of that which I would do is to use have a smaller
> set of infrastructure first, so we can change one file system
> at a time to timespec64 while leaving the common structures to
> use timespec until all file systems are converted. The downside
> is the use of some conversion macros when accessing the times
> in the inode.
> When the common code is changed, those accessor macros get
> turned into trivial assignments that can be removed up later
> or changed in the same patch.
Doesn't really make a lot of sense to me. We have to change
everything evntually, and it's not that much work to do so up
front...
> c) The opposite direction from b) is to first change the common
> code, but then any direct assignment between a timespec in
> a file system and the timespec64 in the inode/iattr/kstat/etc
> first needs a conversion helper so we can build cleanly,
> and then we do one file system at a time to remove them all
> again while changing the internal structures in the
> file system from timespec to timespec64.
No new helpers are necessary - we've already got the helper
functions we need. This:
int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
{
struct inode *inode = d_inode(old_dentry);
+ struct inode_timespec now = current_fs_time(inode->i_sb);
- inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
+ VFS_INODE_SET_XTIME(i_ctime, inode, now);
+ VFS_INODE_SET_XTIME(i_mtime, dir, now);
+ VFS_INODE_SET_XTIME(i_ctime, dir, now);
inc_nlink(inode);
.....
is just wrong. All the type conversion and clamping and checking
done in that VFS_INODE_SET_XTIME() should be done in
current_fs_time() and have it return a timespec64 directly. Indeed,
it already does truncation, and can easily be made to do range
clamping, too. i.e. the change should simply be:
- inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
+ inode->i_ctime = dir->i_ctime = dir->i_mtime = current_fs_time(inode->i_sb);
This will work cleanly with a s/timespec/timespec64/ API changeover,
too, without the need for any additional helpers at all.
i.e. the first thing to do is make sure everything that
creates/converts timestamps uses/returns a struct timespec that is
clamped and truncated at the earliest possible opportunity. i.e. on
entry to the kernel and when pulled from the kernel time. Once all
the code is using and passing around truncated+clamped struct
timespec (like above), then we can simply do a
s/timespec/timespec64/ on the inode and API, and we're done.
> > I think it's a mix - if the timestamps come in from userspace,
> > fail with ERANGE. That could be controlled by sysctl via VFS
> > part of the ->setattr operation, or in each of the individual FS
> > implementations. If they come from the kernel (e.g. atime update) then
> > the generic behvaiour is to warn and continue, filesystems can
> > otherwise select their own policy for kernel updates via
> > ->update_time.
>
> I'd prefer not to have it done by the individual file system
> implementation, so we get a consistent behavior.
Can't be helped, because different filesystems have different
timestamp behaviours, and not all use the generic VFS code for
timestamp updates. The filesystems need to use the correct helper
functions to obtain a valid current time, but you can't stop them
from storing and using arbitrary timestamp formats if they so
desire...
> > > d. disable expired fs at compile time (Arnd's suggestion)
> >
> > Not really an option, because it means we can't use filesystems that
> > interop with other systems (e.g. cameras, etc) because they won't
> > support y2038k timestamps for a long time, if ever (e.g. vfat).
>
> Let me clarify what my idea is here: I want a global kernel option
> that disables all code that has known y2038 issues. If anyone tries
> to build an embedded system with support beyond 2038, that should
> disable all of those things, including file systems, drivers and
> system calls, so we can reasonably assume that everything that works
> today with that kernel build will keep working in the future and
> not break in random ways.
It's not that black and white when it comes to filesystems. y2038k
support is determined by the on-disk structure of the filesystem
being mounted, and that is determined at mount time. When the
filesystem mounts and sets it's valid timestamp ranges the VFS
will need to decide as to whether the filesystem is allowed to
continue mounting or not.
> For a file system, this can be done in a number of ways:
>
> * Most file systems today interpret the time as an unsigned 32-bit
> number (as opposed to signed as ext3, xfs and few others do),
> so as long as we use timespec64 in the syscalls, we are ok.
Actually, sign conversion is a problem we currently have to be very
careful of. See, for example, xfstests:tests/generic/258, which
tests timestamps recording times before epoch. i.e. in XFS we have
to convert the unsigned 32 bit disk timestamp to signed 32 bit
before storing it in the VFS timestamp so it behaves correctly on 64
bit systems. This results in us needing to do this when reading the
inode from disk:
+ /*
+ * time is signed, so need to convert to signed 32 bit before
+ * storing in inode timestamp which may be 64 bit. Otherwise
+ * a time before epoch is converted to a time long after epoch
+ * on 64 bit systems.
+ */
+ inode->i_atime.tv_sec = (int)be32_to_cpu(from->di_atime.t_sec);
+ inode->i_atime.tv_nsec = (int)be32_to_cpu(from->di_atime.t_nsec);
+ inode->i_mtime.tv_sec = (int)be32_to_cpu(from->di_mtime.t_sec);
+ inode->i_mtime.tv_nsec = (int)be32_to_cpu(from->di_mtime.t_nsec);
+ inode->i_ctime.tv_sec = (int)be32_to_cpu(from->di_ctime.t_sec);
+ inode->i_ctime.tv_nsec = (int)be32_to_cpu(from->di_ctime.t_nsec);
+
(http://oss.sgi.com/archives/xfs/2016-01/msg00456.html)
> * Some legacy file systems (maybe hfs) can remain disabled, as
> nobody cares about them any more.
>
> * If we still care about them (e.g. ext2), we can make them support
> only read-only mode. In ext4, this would mean forbidding write
> access to file systems that don't have the extended inode format
> enabled.
For ext2/4, that would have to be handled internally by the
filesystem with feature masks. For other legacy filesystems, then
the VFS mount time checking could allow RO mounts if the supported
ranges are not y2038k clean. Compile time options are not
really the best approach here...
> > If there are places where filesystems are receiving or using
> > unchecked timestamps then those are bugs that need fixing. Those
> > need to be in separate patches to y2038k support...
>
> Fair enough, but that probably means that patch series will have to
> come first.
Yes, that is normal practice for structuring a non-trivial feature
addition patch series. Bug fixes first, then cleanups, then the
functionality changes.
> This will also reduce the number of places in which a
> separate type conversion function needs to be added.
Precisely. I'm pretty sure this should come down to "no new
conversion functions needed" for the vast majority of the code.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-14 23:50 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQYSl-5iF-7@gated-at.bofh.it> |
| In reply to | #1309652 |
On Friday 15 January 2016 08:00:01 Dave Chinner wrote:
> On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote:
> > On Thursday 14 January 2016 08:04:36 Dave Chinner wrote:
> > > On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote:
> > c) The opposite direction from b) is to first change the common
> > code, but then any direct assignment between a timespec in
> > a file system and the timespec64 in the inode/iattr/kstat/etc
> > first needs a conversion helper so we can build cleanly,
> > and then we do one file system at a time to remove them all
> > again while changing the internal structures in the
> > file system from timespec to timespec64.
>
> No new helpers are necessary - we've already got the helper
> functions we need. This:
>
> int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
> {
> struct inode *inode = d_inode(old_dentry);
> + struct inode_timespec now = current_fs_time(inode->i_sb);
>
> - inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> + VFS_INODE_SET_XTIME(i_ctime, inode, now);
> + VFS_INODE_SET_XTIME(i_mtime, dir, now);
> + VFS_INODE_SET_XTIME(i_ctime, dir, now);
> inc_nlink(inode);
> .....
>
> is just wrong. All the type conversion and clamping and checking
> done in that VFS_INODE_SET_XTIME() should be done in
> current_fs_time() and have it return a timespec64 directly. Indeed,
> it already does truncation, and can easily be made to do range
> clamping, too. i.e. the change should simply be:
>
> - inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> + inode->i_ctime = dir->i_ctime = dir->i_mtime = current_fs_time(inode->i_sb);
Yes, that is the obvious case, and I guess works for at least half the
file systems when they always assign righthand side and lefthand
side of the time stamps using the external types or helpers like
CURRENT_TIME and current_fs_time().
However, there are a couple of file systems that need a bit more refactoring
before we can do this, e.g. in ntfs_truncate:
if (!IS_NOCMTIME(VFS_I(base_ni)) && !IS_RDONLY(VFS_I(base_ni))) {
struct timespec now = current_fs_time(VFS_I(base_ni)->i_sb);
int sync_it = 0;
if (!timespec_equal(&VFS_I(base_ni)->i_mtime, &now) ||
!timespec_equal(&VFS_I(base_ni)->i_ctime, &now))
sync_it = 1;
VFS_I(base_ni)->i_mtime = now;
VFS_I(base_ni)->i_ctime = now;
}
The type of the local variable must match the return code of
current_fs_time(), so if we change over i_mtime and current_fs_time
globally, this either has to be rewritten first to avoid the use of
local variables, or it needs temporary conversion helpers, or
it has to be changed in the same patch. None of those is particularly
appealing. There are a few dozen such things in various file systems.
> > > I think it's a mix - if the timestamps come in from userspace,
> > > fail with ERANGE. That could be controlled by sysctl via VFS
> > > part of the ->setattr operation, or in each of the individual FS
> > > implementations. If they come from the kernel (e.g. atime update) then
> > > the generic behvaiour is to warn and continue, filesystems can
> > > otherwise select their own policy for kernel updates via
> > > ->update_time.
> >
> > I'd prefer not to have it done by the individual file system
> > implementation, so we get a consistent behavior.
>
> Can't be helped, because different filesystems have different
> timestamp behaviours, and not all use the generic VFS code for
> timestamp updates. The filesystems need to use the correct helper
> functions to obtain a valid current time, but you can't stop them
> from storing and using arbitrary timestamp formats if they so
> desire...
I mean the decision whether to clamp or error on an overflow
should be done consistently. Having a global sysctl knob or
a compile-time option is better than having each file system
implementor take a guess at what users might prefer, if we can't
come up with a behavior (e.g. clamp all the time, or error out
all the time) that everybody agrees is always correct.
> > > > d. disable expired fs at compile time (Arnd's suggestion)
> > >
> > > Not really an option, because it means we can't use filesystems that
> > > interop with other systems (e.g. cameras, etc) because they won't
> > > support y2038k timestamps for a long time, if ever (e.g. vfat).
> >
> > Let me clarify what my idea is here: I want a global kernel option
> > that disables all code that has known y2038 issues. If anyone tries
> > to build an embedded system with support beyond 2038, that should
> > disable all of those things, including file systems, drivers and
> > system calls, so we can reasonably assume that everything that works
> > today with that kernel build will keep working in the future and
> > not break in random ways.
>
> It's not that black and white when it comes to filesystems. y2038k
> support is determined by the on-disk structure of the filesystem
> being mounted, and that is determined at mount time. When the
> filesystem mounts and sets it's valid timestamp ranges the VFS
> will need to decide as to whether the filesystem is allowed to
> continue mounting or not.
Some file systems are always broken around 2038 (e.g. HFS in 2040),
so if we can't fix them, I want to be able to turn them off
in Kconfig along with the 32-bit time_t syscalls. For those
where y2038 support depends on on-disk feature flags (ext4 and
xfs I guess, maybe one or two more), the config option can
turn off write support for the old format.
This is a rather important part of the y2038 work: If anyone cares about
y2038 problems in this decade, they want to deploy systems with extremely
long service contracts and don't want to update them 20 years from now
to fix an obscure bug that could be prevented today, so we try hard to
identify every line of code that won't work then as it does today.
> > For a file system, this can be done in a number of ways:
> >
> > * Most file systems today interpret the time as an unsigned 32-bit
> > number (as opposed to signed as ext3, xfs and few others do),
> > so as long as we use timespec64 in the syscalls, we are ok.
>
> Actually, sign conversion is a problem we currently have to be very
> careful of. See, for example, xfstests:tests/generic/258, which
> tests timestamps recording times before epoch. i.e. in XFS we have
> to convert the unsigned 32 bit disk timestamp to signed 32 bit
> before storing it in the VFS timestamp so it behaves correctly on 64
> bit systems. This results in us needing to do this when reading the
> inode from disk:
>
> + /*
> + * time is signed, so need to convert to signed 32 bit before
> + * storing in inode timestamp which may be 64 bit. Otherwise
> + * a time before epoch is converted to a time long after epoch
> + * on 64 bit systems.
> + */
> + inode->i_atime.tv_sec = (int)be32_to_cpu(from->di_atime.t_sec);
> + inode->i_atime.tv_nsec = (int)be32_to_cpu(from->di_atime.t_nsec);
> + inode->i_mtime.tv_sec = (int)be32_to_cpu(from->di_mtime.t_sec);
> + inode->i_mtime.tv_nsec = (int)be32_to_cpu(from->di_mtime.t_nsec);
> + inode->i_ctime.tv_sec = (int)be32_to_cpu(from->di_ctime.t_sec);
> + inode->i_ctime.tv_nsec = (int)be32_to_cpu(from->di_ctime.t_nsec);
> +
> (http://oss.sgi.com/archives/xfs/2016-01/msg00456.html)
I'm very aware of this issue. Most file system developers however were
not, so e.g. a timestamp on btrfs is interpreted differently on 32-bit
and 64-bit kernels. Some file systems (AFS, NFSv3?) explicitly define
the timestamps as unsigned, and most others that store 32-bit seconds
apparently never thought about the issue and happen to use unsigned
interpretation on 64-bit systems, since that is what you get out of
be32_to_cpu() without adding the cast.
For the y2038 case, this means we are lucky: almost all the users today
have 64-bit hardware, so they can already represent timestamps in the
range from 1970 to 2106. Once we have 64-bit timestamps in 32-bit user
space, we just need to make that use the same format we use on the
64-bit machines already.
ext2/3/4, xfs and ocfs2 (maybe one or two more, I'd have to check)
currently behave in a consistent manner across 32-bit and 64-bit
architectures by allowing a range between 1902 and 2037, and we
obviously don't have a choice there but to keep that current
behavior, and extend the time format in one way or another to
store additional bits for the epoch.
> > * Some legacy file systems (maybe hfs) can remain disabled, as
> > nobody cares about them any more.
> >
> > * If we still care about them (e.g. ext2), we can make them support
> > only read-only mode. In ext4, this would mean forbidding write
> > access to file systems that don't have the extended inode format
> > enabled.
>
> For ext2/4, that would have to be handled internally by the
> filesystem with feature masks. For other legacy filesystems, then
> the VFS mount time checking could allow RO mounts if the supported
> ranges are not y2038k clean. Compile time options are not
> really the best approach here...
I'm not following the line of thought here. We have some users
that want ext4 to mount old file system images without long
inodes writable, because they don't care about the 2038 problem.
We also have other users that want to force the same file system
image to be read-only because they want to ensure that it does
not stop working correctly when the time overflow happens while
the fs is mounted.
If you don't want a compile-time option for it, how do you suggest
we decide which case we have?
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-15 00:00 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qQZ22-5mm-1@gated-at.bofh.it> |
| In reply to | #1309734 |
On Thursday 14 January 2016 23:46:16 Arnd Bergmann wrote: > > I'm not following the line of thought here. We have some users > that want ext4 to mount old file system images without long > inodes writable, because they don't care about the 2038 problem. > We also have other users that want to force the same file system > image to be read-only because they want to ensure that it does > not stop working correctly when the time overflow happens while > the fs is mounted. > > If you don't want a compile-time option for it, how do you suggest > we decide which case we have? In case that came across wrong, I'm assuming that the first user also wants all the system calls enabled that pass 32-bit time_t values, while the second one wants them all left out from the kernel to ensure that no user space program gets incorrect data. This could be done using a sysctl of course, but I still think we want a compile-time option for the syscalls for clarity, and I would simply use the same compile-time option to determine the behavior of the file system, network protocols and device drivers that deal with 32-bit timestamps outside of the kernel. Arnd
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-15 03:30 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qR2jg-7U0-15@gated-at.bofh.it> |
| In reply to | #1309738 |
On Thu, Jan 14, 2016 at 11:54:36PM +0100, Arnd Bergmann wrote: > On Thursday 14 January 2016 23:46:16 Arnd Bergmann wrote: > > > > I'm not following the line of thought here. We have some users > > that want ext4 to mount old file system images without long > > inodes writable, because they don't care about the 2038 problem. > > We also have other users that want to force the same file system > > image to be read-only because they want to ensure that it does > > not stop working correctly when the time overflow happens while > > the fs is mounted. > > > > If you don't want a compile-time option for it, how do you suggest > > we decide which case we have? > > In case that came across wrong, I'm assuming that the first > user also wants all the system calls enabled that pass 32-bit > time_t values, while the second one wants them all left out from > the kernel to ensure that no user space program gets incorrect > data. system call API support is a completely different class of problem. It's out of the scope of this patchset, and really I don't care what you do with them. The point I'm making is that we'll have to modify all the existing filesystem code to supply a valid timestamp range to the VFS at mount time for the range checking/clamping, similar to how we do the granularity specification right now. That means we can do rejection of non-y2038k compliant filesystems at runtime based on what the filesystem tells the VFS it supports.. Set up the default to be reject if rw, allow if ro, and provide a mount option to override ad allow mounting rw. Users can then make the decision when mounting their filesystems. If they are system/automatically mounted filesystems and aren't y2038k compliant, then the override option can be added to /etc/fstab and we're all good. If the truly paranoid users want to disallow the override and/or read only mount options, then add a sysctl to control that. Cheers, Dave. -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-15 18:10 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qRg2T-Ih-35@gated-at.bofh.it> |
| In reply to | #1309824 |
On Friday 15 January 2016 13:27:34 Dave Chinner wrote: > On Thu, Jan 14, 2016 at 11:54:36PM +0100, Arnd Bergmann wrote: > > On Thursday 14 January 2016 23:46:16 Arnd Bergmann wrote: > > > > > > I'm not following the line of thought here. We have some users > > > that want ext4 to mount old file system images without long > > > inodes writable, because they don't care about the 2038 problem. > > > We also have other users that want to force the same file system > > > image to be read-only because they want to ensure that it does > > > not stop working correctly when the time overflow happens while > > > the fs is mounted. > > > > > > If you don't want a compile-time option for it, how do you suggest > > > we decide which case we have? > > > > In case that came across wrong, I'm assuming that the first > > user also wants all the system calls enabled that pass 32-bit > > time_t values, while the second one wants them all left out from > > the kernel to ensure that no user space program gets incorrect > > data. > > system call API support is a completely different class of problem. > It's out of the scope of this patchset, and really I don't care what > you do with them. Sure, I was just providing some background about why we want a compile-time option in general. > The point I'm making is that we'll have to modify all the existing > filesystem code to supply a valid timestamp range to the VFS at > mount time for the range checking/clamping, similar to how we do the > granularity specification right now. That means we can do rejection > of non-y2038k compliant filesystems at runtime based on what the > filesystem tells the VFS it supports.. Set up the default to be > reject if rw, allow if ro, and provide a mount option to override ad > allow mounting rw. We can't really default to "reject if rw", because that would break all systems using ext3 or xfs, unless users modify their fstab or set the flag that makes the partition y2038 compliant. The compile-time option that I'm thinking of would change the default beween "always allow" and "reject if rw", based on whether the system cares about this issue or not. Almost everyone today won't care about it at all and would be rather annoyed by being unable to mount their rootfs, but some people care about the behavior a lot. Having a global sysctl or mount option as an override would be good, maybe both if that isn't over-engineering the problem when we already have a compile-time option. Arnd
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-15 23:50 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qRllU-42W-13@gated-at.bofh.it> |
| In reply to | #1310286 |
On Fri, Jan 15, 2016 at 06:01:36PM +0100, Arnd Bergmann wrote: > On Friday 15 January 2016 13:27:34 Dave Chinner wrote: > > The point I'm making is that we'll have to modify all the existing > > filesystem code to supply a valid timestamp range to the VFS at > > mount time for the range checking/clamping, similar to how we do the > > granularity specification right now. That means we can do rejection > > of non-y2038k compliant filesystems at runtime based on what the > > filesystem tells the VFS it supports.. Set up the default to be > > reject if rw, allow if ro, and provide a mount option to override ad > > allow mounting rw. > > We can't really default to "reject if rw", because that would break > all systems using ext3 or xfs, unless users modify their fstab > or set the flag that makes the partition y2038 compliant. Right, I was refering to the behaviour of a y2038k compliant kernel, A current non-compliant kernel will have the default behaviour you are suggesting. > The compile-time option that I'm thinking of would change the default > beween "always allow" and "reject if rw", based on whether the > system cares about this issue or not. Almost everyone today won't > care about it at all and would be rather annoyed by being unable > to mount their rootfs, but some people care about the behavior > a lot. Yup, that's exactly what I was implying. > Having a global sysctl or mount option as an override would be good, > maybe both if that isn't over-engineering the problem when we already > have a compile-time option. Distros should not be forces to ship multiple kernels just to provide all the different runtime compliance behaviours their users require. Make the policy runtime enforcable, but select the default behaviour and supported policies via compile time options. Cheers, Dave -- Dave Chinner david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2016-01-15 04:00 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qR2Mi-85x-5@gated-at.bofh.it> |
| In reply to | #1309734 |
On Thu, Jan 14, 2016 at 11:46:16PM +0100, Arnd Bergmann wrote:
> On Friday 15 January 2016 08:00:01 Dave Chinner wrote:
> > On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote:
> > > On Thursday 14 January 2016 08:04:36 Dave Chinner wrote:
> > > > On Wed, Jan 13, 2016 at 08:33:16AM -0800, Deepa Dinamani wrote:
> > > c) The opposite direction from b) is to first change the common
> > > code, but then any direct assignment between a timespec in
> > > a file system and the timespec64 in the inode/iattr/kstat/etc
> > > first needs a conversion helper so we can build cleanly,
> > > and then we do one file system at a time to remove them all
> > > again while changing the internal structures in the
> > > file system from timespec to timespec64.
> >
> > No new helpers are necessary - we've already got the helper
> > functions we need. This:
> >
> > int simple_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
> > {
> > struct inode *inode = d_inode(old_dentry);
> > + struct inode_timespec now = current_fs_time(inode->i_sb);
> >
> > - inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> > + VFS_INODE_SET_XTIME(i_ctime, inode, now);
> > + VFS_INODE_SET_XTIME(i_mtime, dir, now);
> > + VFS_INODE_SET_XTIME(i_ctime, dir, now);
> > inc_nlink(inode);
> > .....
> >
> > is just wrong. All the type conversion and clamping and checking
> > done in that VFS_INODE_SET_XTIME() should be done in
> > current_fs_time() and have it return a timespec64 directly. Indeed,
> > it already does truncation, and can easily be made to do range
> > clamping, too. i.e. the change should simply be:
> >
> > - inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> > + inode->i_ctime = dir->i_ctime = dir->i_mtime = current_fs_time(inode->i_sb);
>
>
> Yes, that is the obvious case, and I guess works for at least half the
> file systems when they always assign righthand side and lefthand
> side of the time stamps using the external types or helpers like
> CURRENT_TIME and current_fs_time().
>
> However, there are a couple of file systems that need a bit more refactoring
> before we can do this, e.g. in ntfs_truncate:
Sure, and nfs is a pain because of all it's internal use of
timespecs, too. But we have these timespec_to_timespec64 helper
functions, and that's what we should use in these cases where the
filesystem cannot support full 64 bit timestamps internally. In
those cases, they'll be telling the superblock this at mount time
things like current_fs_time() won't be returning then a timestamp
that is out of range for a 32 bit timestamp....
> if (!IS_NOCMTIME(VFS_I(base_ni)) && !IS_RDONLY(VFS_I(base_ni))) {
> struct timespec now = current_fs_time(VFS_I(base_ni)->i_sb);
> int sync_it = 0;
>
> if (!timespec_equal(&VFS_I(base_ni)->i_mtime, &now) ||
> !timespec_equal(&VFS_I(base_ni)->i_ctime, &now))
> sync_it = 1;
> VFS_I(base_ni)->i_mtime = now;
> VFS_I(base_ni)->i_ctime = now;
> }
>
> The type of the local variable must match the return code of
> current_fs_time(), so if we change over i_mtime and current_fs_time
> globally, this either has to be rewritten first to avoid the use of
> local variables, or it needs temporary conversion helpers, or
> it has to be changed in the same patch. None of those is particularly
> appealing. There are a few dozen such things in various file systems.
it gets rewritten to:
struct timespec now;
now = timespec64_to_timespec(current_fs_time(VFS_I(base_ni)->i_sb));
....
and the valid timestamp range for ntfs is set to 32bit timestamps.
This then leaves it up to the filesystem developers to make
the ntfs filesystem code 64 bit timestamp clean if the on disk
format is ever changed to support 64 bit times.
Same goes for NFS, and any of the other filesystems that use struct
timespec internally for time representation.
> > > > I think it's a mix - if the timestamps come in from userspace,
> > > > fail with ERANGE. That could be controlled by sysctl via VFS
> > > > part of the ->setattr operation, or in each of the individual FS
> > > > implementations. If they come from the kernel (e.g. atime update) then
> > > > the generic behvaiour is to warn and continue, filesystems can
> > > > otherwise select their own policy for kernel updates via
> > > > ->update_time.
> > >
> > > I'd prefer not to have it done by the individual file system
> > > implementation, so we get a consistent behavior.
> >
> > Can't be helped, because different filesystems have different
> > timestamp behaviours, and not all use the generic VFS code for
> > timestamp updates. The filesystems need to use the correct helper
> > functions to obtain a valid current time, but you can't stop them
> > from storing and using arbitrary timestamp formats if they so
> > desire...
>
> I mean the decision whether to clamp or error on an overflow
> should be done consistently.
Sure, but that comes from using the helpers we already have, and
applying the clamping at a point where errors can be returned to
userspace. current_fs_time() should never return a timestamp outside
what the filesystem has said it supports and we've validated that
behaviour at mount time. hence it's only user provided timestamps
that we need range errors on.
> Having a global sysctl knob or
> a compile-time option is better than having each file system
> implementor take a guess at what users might prefer, if we can't
> come up with a behavior (e.g. clamp all the time, or error out
> all the time) that everybody agrees is always correct.
filesystem implementors will use the helper funtions that are
provided for this. If they don't (like all the current use of
CURRENT_TIME), then that's a bug that needs fixing. i.e. we need
a timespec_clamp() function, similar to timespec_trunc(), and y2038k
compliant filesystems and syscalls need to use them....
> > > >
> > > > Not really an option, because it means we can't use filesystems that
> > > > interop with other systems (e.g. cameras, etc) because they won't
> > > > support y2038k timestamps for a long time, if ever (e.g. vfat).
> > >
> > > Let me clarify what my idea is here: I want a global kernel option
> > > that disables all code that has known y2038 issues. If anyone tries
> > > to build an embedded system with support beyond 2038, that should
> > > disable all of those things, including file systems, drivers and
> > > system calls, so we can reasonably assume that everything that works
> > > today with that kernel build will keep working in the future and
> > > not break in random ways.
> >
> > It's not that black and white when it comes to filesystems. y2038k
> > support is determined by the on-disk structure of the filesystem
> > being mounted, and that is determined at mount time. When the
> > filesystem mounts and sets it's valid timestamp ranges the VFS
> > will need to decide as to whether the filesystem is allowed to
> > continue mounting or not.
>
> Some file systems are always broken around 2038 (e.g. HFS in 2040),
> so if we can't fix them, I want to be able to turn them off
> in Kconfig along with the 32-bit time_t syscalls.
That can be done with kconfig depends rules - it has nothing to do
with this patch set.
> ext2/3/4, xfs and ocfs2 (maybe one or two more, I'd have to check)
> currently behave in a consistent manner across 32-bit and 64-bit
> architectures by allowing a range between 1902 and 2037, and we
> obviously don't have a choice there but to keep that current
> behavior, and extend the time format in one way or another to
> store additional bits for the epoch.
That's a filesystem implementation problem, not a generic inode
timestamp problem. i.e. this is handled when the filesystem converts
the inode timestamp from a timespec64 in the struct inode to
whatever format it stores the timestamp on disk. That conversion
does not change just because the VFS inode moves from a timespec to
a timespec64. Again, those on-disk format changes to support beyond
the current epoch are outside the scope of this patchset, because
they are not affected by the timestamp format the VFS choses to use.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2016-01-15 18:00 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qRfTb-p1-1@gated-at.bofh.it> |
| In reply to | #1309837 |
On Friday 15 January 2016 13:49:37 Dave Chinner wrote:
> On Thu, Jan 14, 2016 at 11:46:16PM +0100, Arnd Bergmann wrote:
> > On Friday 15 January 2016 08:00:01 Dave Chinner wrote:
> > > On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote:
> > > > On Thursday 14 January 2016 08:04:36 Dave Chinner wrote:
> >
> > Yes, that is the obvious case, and I guess works for at least half the
> > file systems when they always assign righthand side and lefthand
> > side of the time stamps using the external types or helpers like
> > CURRENT_TIME and current_fs_time().
> >
> > However, there are a couple of file systems that need a bit more refactoring
> > before we can do this, e.g. in ntfs_truncate:
>
> Sure, and nfs is a pain because of all it's internal use of
> timespecs, too.
lustre is probably the worst.
> But we have these timespec_to_timespec64 helper
> functions, and that's what we should use in these cases where the
> filesystem cannot support full 64 bit timestamps internally. In
> those cases, they'll be telling the superblock this at mount time
> things like current_fs_time() won't be returning then a timestamp
> that is out of range for a 32 bit timestamp....
I'm not worried about the runtime problems here, just how to
get a series of patches that are each doing one reasonable thing
at a time.
> > if (!IS_NOCMTIME(VFS_I(base_ni)) && !IS_RDONLY(VFS_I(base_ni))) {
> > struct timespec now = current_fs_time(VFS_I(base_ni)->i_sb);
> > int sync_it = 0;
> >
> > if (!timespec_equal(&VFS_I(base_ni)->i_mtime, &now) ||
> > !timespec_equal(&VFS_I(base_ni)->i_ctime, &now))
> > sync_it = 1;
> > VFS_I(base_ni)->i_mtime = now;
> > VFS_I(base_ni)->i_ctime = now;
> > }
> >
> > The type of the local variable must match the return code of
> > current_fs_time(), so if we change over i_mtime and current_fs_time
> > globally, this either has to be rewritten first to avoid the use of
> > local variables, or it needs temporary conversion helpers, or
> > it has to be changed in the same patch. None of those is particularly
> > appealing. There are a few dozen such things in various file systems.
>
> it gets rewritten to:
>
> struct timespec now;
>
> now = timespec64_to_timespec(current_fs_time(VFS_I(base_ni)->i_sb));
> ....
>
> and the valid timestamp range for ntfs is set to 32bit timestamps.
> This then leaves it up to the filesystem developers to make
> the ntfs filesystem code 64 bit timestamp clean if the on disk
> format is ever changed to support 64 bit times.
>
> Same goes for NFS, and any of the other filesystems that use struct
> timespec internally for time representation.
That is what I meant in my previous mail about approach c) being ugly
because it requires sprinkling lots of timespec64_to_timespec() and
timespec_to_timespec64() in the initial patch in order to atomically
change the types in inode/iattr/kstat/... without introducing build
regressions.
It's a rather horrible patch, and quite likely to cause conflicts with
other patches that introduce another use of those structures in the
merge window.
> > Having a global sysctl knob or
> > a compile-time option is better than having each file system
> > implementor take a guess at what users might prefer, if we can't
> > come up with a behavior (e.g. clamp all the time, or error out
> > all the time) that everybody agrees is always correct.
>
> filesystem implementors will use the helper funtions that are
> provided for this. If they don't (like all the current use of
> CURRENT_TIME), then that's a bug that needs fixing.
Ok, then we are in total agreement here: the policy remains
to be decided by common code, but the implementation can differ
per file system.
> i.e. we need
> a timespec_clamp() function, similar to timespec_trunc(), and y2038k
> compliant filesystems and syscalls need to use them....
I was thinking we end up with a single function that does both
clamp() and trunk(), but that's an implementation detail.
> > > > Let me clarify what my idea is here: I want a global kernel option
> > > > that disables all code that has known y2038 issues. If anyone tries
> > > > to build an embedded system with support beyond 2038, that should
> > > > disable all of those things, including file systems, drivers and
> > > > system calls, so we can reasonably assume that everything that works
> > > > today with that kernel build will keep working in the future and
> > > > not break in random ways.
> > >
> > > It's not that black and white when it comes to filesystems. y2038k
> > > support is determined by the on-disk structure of the filesystem
> > > being mounted, and that is determined at mount time. When the
> > > filesystem mounts and sets it's valid timestamp ranges the VFS
> > > will need to decide as to whether the filesystem is allowed to
> > > continue mounting or not.
> >
> > Some file systems are always broken around 2038 (e.g. HFS in 2040),
> > so if we can't fix them, I want to be able to turn them off
> > in Kconfig along with the 32-bit time_t syscalls.
>
> That can be done with kconfig depends rules - it has nothing to do
> with this patch set.
kconfig dependencies is what I meant for the simple cases where a
file system is known to always be broken, we just need a small
modification for the cases you mentioned below.
> > ext2/3/4, xfs and ocfs2 (maybe one or two more, I'd have to check)
> > currently behave in a consistent manner across 32-bit and 64-bit
> > architectures by allowing a range between 1902 and 2037, and we
> > obviously don't have a choice there but to keep that current
> > behavior, and extend the time format in one way or another to
> > store additional bits for the epoch.
>
> That's a filesystem implementation problem, not a generic inode
> timestamp problem. i.e. this is handled when the filesystem converts
> the inode timestamp from a timespec64 in the struct inode to
> whatever format it stores the timestamp on disk. That conversion
> does not change just because the VFS inode moves from a timespec to
> a timespec64. Again, those on-disk format changes to support beyond
> the current epoch are outside the scope of this patchset, because
> they are not affected by the timestamp format the VFS choses to use.
Fine with me, we can have another series to add the Kconfig dependencies
and modify the file systems that need this.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Andreas Dilger <adilger@dilger.ca> |
|---|---|
| Date | 2016-01-16 20:20 +0100 |
| Subject | Re: [Y2038] [RFC 02/15] vfs: Change all structures to support 64 bit time |
| Message-ID | <qREyd-ed-1@gated-at.bofh.it> |
| In reply to | #1310272 |
[Multipart message — attachments visible in raw view] — view raw
> On Jan 15, 2016, at 9:50 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Friday 15 January 2016 13:49:37 Dave Chinner wrote:
>> On Thu, Jan 14, 2016 at 11:46:16PM +0100, Arnd Bergmann wrote:
>>> On Friday 15 January 2016 08:00:01 Dave Chinner wrote:
>>>> On Thu, Jan 14, 2016 at 05:53:21PM +0100, Arnd Bergmann wrote:
>>>>> On Thursday 14 January 2016 08:04:36 Dave Chinner wrote:
>>>
>>> Yes, that is the obvious case, and I guess works for at least half the
>>> file systems when they always assign righthand side and lefthand
>>> side of the time stamps using the external types or helpers like
>>> CURRENT_TIME and current_fs_time().
>>>
>>> However, there are a couple of file systems that need a bit more refactoring
>>> before we can do this, e.g. in ntfs_truncate:
>>
>> Sure, and nfs is a pain because of all it's internal use of
>> timespecs, too.
>
> lustre is probably the worst.
Lustre currently only has one-second granularity in a 64-bit field,
so it doesn't really care about the difference between timespec or
timespec64 at all.
The only other uses are for measuring relative times, so the 64-bitness
shouldn't really matter.
Could you please point out what issues exist so they can be fixed.
Cheers, Andreas
>> But we have these timespec_to_timespec64 helper
>> functions, and that's what we should use in these cases where the
>> filesystem cannot support full 64 bit timestamps internally. In
>> those cases, they'll be telling the superblock this at mount time
>> things like current_fs_time() won't be returning then a timestamp
>> that is out of range for a 32 bit timestamp....
>
> I'm not worried about the runtime problems here, just how to
> get a series of patches that are each doing one reasonable thing
> at a time.
>
>>> if (!IS_NOCMTIME(VFS_I(base_ni)) && !IS_RDONLY(VFS_I(base_ni))) {
>>> struct timespec now = current_fs_time(VFS_I(base_ni)->i_sb);
>>> int sync_it = 0;
>>>
>>> if (!timespec_equal(&VFS_I(base_ni)->i_mtime, &now) ||
>>> !timespec_equal(&VFS_I(base_ni)->i_ctime, &now))
>>> sync_it = 1;
>>> VFS_I(base_ni)->i_mtime = now;
>>> VFS_I(base_ni)->i_ctime = now;
>>> }
>>>
>>> The type of the local variable must match the return code of
>>> current_fs_time(), so if we change over i_mtime and current_fs_time
>>> globally, this either has to be rewritten first to avoid the use of
>>> local variables, or it needs temporary conversion helpers, or
>>> it has to be changed in the same patch. None of those is particularly
>>> appealing. There are a few dozen such things in various file systems.
>>
>> it gets rewritten to:
>>
>> struct timespec now;
>>
>> now = timespec64_to_timespec(current_fs_time(VFS_I(base_ni)->i_sb));
>> ....
>>
>> and the valid timestamp range for ntfs is set to 32bit timestamps.
>> This then leaves it up to the filesystem developers to make
>> the ntfs filesystem code 64 bit timestamp clean if the on disk
>> format is ever changed to support 64 bit times.
>>
>> Same goes for NFS, and any of the other filesystems that use struct
>> timespec internally for time representation.
>
> That is what I meant in my previous mail about approach c) being ugly
> because it requires sprinkling lots of timespec64_to_timespec() and
> timespec_to_timespec64() in the initial patch in order to atomically
> change the types in inode/iattr/kstat/... without introducing build
> regressions.
>
> It's a rather horrible patch, and quite likely to cause conflicts with
> other patches that introduce another use of those structures in the
> merge window.
>
>>> Having a global sysctl knob or
>>> a compile-time option is better than having each file system
>>> implementor take a guess at what users might prefer, if we can't
>>> come up with a behavior (e.g. clamp all the time, or error out
>>> all the time) that everybody agrees is always correct.
>>
>> filesystem implementors will use the helper funtions that are
>> provided for this. If they don't (like all the current use of
>> CURRENT_TIME), then that's a bug that needs fixing.
>
> Ok, then we are in total agreement here: the policy remains
> to be decided by common code, but the implementation can differ
> per file system.
>
>> i.e. we need
>> a timespec_clamp() function, similar to timespec_trunc(), and y2038k
>> compliant filesystems and syscalls need to use them....
>
> I was thinking we end up with a single function that does both
> clamp() and trunk(), but that's an implementation detail.
>>>>> Let me clarify what my idea is here: I want a global kernel option
>>>>> that disables all code that has known y2038 issues. If anyone tries
>>>>> to build an embedded system with support beyond 2038, that should
>>>>> disable all of those things, including file systems, drivers and
>>>>> system calls, so we can reasonably assume that everything that works
>>>>> today with that kernel build will keep working in the future and
>>>>> not break in random ways.
>>>>
>>>> It's not that black and white when it comes to filesystems. y2038k
>>>> support is determined by the on-disk structure of the filesystem
>>>> being mounted, and that is determined at mount time. When the
>>>> filesystem mounts and sets it's valid timestamp ranges the VFS
>>>> will need to decide as to whether the filesystem is allowed to
>>>> continue mounting or not.
>>>
>>> Some file systems are always broken around 2038 (e.g. HFS in 2040),
>>> so if we can't fix them, I want to be able to turn them off
>>> in Kconfig along with the 32-bit time_t syscalls.
>>
>> That can be done with kconfig depends rules - it has nothing to do
>> with this patch set.
>
> kconfig dependencies is what I meant for the simple cases where a
> file system is known to always be broken, we just need a small
> modification for the cases you mentioned below.
>
>>> ext2/3/4, xfs and ocfs2 (maybe one or two more, I'd have to check)
>>> currently behave in a consistent manner across 32-bit and 64-bit
>>> architectures by allowing a range between 1902 and 2037, and we
>>> obviously don't have a choice there but to keep that current
>>> behavior, and extend the time format in one way or another to
>>> store additional bits for the epoch.
>>
>> That's a filesystem implementation problem, not a generic inode
>> timestamp problem. i.e. this is handled when the filesystem converts
>> the inode timestamp from a timespec64 in the struct inode to
>> whatever format it stores the timestamp on disk. That conversion
>> does not change just because the VFS inode moves from a timespec to
>> a timespec64. Again, those on-disk format changes to support beyond
>> the current epoch are outside the scope of this patchset, because
>> they are not affected by the timestamp format the VFS choses to use.
>
> Fine with me, we can have another series to add the Kconfig dependencies
> and modify the file systems that need this.
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers, Andreas
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web