| Message ID | 20230928110554.34758-2-jlayton@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, 2023-09-29 at 11:44 +0200, Christian Brauner wrote: > > It is a lot of churn though. > > I think that i_{a,c,m}time shouldn't be accessed directly by > filesystems same as no filesystem should really access i_{g,u}id which > we also provide i_{g,u}id_{read,write}() accessors for. The mode is > another example where really most often should use helpers because of all > the set*id stripping that we need to do (and the bugs that we had > because of this...). > > The interdependency between ctime and mtime is enough to hide this in > accessors. The other big advantage is simply grepability. So really I > would like to see this change even without the type switch. > > In other words, there's no need to lump the two changes together. Do the > conversion part and we can argue about the switch to discrete integers > separately. > > The other adavantage is that we have a cycle to see any possible > regression from the conversion. > > Thoughts anyone? That works for me, and sort of what I was planning anyway. I mostly just did the change to timestamp storage to see what it would look like afterward. FWIW, I'm planning to do a v2 patchbomb early next week, with the changes that Chuck suggested (specific helpers for fetching the _sec and _nsec fields). For now, I'll drop the change from timespec64 to discrete fields. We can do that in a separate follow-on set.
On Thu, 28 Sept 2023 at 20:50, Amir Goldstein <amir73il@gmail.com> wrote: > > OTOH, it is perfectly fine if the vfs wants to stop providing sub 100ns > services to filesystems. It's just going to be the fs problem and the > preserved pre-historic/fine-grained time on existing files would only > need to be provided in getattr(). It does not need to be in __i_mtime. Hmm. That sounds technically sane, but for one thing: if the aim is to try to do (a) atomic timestamp access (b) shrink the inode then having the filesystem maintain its own timestamp for fine-grained data will break both of those goals. Yes, we'd make 'struct inode' smaller if we pack the times into one 64-bit entity, but if btrfs responds by adding mtime fields to "struct btrfs_inode", we lost the size advantage and only made things worse. And if ->getattr() then reads those fields without locking (and we definitely don't want locking in that path), then we lost the atomicity thing too. So no. A "but the filesystem can maintain finer granularity" model is not acceptable, I think. If we do require nanoseconds for compatibility, what we could possibly do is say "we guarantee nanosecond values for *legacy* dates", and say that future dates use 100ns resolution. We'd define "legacy dates" to be the traditional 32-bit signed time_t. So with a 64-bit fstime_t, we'd have the "legacy format": - top 32 bits are seconds, bottom 32 bits are ns which gives us that ns format. Then, because only 30 bits are needed for nanosecond resolution, we use the top two bits of that ns field as flags. '00' means that legacy format, and '01' would mean "we're not doing nanosecond resolution, we're doing 64ns resolution, and the low 6 bits of the ns field are actually bits 32-37 of the seconds field". That still gives us some extensibility (unless the multi-grain code still wants to use the other top bit), and it gives us 40 bits of seconds, which is quite a lot. And all the conversion functions will be simple bit field manipulations, so there are no expensive ops here. Anyway, I agree with the "let's introduce the accessor functions first, we can do the 'pack into one word' decisions later". Linus
On Fri, Sep 29, 2023 at 3:06 AM David Howells via samba-technical <samba-technical@lists.samba.org> wrote: > > > Jeff Layton <jlayton@kernel.org> wrote: > > > Correct. We'd lose some fidelity in currently stored timestamps, but as > > Linus and Ted pointed out, anything below ~100ns granularity is > > effectively just noise, as that's the floor overhead for calling into > > the kernel. It's hard to argue that any application needs that sort of > > timestamp resolution, at least with contemporary hardware. > > Albeit with the danger of making Steve French very happy;-), would it make > sense to switch internally to Microsoft-style 64-bit timestamps with their > 100ns granularity? 100ns granularity does seem to make sense and IIRC was used by various DCE standards in the 90s and 2000s (not just used for SMB2/SMB3 protocol and various Windows filesystems)
On Sat, Sep 30, 2023 at 09:50:41AM -0500, Steve French wrote: > On Fri, Sep 29, 2023 at 3:06 AM David Howells via samba-technical > <samba-technical@lists.samba.org> wrote: > > > > > > Jeff Layton <jlayton@kernel.org> wrote: > > > > > Correct. We'd lose some fidelity in currently stored timestamps, but as > > > Linus and Ted pointed out, anything below ~100ns granularity is > > > effectively just noise, as that's the floor overhead for calling into > > > the kernel. It's hard to argue that any application needs that sort of > > > timestamp resolution, at least with contemporary hardware. > > > > Albeit with the danger of making Steve French very happy;-), would it make > > sense to switch internally to Microsoft-style 64-bit timestamps with their > > 100ns granularity? > > 100ns granularity does seem to make sense and IIRC was used by various > DCE standards in the 90s and 2000s (not just used for SMB2/SMB3 protocol and > various Windows filesystems) Historically it probably comes from VMS, where system time and file timestamps were a 64 bit integer counting in 100ns units starting on MJD 2400000.5 (Nov 17th 1858). Gabriel > > > -- > Thanks, > > Steve
diff --git a/include/linux/fs.h b/include/linux/fs.h index 831657011036..de902ff2938b 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -671,9 +671,12 @@ struct inode { }; dev_t i_rdev; loff_t i_size; - struct timespec64 __i_atime; /* use inode_*_atime accessors */ - struct timespec64 __i_mtime; /* use inode_*_mtime accessors */ - struct timespec64 __i_ctime; /* use inode_*_ctime accessors */ + time64_t i_atime_sec; + time64_t i_mtime_sec; + time64_t i_ctime_sec; + u32 i_atime_nsec; + u32 i_mtime_nsec; + u32 i_ctime_nsec; spinlock_t i_lock; /* i_blocks, i_bytes, maybe i_size */ unsigned short i_bytes; u8 i_blkbits; @@ -1519,7 +1522,9 @@ struct timespec64 inode_set_ctime_current(struct inode *inode); */ static inline struct timespec64 inode_get_ctime(const struct inode *inode) { - return inode->__i_ctime; + struct timespec64 ts = { .tv_sec = inode->i_ctime_sec, + .tv_nsec = inode->i_ctime_nsec }; + return ts; } /** @@ -1532,7 +1537,8 @@ static inline struct timespec64 inode_get_ctime(const struct inode *inode) static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode, struct timespec64 ts) { - inode->__i_ctime = ts; + inode->i_ctime_sec = ts.tv_sec; + inode->i_ctime_nsec = ts.tv_sec; return ts; } @@ -1555,13 +1561,17 @@ static inline struct timespec64 inode_set_ctime(struct inode *inode, static inline struct timespec64 inode_get_atime(const struct inode *inode) { - return inode->__i_atime; + struct timespec64 ts = { .tv_sec = inode->i_atime_sec, + .tv_nsec = inode->i_atime_nsec }; + + return ts; } static inline struct timespec64 inode_set_atime_to_ts(struct inode *inode, struct timespec64 ts) { - inode->__i_atime = ts; + inode->i_atime_sec = ts.tv_sec; + inode->i_atime_nsec = ts.tv_sec; return ts; } @@ -1575,13 +1585,17 @@ static inline struct timespec64 inode_set_atime(struct inode *inode, static inline struct timespec64 inode_get_mtime(const struct inode *inode) { - return inode->__i_mtime; + struct timespec64 ts = { .tv_sec = inode->i_mtime_sec, + .tv_nsec = inode->i_mtime_nsec }; + + return ts; } static inline struct timespec64 inode_set_mtime_to_ts(struct inode *inode, struct timespec64 ts) { - inode->__i_mtime = ts; + inode->i_atime_sec = ts.tv_sec; + inode->i_atime_nsec = ts.tv_sec; return ts; }
This shaves 8 bytes off struct inode, according to pahole. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- include/linux/fs.h | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)