| Message ID | 20230807-mgctime-v7-12-d1dec143a704@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: implement multigrain timestamps | expand |
On Mon, 2023-08-07 at 15:38 -0400, Jeff Layton wrote: > Enable multigrain timestamps, which should ensure that there is an > apparent change to the timestamp whenever it has been written after > being actively observed via getattr. > > For ext4, we only need to enable the FS_MGTIME flag. Hi Jeff, This patch causes a gnulib test failure: $ ~/sources/lfs/grep-3.11/gnulib-tests/test-stat-time test-stat-time.c:141: assertion 'statinfo[0].st_mtime < statinfo[2].st_mtime || (statinfo[0].st_mtime == statinfo[2].st_mtime && (get_stat_mtime_ns (&statinfo[0]) < get_stat_mtime_ns (&statinfo[2])))' failed Aborted (core dumped) The source code of the test: https://git.savannah.gnu.org/cgit/gnulib.git/tree/tests/test-stat-time.c Is this an expected change? > Acked-by: Theodore Ts'o <tytso@mit.edu> > Reviewed-by: Jan Kara <jack@suse.cz> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ext4/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index b54c70e1a74e..cb1ff47af156 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -7279,7 +7279,7 @@ static struct file_system_type ext4_fs_type = { > .init_fs_context = ext4_init_fs_context, > .parameters = ext4_param_specs, > .kill_sb = kill_block_super, > - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, > + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP | > FS_MGTIME, > }; > MODULE_ALIAS_FS("ext4"); > >
On Tue 19-09-23 15:05:24, Xi Ruoyao wrote: > On Mon, 2023-08-07 at 15:38 -0400, Jeff Layton wrote: > > Enable multigrain timestamps, which should ensure that there is an > > apparent change to the timestamp whenever it has been written after > > being actively observed via getattr. > > > > For ext4, we only need to enable the FS_MGTIME flag. > > Hi Jeff, > > This patch causes a gnulib test failure: > > $ ~/sources/lfs/grep-3.11/gnulib-tests/test-stat-time > test-stat-time.c:141: assertion 'statinfo[0].st_mtime < statinfo[2].st_mtime || (statinfo[0].st_mtime == statinfo[2].st_mtime && (get_stat_mtime_ns (&statinfo[0]) < get_stat_mtime_ns (&statinfo[2])))' failed > Aborted (core dumped) > > The source code of the test: > https://git.savannah.gnu.org/cgit/gnulib.git/tree/tests/test-stat-time.c > > Is this an expected change? Kind of yes. The test first tries to estimate filesystem timestamp granularity in nap() function - due to this patch, the detected granularity will likely be 1 ns so effectively all the test calls will happen immediately one after another. But we don't bother setting the timestamps with more than 1 jiffy (usually 4 ms) precision unless we think someone is watching. So as a result timestamps of all stamp1 and stamp2 files are going to be equal which makes the test fail. The ultimate problem is that a sequence like: write(f1) stat(f2) write(f2) stat(f2) write(f1) stat(f1) can result in f1 timestamp to be (slightly) lower than the final f2 timestamp because the second write to f1 didn't bother updating the timestamp. That can indeed be a bit confusing to programs if they compare timestamps between two files. Jeff? Honza > > Acked-by: Theodore Ts'o <tytso@mit.edu> > > Reviewed-by: Jan Kara <jack@suse.cz> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/ext4/super.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > > index b54c70e1a74e..cb1ff47af156 100644 > > --- a/fs/ext4/super.c > > +++ b/fs/ext4/super.c > > @@ -7279,7 +7279,7 @@ static struct file_system_type ext4_fs_type = { > > .init_fs_context = ext4_init_fs_context, > > .parameters = ext4_param_specs, > > .kill_sb = kill_block_super, > > - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, > > + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP | > > FS_MGTIME, > > }; > > MODULE_ALIAS_FS("ext4"); > > > > >
On Tue, 2023-09-19 at 13:04 +0200, Jan Kara wrote: > On Tue 19-09-23 15:05:24, Xi Ruoyao wrote: > > On Mon, 2023-08-07 at 15:38 -0400, Jeff Layton wrote: > > > Enable multigrain timestamps, which should ensure that there is an > > > apparent change to the timestamp whenever it has been written after > > > being actively observed via getattr. > > > > > > For ext4, we only need to enable the FS_MGTIME flag. > > > > Hi Jeff, > > > > This patch causes a gnulib test failure: > > > > $ ~/sources/lfs/grep-3.11/gnulib-tests/test-stat-time > > test-stat-time.c:141: assertion 'statinfo[0].st_mtime < statinfo[2].st_mtime || (statinfo[0].st_mtime == statinfo[2].st_mtime && (get_stat_mtime_ns (&statinfo[0]) < get_stat_mtime_ns (&statinfo[2])))' failed > > Aborted (core dumped) > > > > The source code of the test: > > https://git.savannah.gnu.org/cgit/gnulib.git/tree/tests/test-stat-time.c > > > > Is this an expected change? > > Kind of yes. The test first tries to estimate filesystem timestamp > granularity in nap() function - due to this patch, the detected granularity > will likely be 1 ns so effectively all the test calls will happen > immediately one after another. But we don't bother setting the timestamps > with more than 1 jiffy (usually 4 ms) precision unless we think someone is > watching. So as a result timestamps of all stamp1 and stamp2 files are > going to be equal which makes the test fail. > That was my take too. The multigrain ctime changes are probably causing nap() to settle on too small a time delta. > The ultimate problem is that a sequence like: > > write(f1) > stat(f2) > write(f2) > stat(f2) > write(f1) > stat(f1) > > can result in f1 timestamp to be (slightly) lower than the final f2 > timestamp because the second write to f1 didn't bother updating the > timestamp. That can indeed be a bit confusing to programs if they compare > timestamps between two files. Jeff? > Basically yes. When there is no stat() call issued on the file in between writes, the kernel will use coarse-grained timestamps when updating it (since no one is watching). I'm not sure what we can do for this test. The nap() function is making an assumption that the timestamp granularity will be constant, and that isn't necessarily the case now.
On Tue, 2023-09-19 at 16:52 +0200, Bruno Haible wrote: > Jeff Layton wrote: > > I'm not sure what we can do for this test. The nap() function is making > > an assumption that the timestamp granularity will be constant, and that > > isn't necessarily the case now. > > This is only of secondary importance, because the scenario by Jan Kara > shows a much more fundamental breakage: > > > > The ultimate problem is that a sequence like: > > > > > > write(f1) > > > stat(f2) > > > write(f2) > > > stat(f2) > > > write(f1) > > > stat(f1) > > > > > > can result in f1 timestamp to be (slightly) lower than the final f2 > > > timestamp because the second write to f1 didn't bother updating the > > > timestamp. That can indeed be a bit confusing to programs if they compare > > > timestamps between two files. Jeff? > > > > > > > Basically yes. > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > Many things in user-space depend on timestamps, such as build system > centered around 'make', but also 'find ... -newer ...'. > What does breakage with make look like in this situation? The "fuzz" here is going to be on the order of a jiffy. The typical case for make timestamp comparisons is comparing source files vs. a build target. If those are being written nearly simultaneously, then that could be an issue, but is that a typical behavior? It seems like it would be hard to rely on that anyway, esp. given filesystems like NFS that can do lazy writeback. One of the operating principles with this series is that timestamps can be of varying granularity between different files. Note that Linux already violates this assumption when you're working across filesystems of different types. As to potential fixes if this is a real problem: I don't really want to put this behind a mount or mkfs option (a'la relatime, etc.), but that is one possibility. I wonder if it would be feasible to just advance the coarse-grained current_time whenever we end up updating a ctime with a fine-grained timestamp? It might produce some inode write amplification. Files that were written within the same jiffy could see more inode transactions logged, but that still might not be _too_ awful. I'll keep thinking about it for now.
On 2023-09-19 09:31, Jeff Layton wrote: > The typical case for make > timestamp comparisons is comparing source files vs. a build target. If > those are being written nearly simultaneously, then that could be an > issue, but is that a typical behavior? I vaguely remember running into problems with 'make' a while ago (perhaps with a BSDish system) when filesystem timestamps were arbitrarily truncated in some cases but not others. These files would look older than they really were, so 'make' would think they were up-to-date when they weren't, and 'make' would omit actions that it should have done, thus screwing up the build. File timestamps can be close together with 'make -j' on fast hosts. Sometimes a shell script (or 'make' itself) will run 'make', then modify a file F, then immediately run 'make' again; the latter 'make' won't work if F's timestamp is mistakenly older than targets that depend on it. Although 'make'-like apps are the biggest canaries in this coal mine, the issue also affects 'find -newer' (as Bruno mentioned), 'rsync -u', 'mv -u', 'tar -u', Emacs file-newer-than-file-p, and surely many other places. For example, any app that creates a timestamp file, then backs up all files newer than that file, would be at risk. > I wonder if it would be feasible to just advance the coarse-grained > current_time whenever we end up updating a ctime with a fine-grained > timestamp? Wouldn't this need to be done globally, that is, not just on a per-file or per-filesystem basis? If so, I don't see how we'd avoid locking performance issues. PS. Although I'm no expert in the Linux inode code I hope you don't mind my asking a question about this part of inode_set_ctime_current: /* * If we've recently updated with a fine-grained timestamp, * then the coarse-grained one may still be earlier than the * existing ctime. Just keep the existing value if so. */ ctime.tv_sec = inode->__i_ctime.tv_sec; if (timespec64_compare(&ctime, &now) > 0) return ctime; Suppose root used clock_settime to set the clock backwards. Won't this code incorrectly refuse to update the file's timestamp afterwards? That is, shouldn't the last line be "goto fine_grained;" rather than "return ctime;", with the comment changed from "keep the existing value" to "use a fine-grained value"?
On Tue, 2023-09-19 at 13:10 -0700, Paul Eggert wrote: > On 2023-09-19 09:31, Jeff Layton wrote: > > The typical case for make > > timestamp comparisons is comparing source files vs. a build target. If > > those are being written nearly simultaneously, then that could be an > > issue, but is that a typical behavior? > > I vaguely remember running into problems with 'make' a while ago > (perhaps with a BSDish system) when filesystem timestamps were > arbitrarily truncated in some cases but not others. These files would > look older than they really were, so 'make' would think they were > up-to-date when they weren't, and 'make' would omit actions that it > should have done, thus screwing up the build. > > File timestamps can be close together with 'make -j' on fast hosts. > Sometimes a shell script (or 'make' itself) will run 'make', then modify > a file F, then immediately run 'make' again; the latter 'make' won't > work if F's timestamp is mistakenly older than targets that depend on it. > > Although 'make'-like apps are the biggest canaries in this coal mine, > the issue also affects 'find -newer' (as Bruno mentioned), 'rsync -u', > 'mv -u', 'tar -u', Emacs file-newer-than-file-p, and surely many other > places. For example, any app that creates a timestamp file, then backs > up all files newer than that file, would be at risk. > > > > I wonder if it would be feasible to just advance the coarse-grained > > current_time whenever we end up updating a ctime with a fine-grained > > timestamp? > > Wouldn't this need to be done globally, that is, not just on a per-file > or per-filesystem basis? If so, I don't see how we'd avoid locking > performance issues. > Maybe. Another idea might be to introduce a new timekeeper for multigrain filesystems, but all of those would likely have to share the same coarse-grained clock source. So yeah, if you stat an inode and then update it, any inode written on a multigrain filesystem within the same jiffy-sized window would have to log an extra transaction to write out the inode. That's what I meant when I was talking about write amplification. > > PS. Although I'm no expert in the Linux inode code I hope you don't mind > my asking a question about this part of inode_set_ctime_current: > > /* > * If we've recently updated with a fine-grained timestamp, > * then the coarse-grained one may still be earlier than the > * existing ctime. Just keep the existing value if so. > */ > ctime.tv_sec = inode->__i_ctime.tv_sec; > if (timespec64_compare(&ctime, &now) > 0) > return ctime; > > Suppose root used clock_settime to set the clock backwards. Won't this > code incorrectly refuse to update the file's timestamp afterwards? That > is, shouldn't the last line be "goto fine_grained;" rather than "return > ctime;", with the comment changed from "keep the existing value" to "use > a fine-grained value"? It is a problem, and Linus pointed that out yesterday, which is why I sent this earlier today: https://lore.kernel.org/linux-fsdevel/20230919-ctime-v1-1-97b3da92f504@kernel.org/T/#u Bear in mind that we're not dealing with a situation where the value has not been queried since its last update, so we don't need to use a fine grained timestamp there (and really, it's preferable not to do so). A coarse one should be fine in this case.
> > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > Many things in user-space depend on timestamps, such as build system > > centered around 'make', but also 'find ... -newer ...'. > > > > > What does breakage with make look like in this situation? The "fuzz" > here is going to be on the order of a jiffy. The typical case for make > timestamp comparisons is comparing source files vs. a build target. If > those are being written nearly simultaneously, then that could be an > issue, but is that a typical behavior? It seems like it would be hard to > rely on that anyway, esp. given filesystems like NFS that can do lazy > writeback. > > One of the operating principles with this series is that timestamps can > be of varying granularity between different files. Note that Linux > already violates this assumption when you're working across filesystems > of different types. > > As to potential fixes if this is a real problem: > > I don't really want to put this behind a mount or mkfs option (a'la > relatime, etc.), but that is one possibility. > > I wonder if it would be feasible to just advance the coarse-grained > current_time whenever we end up updating a ctime with a fine-grained > timestamp? It might produce some inode write amplification. Files that Less than ideal imho. If this risks breaking existing workloads by enabling it unconditionally and there isn't a clear way to detect and handle these situations without risk of regression then we should move this behind a mount option. So how about the following: From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 From: Christian Brauner <brauner@kernel.org> Date: Wed, 20 Sep 2023 10:00:08 +0200 Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option While we initially thought we can do this unconditionally it turns out that this might break existing workloads that rely on timestamps in very specific ways and we always knew this was a possibility. Move multi-grain timestamps behind a vfs mount option. Signed-off-by: Christian Brauner <brauner@kernel.org> --- fs/fs_context.c | 18 ++++++++++++++++++ fs/inode.c | 4 ++-- fs/proc_namespace.c | 1 + fs/stat.c | 2 +- include/linux/fs.h | 4 +++- 5 files changed, 25 insertions(+), 4 deletions(-) diff --git a/fs/fs_context.c b/fs/fs_context.c index a0ad7a0c4680..dd4dade0bb9e 100644 --- a/fs/fs_context.c +++ b/fs/fs_context.c @@ -44,6 +44,7 @@ static const struct constant_table common_set_sb_flag[] = { { "mand", SB_MANDLOCK }, { "ro", SB_RDONLY }, { "sync", SB_SYNCHRONOUS }, + { "mgtime", SB_MGTIME }, { }, }; @@ -52,18 +53,32 @@ static const struct constant_table common_clear_sb_flag[] = { { "nolazytime", SB_LAZYTIME }, { "nomand", SB_MANDLOCK }, { "rw", SB_RDONLY }, + { "nomgtime", SB_MGTIME }, { }, }; +static inline int check_mgtime(unsigned int token, const struct fs_context *fc) +{ + if (token != SB_MGTIME) + return 0; + if (!(fc->fs_type->fs_flags & FS_MGTIME)) + return invalf(fc, "Filesystem doesn't support multi-grain timestamps"); + return 0; +} + /* * Check for a common mount option that manipulates s_flags. */ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) { unsigned int token; + int ret; token = lookup_constant(common_set_sb_flag, key, 0); if (token) { + ret = check_mgtime(token, fc); + if (ret) + return ret; fc->sb_flags |= token; fc->sb_flags_mask |= token; return 0; @@ -71,6 +86,9 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) token = lookup_constant(common_clear_sb_flag, key, 0); if (token) { + ret = check_mgtime(token, fc); + if (ret) + return ret; fc->sb_flags &= ~token; fc->sb_flags_mask |= token; return 0; diff --git a/fs/inode.c b/fs/inode.c index 54237f4242ff..fd1a2390aaa3 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2141,7 +2141,7 @@ EXPORT_SYMBOL(current_mgtime); static struct timespec64 current_ctime(struct inode *inode) { - if (is_mgtime(inode)) + if (IS_MGTIME(inode)) return current_mgtime(inode); return current_time(inode); } @@ -2588,7 +2588,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode) now = current_time(inode); /* Just copy it into place if it's not multigrain */ - if (!is_mgtime(inode)) { + if (!IS_MGTIME(inode)) { inode_set_ctime_to_ts(inode, now); return now; } diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c index 250eb5bf7b52..08f5bf4d2c6c 100644 --- a/fs/proc_namespace.c +++ b/fs/proc_namespace.c @@ -49,6 +49,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb) { SB_DIRSYNC, ",dirsync" }, { SB_MANDLOCK, ",mand" }, { SB_LAZYTIME, ",lazytime" }, + { SB_MGTIME, ",mgtime" }, { 0, NULL } }; const struct proc_fs_opts *fs_infop; diff --git a/fs/stat.c b/fs/stat.c index 6e60389d6a15..2f18dd5de18b 100644 --- a/fs/stat.c +++ b/fs/stat.c @@ -90,7 +90,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask, stat->size = i_size_read(inode); stat->atime = inode->i_atime; - if (is_mgtime(inode)) { + if (IS_MGTIME(inode)) { fill_mg_cmtime(stat, request_mask, inode); } else { stat->mtime = inode->i_mtime; diff --git a/include/linux/fs.h b/include/linux/fs.h index 4aeb3fa11927..03e415fb3a7c 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1114,6 +1114,7 @@ extern int send_sigurg(struct fown_struct *fown); #define SB_NODEV BIT(2) /* Disallow access to device special files */ #define SB_NOEXEC BIT(3) /* Disallow program execution */ #define SB_SYNCHRONOUS BIT(4) /* Writes are synced at once */ +#define SB_MGTIME BIT(5) /* Use multi-grain timestamps */ #define SB_MANDLOCK BIT(6) /* Allow mandatory locks on an FS */ #define SB_DIRSYNC BIT(7) /* Directory modifications are synchronous */ #define SB_NOATIME BIT(10) /* Do not update access times. */ @@ -2105,6 +2106,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) #define IS_MANDLOCK(inode) __IS_FLG(inode, SB_MANDLOCK) #define IS_NOATIME(inode) __IS_FLG(inode, SB_RDONLY|SB_NOATIME) +#define IS_MGTIME(inode) __IS_FLG(inode, SB_MGTIME) #define IS_I_VERSION(inode) __IS_FLG(inode, SB_I_VERSION) #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) @@ -2366,7 +2368,7 @@ struct file_system_type { */ static inline bool is_mgtime(const struct inode *inode) { - return inode->i_sb->s_type->fs_flags & FS_MGTIME; + return inode->i_sb->s_flags & SB_MGTIME; } extern struct dentry *mount_bdev(struct file_system_type *fs_type,
On Wed, 2023-09-20 at 10:41 +0200, Christian Brauner wrote: > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > > > Many things in user-space depend on timestamps, such as build system > > > centered around 'make', but also 'find ... -newer ...'. > > > > > > > > > What does breakage with make look like in this situation? The "fuzz" > > here is going to be on the order of a jiffy. The typical case for make > > timestamp comparisons is comparing source files vs. a build target. If > > those are being written nearly simultaneously, then that could be an > > issue, but is that a typical behavior? It seems like it would be hard to > > rely on that anyway, esp. given filesystems like NFS that can do lazy > > writeback. > > > > One of the operating principles with this series is that timestamps can > > be of varying granularity between different files. Note that Linux > > already violates this assumption when you're working across filesystems > > of different types. > > > > As to potential fixes if this is a real problem: > > > > I don't really want to put this behind a mount or mkfs option (a'la > > relatime, etc.), but that is one possibility. > > > > I wonder if it would be feasible to just advance the coarse-grained > > current_time whenever we end up updating a ctime with a fine-grained > > timestamp? It might produce some inode write amplification. Files that > > Less than ideal imho. > > If this risks breaking existing workloads by enabling it unconditionally > and there isn't a clear way to detect and handle these situations > without risk of regression then we should move this behind a mount > option. > > So how about the following: > > From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 > From: Christian Brauner <brauner@kernel.org> > Date: Wed, 20 Sep 2023 10:00:08 +0200 > Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option > > While we initially thought we can do this unconditionally it turns out > that this might break existing workloads that rely on timestamps in very > specific ways and we always knew this was a possibility. Move > multi-grain timestamps behind a vfs mount option. I agree with this solution. You can add some metainfo: Reported-by: Ken Moffat <ken@linuxfromscratch.org> Bisected-by: Xi Ruoyao <xry111@linuxfromscratch.org> Link: https://lists.linuxfromscratch.org/sympa/arc/lfs-dev/2023-09/msg00036.html > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > fs/fs_context.c | 18 ++++++++++++++++++ > fs/inode.c | 4 ++-- > fs/proc_namespace.c | 1 + > fs/stat.c | 2 +- > include/linux/fs.h | 4 +++- > 5 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index a0ad7a0c4680..dd4dade0bb9e 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -44,6 +44,7 @@ static const struct constant_table common_set_sb_flag[] = { > { "mand", SB_MANDLOCK }, > { "ro", SB_RDONLY }, > { "sync", SB_SYNCHRONOUS }, > + { "mgtime", SB_MGTIME }, > { }, > }; > > @@ -52,18 +53,32 @@ static const struct constant_table common_clear_sb_flag[] = { > { "nolazytime", SB_LAZYTIME }, > { "nomand", SB_MANDLOCK }, > { "rw", SB_RDONLY }, > + { "nomgtime", SB_MGTIME }, > { }, > }; > > +static inline int check_mgtime(unsigned int token, const struct fs_context *fc) > +{ > + if (token != SB_MGTIME) > + return 0; > + if (!(fc->fs_type->fs_flags & FS_MGTIME)) > + return invalf(fc, "Filesystem doesn't support multi-grain timestamps"); > + return 0; > +} > + > /* > * Check for a common mount option that manipulates s_flags. > */ > static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > { > unsigned int token; > + int ret; > > token = lookup_constant(common_set_sb_flag, key, 0); > if (token) { > + ret = check_mgtime(token, fc); > + if (ret) > + return ret; > fc->sb_flags |= token; > fc->sb_flags_mask |= token; > return 0; > @@ -71,6 +86,9 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > > token = lookup_constant(common_clear_sb_flag, key, 0); > if (token) { > + ret = check_mgtime(token, fc); > + if (ret) > + return ret; > fc->sb_flags &= ~token; > fc->sb_flags_mask |= token; > return 0; > diff --git a/fs/inode.c b/fs/inode.c > index 54237f4242ff..fd1a2390aaa3 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2141,7 +2141,7 @@ EXPORT_SYMBOL(current_mgtime); > > static struct timespec64 current_ctime(struct inode *inode) > { > - if (is_mgtime(inode)) > + if (IS_MGTIME(inode)) > return current_mgtime(inode); > return current_time(inode); > } > @@ -2588,7 +2588,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode) > now = current_time(inode); > > /* Just copy it into place if it's not multigrain */ > - if (!is_mgtime(inode)) { > + if (!IS_MGTIME(inode)) { > inode_set_ctime_to_ts(inode, now); > return now; > } > diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c > index 250eb5bf7b52..08f5bf4d2c6c 100644 > --- a/fs/proc_namespace.c > +++ b/fs/proc_namespace.c > @@ -49,6 +49,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb) > { SB_DIRSYNC, ",dirsync" }, > { SB_MANDLOCK, ",mand" }, > { SB_LAZYTIME, ",lazytime" }, > + { SB_MGTIME, ",mgtime" }, > { 0, NULL } > }; > const struct proc_fs_opts *fs_infop; > diff --git a/fs/stat.c b/fs/stat.c > index 6e60389d6a15..2f18dd5de18b 100644 > --- a/fs/stat.c > +++ b/fs/stat.c > @@ -90,7 +90,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask, > stat->size = i_size_read(inode); > stat->atime = inode->i_atime; > > - if (is_mgtime(inode)) { > + if (IS_MGTIME(inode)) { > fill_mg_cmtime(stat, request_mask, inode); > } else { > stat->mtime = inode->i_mtime; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 4aeb3fa11927..03e415fb3a7c 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1114,6 +1114,7 @@ extern int send_sigurg(struct fown_struct *fown); > #define SB_NODEV BIT(2) /* Disallow access to device special files */ > #define SB_NOEXEC BIT(3) /* Disallow program execution */ > #define SB_SYNCHRONOUS BIT(4) /* Writes are synced at once */ > +#define SB_MGTIME BIT(5) /* Use multi-grain timestamps */ > #define SB_MANDLOCK BIT(6) /* Allow mandatory locks on an FS */ > #define SB_DIRSYNC BIT(7) /* Directory modifications are synchronous */ > #define SB_NOATIME BIT(10) /* Do not update access times. */ > @@ -2105,6 +2106,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags > ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) > #define IS_MANDLOCK(inode) __IS_FLG(inode, SB_MANDLOCK) > #define IS_NOATIME(inode) __IS_FLG(inode, SB_RDONLY|SB_NOATIME) > +#define IS_MGTIME(inode) __IS_FLG(inode, SB_MGTIME) > #define IS_I_VERSION(inode) __IS_FLG(inode, SB_I_VERSION) > > #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) > @@ -2366,7 +2368,7 @@ struct file_system_type { > */ > static inline bool is_mgtime(const struct inode *inode) > { > - return inode->i_sb->s_type->fs_flags & FS_MGTIME; > + return inode->i_sb->s_flags & SB_MGTIME; > } > > extern struct dentry *mount_bdev(struct file_system_type *fs_type,
On Wed, 2023-09-20 at 10:41 +0200, Christian Brauner wrote: > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > > > Many things in user-space depend on timestamps, such as build system > > > centered around 'make', but also 'find ... -newer ...'. > > > > > > > > > What does breakage with make look like in this situation? The "fuzz" > > here is going to be on the order of a jiffy. The typical case for make > > timestamp comparisons is comparing source files vs. a build target. If > > those are being written nearly simultaneously, then that could be an > > issue, but is that a typical behavior? It seems like it would be hard to > > rely on that anyway, esp. given filesystems like NFS that can do lazy > > writeback. > > > > One of the operating principles with this series is that timestamps can > > be of varying granularity between different files. Note that Linux > > already violates this assumption when you're working across filesystems > > of different types. > > > > As to potential fixes if this is a real problem: > > > > I don't really want to put this behind a mount or mkfs option (a'la > > relatime, etc.), but that is one possibility. > > > > I wonder if it would be feasible to just advance the coarse-grained > > current_time whenever we end up updating a ctime with a fine-grained > > timestamp? It might produce some inode write amplification. Files that > > Less than ideal imho. > > If this risks breaking existing workloads by enabling it unconditionally > and there isn't a clear way to detect and handle these situations > without risk of regression then we should move this behind a mount > option. > > So how about the following: > > From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 > From: Christian Brauner <brauner@kernel.org> > Date: Wed, 20 Sep 2023 10:00:08 +0200 > Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option > > While we initially thought we can do this unconditionally it turns out > that this might break existing workloads that rely on timestamps in very > specific ways and we always knew this was a possibility. Move > multi-grain timestamps behind a vfs mount option. > > Signed-off-by: Christian Brauner <brauner@kernel.org> > --- > fs/fs_context.c | 18 ++++++++++++++++++ > fs/inode.c | 4 ++-- > fs/proc_namespace.c | 1 + > fs/stat.c | 2 +- > include/linux/fs.h | 4 +++- > 5 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index a0ad7a0c4680..dd4dade0bb9e 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -44,6 +44,7 @@ static const struct constant_table common_set_sb_flag[] = { > { "mand", SB_MANDLOCK }, > { "ro", SB_RDONLY }, > { "sync", SB_SYNCHRONOUS }, > + { "mgtime", SB_MGTIME }, > { }, > }; > > > @@ -52,18 +53,32 @@ static const struct constant_table common_clear_sb_flag[] = { > { "nolazytime", SB_LAZYTIME }, > { "nomand", SB_MANDLOCK }, > { "rw", SB_RDONLY }, > + { "nomgtime", SB_MGTIME }, > { }, > }; > > > +static inline int check_mgtime(unsigned int token, const struct fs_context *fc) > +{ > + if (token != SB_MGTIME) > + return 0; > + if (!(fc->fs_type->fs_flags & FS_MGTIME)) > + return invalf(fc, "Filesystem doesn't support multi-grain timestamps"); > + return 0; > +} > + > /* > * Check for a common mount option that manipulates s_flags. > */ > static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > { > unsigned int token; > + int ret; > > > token = lookup_constant(common_set_sb_flag, key, 0); > if (token) { > + ret = check_mgtime(token, fc); > + if (ret) > + return ret; > fc->sb_flags |= token; > fc->sb_flags_mask |= token; > return 0; > @@ -71,6 +86,9 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > > > token = lookup_constant(common_clear_sb_flag, key, 0); > if (token) { > + ret = check_mgtime(token, fc); > + if (ret) > + return ret; > fc->sb_flags &= ~token; > fc->sb_flags_mask |= token; > return 0; > diff --git a/fs/inode.c b/fs/inode.c > index 54237f4242ff..fd1a2390aaa3 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2141,7 +2141,7 @@ EXPORT_SYMBOL(current_mgtime); > > > static struct timespec64 current_ctime(struct inode *inode) > { > - if (is_mgtime(inode)) > + if (IS_MGTIME(inode)) > return current_mgtime(inode); > return current_time(inode); > } > @@ -2588,7 +2588,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode) > now = current_time(inode); > > > /* Just copy it into place if it's not multigrain */ > - if (!is_mgtime(inode)) { > + if (!IS_MGTIME(inode)) { > inode_set_ctime_to_ts(inode, now); > return now; > } > diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c > index 250eb5bf7b52..08f5bf4d2c6c 100644 > --- a/fs/proc_namespace.c > +++ b/fs/proc_namespace.c > @@ -49,6 +49,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb) > { SB_DIRSYNC, ",dirsync" }, > { SB_MANDLOCK, ",mand" }, > { SB_LAZYTIME, ",lazytime" }, > + { SB_MGTIME, ",mgtime" }, > { 0, NULL } > }; > const struct proc_fs_opts *fs_infop; > diff --git a/fs/stat.c b/fs/stat.c > index 6e60389d6a15..2f18dd5de18b 100644 > --- a/fs/stat.c > +++ b/fs/stat.c > @@ -90,7 +90,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask, > stat->size = i_size_read(inode); > stat->atime = inode->i_atime; > > > - if (is_mgtime(inode)) { > + if (IS_MGTIME(inode)) { > fill_mg_cmtime(stat, request_mask, inode); > } else { > stat->mtime = inode->i_mtime; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 4aeb3fa11927..03e415fb3a7c 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1114,6 +1114,7 @@ extern int send_sigurg(struct fown_struct *fown); > #define SB_NODEV BIT(2) /* Disallow access to device special files */ > #define SB_NOEXEC BIT(3) /* Disallow program execution */ > #define SB_SYNCHRONOUS BIT(4) /* Writes are synced at once */ > +#define SB_MGTIME BIT(5) /* Use multi-grain timestamps */ > #define SB_MANDLOCK BIT(6) /* Allow mandatory locks on an FS */ > #define SB_DIRSYNC BIT(7) /* Directory modifications are synchronous */ > #define SB_NOATIME BIT(10) /* Do not update access times. */ > @@ -2105,6 +2106,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags > ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) > #define IS_MANDLOCK(inode) __IS_FLG(inode, SB_MANDLOCK) > #define IS_NOATIME(inode) __IS_FLG(inode, SB_RDONLY|SB_NOATIME) > +#define IS_MGTIME(inode) __IS_FLG(inode, SB_MGTIME) > #define IS_I_VERSION(inode) __IS_FLG(inode, SB_I_VERSION) > > > #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) > @@ -2366,7 +2368,7 @@ struct file_system_type { > */ > static inline bool is_mgtime(const struct inode *inode) > { > - return inode->i_sb->s_type->fs_flags & FS_MGTIME; > + return inode->i_sb->s_flags & SB_MGTIME; > } > > > extern struct dentry *mount_bdev(struct file_system_type *fs_type, The mount option looks reasonable. Thanks for throwing together the patch. Maybe in the future we can come up with a way to mitigate the problems and do this unconditionally? Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Tue 19-09-23 12:31:08, Jeff Layton wrote: > On Tue, 2023-09-19 at 16:52 +0200, Bruno Haible wrote: > > Jeff Layton wrote: > > > I'm not sure what we can do for this test. The nap() function is making > > > an assumption that the timestamp granularity will be constant, and that > > > isn't necessarily the case now. > > > > This is only of secondary importance, because the scenario by Jan Kara > > shows a much more fundamental breakage: > > > > > > The ultimate problem is that a sequence like: > > > > > > > > write(f1) > > > > stat(f2) > > > > write(f2) > > > > stat(f2) > > > > write(f1) > > > > stat(f1) > > > > > > > > can result in f1 timestamp to be (slightly) lower than the final f2 > > > > timestamp because the second write to f1 didn't bother updating the > > > > timestamp. That can indeed be a bit confusing to programs if they compare > > > > timestamps between two files. Jeff? > > > > > > > > > > Basically yes. > > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > Many things in user-space depend on timestamps, such as build system > > centered around 'make', but also 'find ... -newer ...'. > > > > > What does breakage with make look like in this situation? The "fuzz" > here is going to be on the order of a jiffy. The typical case for make > timestamp comparisons is comparing source files vs. a build target. If > those are being written nearly simultaneously, then that could be an > issue, but is that a typical behavior? It seems like it would be hard to > rely on that anyway, esp. given filesystems like NFS that can do lazy > writeback. TL;DR I don't think we can just wave away the change as "the problem has always been there". Firstly, the fact that something is not quite reliable on NFS doesn't mean people don't rely on the behavior on local filesystems. NFS has a historical reputation of being a bit weird ;). Secondly, I agree that the same problems can manifest currently for files on two filesystems with different timestamp granularity. But again that is something that is rare - widely used filesystems have a granularity of a jiffy and in most cases build and source files are on the same filesystem anyway. So yes, in principle the problems could happen even before multigrain timestamps but having different granularity per inode just made them manifest in much much more setups and that matters because setups that were perfectly fine before are not anymore. > One of the operating principles with this series is that timestamps can > be of varying granularity between different files. Note that Linux > already violates this assumption when you're working across filesystems > of different types. > > As to potential fixes if this is a real problem: Regarding whether the problem is real: I wouldn't worry too much about the particular test that started this thread. That seems like something very special. But the build system issues could be real - as you wrote in your motivation for the series - a lot can happen in a jiffy on contemporary computers. I can imagine build product having newer timestamp than build source because the modification of source managed to squeeze into the same jiffy and still use a coarse-grained timestamp. Or some other producer-consumer type of setup... Sure usually there would be enough stat(2) calls on both sides to force finegrained timestamps on both files but if there are not in some corner case, debugging the problem is really tough. > I don't really want to put this behind a mount or mkfs option (a'la > relatime, etc.), but that is one possibility. > > I wonder if it would be feasible to just advance the coarse-grained > current_time whenever we end up updating a ctime with a fine-grained > timestamp? It might produce some inode write amplification. Files that > were written within the same jiffy could see more inode transactions > logged, but that still might not be _too_ awful. From a first glance I'd guess the performance overhead will be too big for a busy filesystem to enable this unconditionally. But I could be wrong... Honza
On Wed 20-09-23 10:41:30, Christian Brauner wrote: > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > > > Many things in user-space depend on timestamps, such as build system > > > centered around 'make', but also 'find ... -newer ...'. > > > > > > > > > What does breakage with make look like in this situation? The "fuzz" > > here is going to be on the order of a jiffy. The typical case for make > > timestamp comparisons is comparing source files vs. a build target. If > > those are being written nearly simultaneously, then that could be an > > issue, but is that a typical behavior? It seems like it would be hard to > > rely on that anyway, esp. given filesystems like NFS that can do lazy > > writeback. > > > > One of the operating principles with this series is that timestamps can > > be of varying granularity between different files. Note that Linux > > already violates this assumption when you're working across filesystems > > of different types. > > > > As to potential fixes if this is a real problem: > > > > I don't really want to put this behind a mount or mkfs option (a'la > > relatime, etc.), but that is one possibility. > > > > I wonder if it would be feasible to just advance the coarse-grained > > current_time whenever we end up updating a ctime with a fine-grained > > timestamp? It might produce some inode write amplification. Files that > > Less than ideal imho. > > If this risks breaking existing workloads by enabling it unconditionally > and there isn't a clear way to detect and handle these situations > without risk of regression then we should move this behind a mount > option. > > So how about the following: > > From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 > From: Christian Brauner <brauner@kernel.org> > Date: Wed, 20 Sep 2023 10:00:08 +0200 > Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option > > While we initially thought we can do this unconditionally it turns out > that this might break existing workloads that rely on timestamps in very > specific ways and we always knew this was a possibility. Move > multi-grain timestamps behind a vfs mount option. > > Signed-off-by: Christian Brauner <brauner@kernel.org> Surely this is a safe choice as it moves the responsibility to the sysadmin and the cases where finegrained timestamps are required. But I kind of wonder how is the sysadmin going to decide whether mgtime is safe for his system or not? Because the possible breakage needn't be obvious at the first sight... If I were a sysadmin, I'd rather opt for something like finegrained timestamps + lazytime (if I needed the finegrained timestamps functionality). That should avoid the IO overhead of finegrained timestamps as well and I'd know I can have problems with timestamps only after a system crash. I've just got another idea how we could solve the problem: Couldn't we always just report coarsegrained timestamp to userspace and provide access to finegrained value only to NFS which should know what it's doing? Honza > --- > fs/fs_context.c | 18 ++++++++++++++++++ > fs/inode.c | 4 ++-- > fs/proc_namespace.c | 1 + > fs/stat.c | 2 +- > include/linux/fs.h | 4 +++- > 5 files changed, 25 insertions(+), 4 deletions(-) > > diff --git a/fs/fs_context.c b/fs/fs_context.c > index a0ad7a0c4680..dd4dade0bb9e 100644 > --- a/fs/fs_context.c > +++ b/fs/fs_context.c > @@ -44,6 +44,7 @@ static const struct constant_table common_set_sb_flag[] = { > { "mand", SB_MANDLOCK }, > { "ro", SB_RDONLY }, > { "sync", SB_SYNCHRONOUS }, > + { "mgtime", SB_MGTIME }, > { }, > }; > > @@ -52,18 +53,32 @@ static const struct constant_table common_clear_sb_flag[] = { > { "nolazytime", SB_LAZYTIME }, > { "nomand", SB_MANDLOCK }, > { "rw", SB_RDONLY }, > + { "nomgtime", SB_MGTIME }, > { }, > }; > > +static inline int check_mgtime(unsigned int token, const struct fs_context *fc) > +{ > + if (token != SB_MGTIME) > + return 0; > + if (!(fc->fs_type->fs_flags & FS_MGTIME)) > + return invalf(fc, "Filesystem doesn't support multi-grain timestamps"); > + return 0; > +} > + > /* > * Check for a common mount option that manipulates s_flags. > */ > static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > { > unsigned int token; > + int ret; > > token = lookup_constant(common_set_sb_flag, key, 0); > if (token) { > + ret = check_mgtime(token, fc); > + if (ret) > + return ret; > fc->sb_flags |= token; > fc->sb_flags_mask |= token; > return 0; > @@ -71,6 +86,9 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > > token = lookup_constant(common_clear_sb_flag, key, 0); > if (token) { > + ret = check_mgtime(token, fc); > + if (ret) > + return ret; > fc->sb_flags &= ~token; > fc->sb_flags_mask |= token; > return 0; > diff --git a/fs/inode.c b/fs/inode.c > index 54237f4242ff..fd1a2390aaa3 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -2141,7 +2141,7 @@ EXPORT_SYMBOL(current_mgtime); > > static struct timespec64 current_ctime(struct inode *inode) > { > - if (is_mgtime(inode)) > + if (IS_MGTIME(inode)) > return current_mgtime(inode); > return current_time(inode); > } > @@ -2588,7 +2588,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode) > now = current_time(inode); > > /* Just copy it into place if it's not multigrain */ > - if (!is_mgtime(inode)) { > + if (!IS_MGTIME(inode)) { > inode_set_ctime_to_ts(inode, now); > return now; > } > diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c > index 250eb5bf7b52..08f5bf4d2c6c 100644 > --- a/fs/proc_namespace.c > +++ b/fs/proc_namespace.c > @@ -49,6 +49,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb) > { SB_DIRSYNC, ",dirsync" }, > { SB_MANDLOCK, ",mand" }, > { SB_LAZYTIME, ",lazytime" }, > + { SB_MGTIME, ",mgtime" }, > { 0, NULL } > }; > const struct proc_fs_opts *fs_infop; > diff --git a/fs/stat.c b/fs/stat.c > index 6e60389d6a15..2f18dd5de18b 100644 > --- a/fs/stat.c > +++ b/fs/stat.c > @@ -90,7 +90,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask, > stat->size = i_size_read(inode); > stat->atime = inode->i_atime; > > - if (is_mgtime(inode)) { > + if (IS_MGTIME(inode)) { > fill_mg_cmtime(stat, request_mask, inode); > } else { > stat->mtime = inode->i_mtime; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 4aeb3fa11927..03e415fb3a7c 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1114,6 +1114,7 @@ extern int send_sigurg(struct fown_struct *fown); > #define SB_NODEV BIT(2) /* Disallow access to device special files */ > #define SB_NOEXEC BIT(3) /* Disallow program execution */ > #define SB_SYNCHRONOUS BIT(4) /* Writes are synced at once */ > +#define SB_MGTIME BIT(5) /* Use multi-grain timestamps */ > #define SB_MANDLOCK BIT(6) /* Allow mandatory locks on an FS */ > #define SB_DIRSYNC BIT(7) /* Directory modifications are synchronous */ > #define SB_NOATIME BIT(10) /* Do not update access times. */ > @@ -2105,6 +2106,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags > ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) > #define IS_MANDLOCK(inode) __IS_FLG(inode, SB_MANDLOCK) > #define IS_NOATIME(inode) __IS_FLG(inode, SB_RDONLY|SB_NOATIME) > +#define IS_MGTIME(inode) __IS_FLG(inode, SB_MGTIME) > #define IS_I_VERSION(inode) __IS_FLG(inode, SB_I_VERSION) > > #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) > @@ -2366,7 +2368,7 @@ struct file_system_type { > */ > static inline bool is_mgtime(const struct inode *inode) > { > - return inode->i_sb->s_type->fs_flags & FS_MGTIME; > + return inode->i_sb->s_flags & SB_MGTIME; > } > > extern struct dentry *mount_bdev(struct file_system_type *fs_type, > -- > 2.34.1 >
On Wed, Sep 20, 2023 at 12:17:31PM +0200, Jan Kara wrote: > On Wed 20-09-23 10:41:30, Christian Brauner wrote: > > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > > > > > Many things in user-space depend on timestamps, such as build system > > > > centered around 'make', but also 'find ... -newer ...'. > > > > > > > > > > > > > What does breakage with make look like in this situation? The "fuzz" > > > here is going to be on the order of a jiffy. The typical case for make > > > timestamp comparisons is comparing source files vs. a build target. If > > > those are being written nearly simultaneously, then that could be an > > > issue, but is that a typical behavior? It seems like it would be hard to > > > rely on that anyway, esp. given filesystems like NFS that can do lazy > > > writeback. > > > > > > One of the operating principles with this series is that timestamps can > > > be of varying granularity between different files. Note that Linux > > > already violates this assumption when you're working across filesystems > > > of different types. > > > > > > As to potential fixes if this is a real problem: > > > > > > I don't really want to put this behind a mount or mkfs option (a'la > > > relatime, etc.), but that is one possibility. > > > > > > I wonder if it would be feasible to just advance the coarse-grained > > > current_time whenever we end up updating a ctime with a fine-grained > > > timestamp? It might produce some inode write amplification. Files that > > > > Less than ideal imho. > > > > If this risks breaking existing workloads by enabling it unconditionally > > and there isn't a clear way to detect and handle these situations > > without risk of regression then we should move this behind a mount > > option. > > > > So how about the following: > > > > From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 > > From: Christian Brauner <brauner@kernel.org> > > Date: Wed, 20 Sep 2023 10:00:08 +0200 > > Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option > > > > While we initially thought we can do this unconditionally it turns out > > that this might break existing workloads that rely on timestamps in very > > specific ways and we always knew this was a possibility. Move > > multi-grain timestamps behind a vfs mount option. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > Surely this is a safe choice as it moves the responsibility to the sysadmin > and the cases where finegrained timestamps are required. But I kind of > wonder how is the sysadmin going to decide whether mgtime is safe for his > system or not? Because the possible breakage needn't be obvious at the > first sight... If I were a sysadmin, I'd rather opt for something like I think you'll basically enable this because you want to export a filesystem via NFS. > finegrained timestamps + lazytime (if I needed the finegrained timestamps > functionality). That should avoid the IO overhead of finegrained timestamps That would work with this patch, no? Or are you saying it would need something else? > as well and I'd know I can have problems with timestamps only after a > system crash. > > I've just got another idea how we could solve the problem: Couldn't we > always just report coarsegrained timestamp to userspace and provide access > to finegrained value only to NFS which should know what it's doing? What would changes would be involved for that? If this is invasive work and we decide this is something that we want to do then we should remove FS_MGTIME from btrfs, xfs, ext4, and tmpfs for v6.6.
On Wed, 2023-09-20 at 12:17 +0200, Jan Kara wrote: > On Wed 20-09-23 10:41:30, Christian Brauner wrote: > > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > > > > > Many things in user-space depend on timestamps, such as build system > > > > centered around 'make', but also 'find ... -newer ...'. > > > > > > > > > > > > > What does breakage with make look like in this situation? The "fuzz" > > > here is going to be on the order of a jiffy. The typical case for make > > > timestamp comparisons is comparing source files vs. a build target. If > > > those are being written nearly simultaneously, then that could be an > > > issue, but is that a typical behavior? It seems like it would be hard to > > > rely on that anyway, esp. given filesystems like NFS that can do lazy > > > writeback. > > > > > > One of the operating principles with this series is that timestamps can > > > be of varying granularity between different files. Note that Linux > > > already violates this assumption when you're working across filesystems > > > of different types. > > > > > > As to potential fixes if this is a real problem: > > > > > > I don't really want to put this behind a mount or mkfs option (a'la > > > relatime, etc.), but that is one possibility. > > > > > > I wonder if it would be feasible to just advance the coarse-grained > > > current_time whenever we end up updating a ctime with a fine-grained > > > timestamp? It might produce some inode write amplification. Files that > > > > Less than ideal imho. > > > > If this risks breaking existing workloads by enabling it unconditionally > > and there isn't a clear way to detect and handle these situations > > without risk of regression then we should move this behind a mount > > option. > > > > So how about the following: > > > > From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 > > From: Christian Brauner <brauner@kernel.org> > > Date: Wed, 20 Sep 2023 10:00:08 +0200 > > Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option > > > > While we initially thought we can do this unconditionally it turns out > > that this might break existing workloads that rely on timestamps in very > > specific ways and we always knew this was a possibility. Move > > multi-grain timestamps behind a vfs mount option. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > Surely this is a safe choice as it moves the responsibility to the sysadmin > and the cases where finegrained timestamps are required. But I kind of > wonder how is the sysadmin going to decide whether mgtime is safe for his > system or not? Because the possible breakage needn't be obvious at the > first sight... > That's the main reason I really didn't want to go with a mount option. Documenting that may be difficult. While there is some pessimism around it, I may still take a stab at just advancing the coarse clock whenever we fetch a fine-grained timestamp. It'd be nice to remove this option in the future if that turns out to be feasible. > If I were a sysadmin, I'd rather opt for something like > finegrained timestamps + lazytime (if I needed the finegrained timestamps > functionality). That should avoid the IO overhead of finegrained timestamps > as well and I'd know I can have problems with timestamps only after a > system crash. > I've just got another idea how we could solve the problem: Couldn't we > always just report coarsegrained timestamp to userspace and provide access > to finegrained value only to NFS which should know what it's doing? > I think that'd be hard. First of all, where would we store the second timestamp? We can't just truncate the fine-grained ones to come up with a coarse-grained one. It might also be confusing having nfsd and local filesystems present different attributes. > > --- > > fs/fs_context.c | 18 ++++++++++++++++++ > > fs/inode.c | 4 ++-- > > fs/proc_namespace.c | 1 + > > fs/stat.c | 2 +- > > include/linux/fs.h | 4 +++- > > 5 files changed, 25 insertions(+), 4 deletions(-) > > > > diff --git a/fs/fs_context.c b/fs/fs_context.c > > index a0ad7a0c4680..dd4dade0bb9e 100644 > > --- a/fs/fs_context.c > > +++ b/fs/fs_context.c > > @@ -44,6 +44,7 @@ static const struct constant_table common_set_sb_flag[] = { > > { "mand", SB_MANDLOCK }, > > { "ro", SB_RDONLY }, > > { "sync", SB_SYNCHRONOUS }, > > + { "mgtime", SB_MGTIME }, > > { }, > > }; > > > > @@ -52,18 +53,32 @@ static const struct constant_table common_clear_sb_flag[] = { > > { "nolazytime", SB_LAZYTIME }, > > { "nomand", SB_MANDLOCK }, > > { "rw", SB_RDONLY }, > > + { "nomgtime", SB_MGTIME }, > > { }, > > }; > > > > +static inline int check_mgtime(unsigned int token, const struct fs_context *fc) > > +{ > > + if (token != SB_MGTIME) > > + return 0; > > + if (!(fc->fs_type->fs_flags & FS_MGTIME)) > > + return invalf(fc, "Filesystem doesn't support multi-grain timestamps"); > > + return 0; > > +} > > + > > /* > > * Check for a common mount option that manipulates s_flags. > > */ > > static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > > { > > unsigned int token; > > + int ret; > > > > token = lookup_constant(common_set_sb_flag, key, 0); > > if (token) { > > + ret = check_mgtime(token, fc); > > + if (ret) > > + return ret; > > fc->sb_flags |= token; > > fc->sb_flags_mask |= token; > > return 0; > > @@ -71,6 +86,9 @@ static int vfs_parse_sb_flag(struct fs_context *fc, const char *key) > > > > token = lookup_constant(common_clear_sb_flag, key, 0); > > if (token) { > > + ret = check_mgtime(token, fc); > > + if (ret) > > + return ret; > > fc->sb_flags &= ~token; > > fc->sb_flags_mask |= token; > > return 0; > > diff --git a/fs/inode.c b/fs/inode.c > > index 54237f4242ff..fd1a2390aaa3 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -2141,7 +2141,7 @@ EXPORT_SYMBOL(current_mgtime); > > > > static struct timespec64 current_ctime(struct inode *inode) > > { > > - if (is_mgtime(inode)) > > + if (IS_MGTIME(inode)) > > return current_mgtime(inode); > > return current_time(inode); > > } > > @@ -2588,7 +2588,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode) > > now = current_time(inode); > > > > /* Just copy it into place if it's not multigrain */ > > - if (!is_mgtime(inode)) { > > + if (!IS_MGTIME(inode)) { > > inode_set_ctime_to_ts(inode, now); > > return now; > > } > > diff --git a/fs/proc_namespace.c b/fs/proc_namespace.c > > index 250eb5bf7b52..08f5bf4d2c6c 100644 > > --- a/fs/proc_namespace.c > > +++ b/fs/proc_namespace.c > > @@ -49,6 +49,7 @@ static int show_sb_opts(struct seq_file *m, struct super_block *sb) > > { SB_DIRSYNC, ",dirsync" }, > > { SB_MANDLOCK, ",mand" }, > > { SB_LAZYTIME, ",lazytime" }, > > + { SB_MGTIME, ",mgtime" }, > > { 0, NULL } > > }; > > const struct proc_fs_opts *fs_infop; > > diff --git a/fs/stat.c b/fs/stat.c > > index 6e60389d6a15..2f18dd5de18b 100644 > > --- a/fs/stat.c > > +++ b/fs/stat.c > > @@ -90,7 +90,7 @@ void generic_fillattr(struct mnt_idmap *idmap, u32 request_mask, > > stat->size = i_size_read(inode); > > stat->atime = inode->i_atime; > > > > - if (is_mgtime(inode)) { > > + if (IS_MGTIME(inode)) { > > fill_mg_cmtime(stat, request_mask, inode); > > } else { > > stat->mtime = inode->i_mtime; > > diff --git a/include/linux/fs.h b/include/linux/fs.h > > index 4aeb3fa11927..03e415fb3a7c 100644 > > --- a/include/linux/fs.h > > +++ b/include/linux/fs.h > > @@ -1114,6 +1114,7 @@ extern int send_sigurg(struct fown_struct *fown); > > #define SB_NODEV BIT(2) /* Disallow access to device special files */ > > #define SB_NOEXEC BIT(3) /* Disallow program execution */ > > #define SB_SYNCHRONOUS BIT(4) /* Writes are synced at once */ > > +#define SB_MGTIME BIT(5) /* Use multi-grain timestamps */ > > #define SB_MANDLOCK BIT(6) /* Allow mandatory locks on an FS */ > > #define SB_DIRSYNC BIT(7) /* Directory modifications are synchronous */ > > #define SB_NOATIME BIT(10) /* Do not update access times. */ > > @@ -2105,6 +2106,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags > > ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) > > #define IS_MANDLOCK(inode) __IS_FLG(inode, SB_MANDLOCK) > > #define IS_NOATIME(inode) __IS_FLG(inode, SB_RDONLY|SB_NOATIME) > > +#define IS_MGTIME(inode) __IS_FLG(inode, SB_MGTIME) > > #define IS_I_VERSION(inode) __IS_FLG(inode, SB_I_VERSION) > > > > #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) > > @@ -2366,7 +2368,7 @@ struct file_system_type { > > */ > > static inline bool is_mgtime(const struct inode *inode) > > { > > - return inode->i_sb->s_type->fs_flags & FS_MGTIME; > > + return inode->i_sb->s_flags & SB_MGTIME; > > } > > > > extern struct dentry *mount_bdev(struct file_system_type *fs_type, > > -- > > 2.34.1 > >
> > > While we initially thought we can do this unconditionally it turns out > > > that this might break existing workloads that rely on timestamps in very > > > specific ways and we always knew this was a possibility. Move > > > multi-grain timestamps behind a vfs mount option. > > > > Surely this is a safe choice as it moves the responsibility to the sysadmin > > and the cases where finegrained timestamps are required. But I kind of > > wonder how is the sysadmin going to decide whether mgtime is safe for his > > system or not? Because the possible breakage needn't be obvious at the > > first sight... > > > > That's the main reason I really didn't want to go with a mount option. > Documenting that may be difficult. While there is some pessimism around > it, I may still take a stab at just advancing the coarse clock whenever > we fetch a fine-grained timestamp. It'd be nice to remove this option in > the future if that turns out to be feasible. > > > If I were a sysadmin, I'd rather opt for something like > > finegrained timestamps + lazytime (if I needed the finegrained timestamps > > functionality). That should avoid the IO overhead of finegrained timestamps > > as well and I'd know I can have problems with timestamps only after a > > system crash. > > > I've just got another idea how we could solve the problem: Couldn't we > > always just report coarsegrained timestamp to userspace and provide access > > to finegrained value only to NFS which should know what it's doing? > > > > I think that'd be hard. First of all, where would we store the second > timestamp? We can't just truncate the fine-grained ones to come up with > a coarse-grained one. It might also be confusing having nfsd and local > filesystems present different attributes. As far as I can tell we have two options. The first one is to make this into a mount option which I really think isn't a big deal and lets us avoid this whole problem while allowing filesytems exposed via NFS to make use of this feature for change tracking. The second option is that we turn off fine-grained finestamps for v6.6 and you get to explore other options. It isn't a big deal regressions like this were always to be expected but v6.6 needs to stabilize so anything that requires more significant work is not an option.
On Wed, 2023-09-20 at 13:48 +0200, Christian Brauner wrote: > > > > While we initially thought we can do this unconditionally it turns out > > > > that this might break existing workloads that rely on timestamps in very > > > > specific ways and we always knew this was a possibility. Move > > > > multi-grain timestamps behind a vfs mount option. > > > > > > Surely this is a safe choice as it moves the responsibility to the sysadmin > > > and the cases where finegrained timestamps are required. But I kind of > > > wonder how is the sysadmin going to decide whether mgtime is safe for his > > > system or not? Because the possible breakage needn't be obvious at the > > > first sight... > > > > > > > That's the main reason I really didn't want to go with a mount option. > > Documenting that may be difficult. While there is some pessimism around > > it, I may still take a stab at just advancing the coarse clock whenever > > we fetch a fine-grained timestamp. It'd be nice to remove this option in > > the future if that turns out to be feasible. > > > > > If I were a sysadmin, I'd rather opt for something like > > > finegrained timestamps + lazytime (if I needed the finegrained timestamps > > > functionality). That should avoid the IO overhead of finegrained timestamps > > > as well and I'd know I can have problems with timestamps only after a > > > system crash. > > > > > I've just got another idea how we could solve the problem: Couldn't we > > > always just report coarsegrained timestamp to userspace and provide access > > > to finegrained value only to NFS which should know what it's doing? > > > > > > > I think that'd be hard. First of all, where would we store the second > > timestamp? We can't just truncate the fine-grained ones to come up with > > a coarse-grained one. It might also be confusing having nfsd and local > > filesystems present different attributes. > > As far as I can tell we have two options. The first one is to make this > into a mount option which I really think isn't a big deal and lets us > avoid this whole problem while allowing filesytems exposed via NFS to > make use of this feature for change tracking. > > The second option is that we turn off fine-grained finestamps for v6.6 > and you get to explore other options. > > It isn't a big deal regressions like this were always to be expected but > v6.6 needs to stabilize so anything that requires more significant work > is not an option. Oh, absolutely. I wasn't proposing to do that work for v6.6. For that, we absolutely either need the mount option or to just revert the mgtime conversions. My plan was to take a stab at doing this for a later kernel release. This is very much a "back to the drawing board" idea. It may not pan out after all, but if it does then we could consider removing the mount option at that point.
> I wasn't proposing to do that work for v6.6. For that, we absolutely > either need the mount option or to just revert the mgtime conversions. This sounds like you want me to do a full-on revert of your series but why? The conversion and changes support an actual use-case and are fine. It's a matter of whether we unconditionally expose it to users or not. @Jan, what do you think? > My plan was to take a stab at doing this for a later kernel release. Ok.
On Wed, 2023-09-20 at 14:08 +0200, Christian Brauner wrote: > > I wasn't proposing to do that work for v6.6. For that, we absolutely > > either need the mount option or to just revert the mgtime conversions. > > This sounds like you want me to do a full-on revert of your series but > why? The conversion and changes support an actual use-case and are fine. > It's a matter of whether we unconditionally expose it to users or not. > I don't, actually. I'm just mentioning that it's possible if we find the mount option to be unpalatable. > @Jan, what do you think? > > > My plan was to take a stab at doing this for a later kernel release. > > Ok. If it works out, then we may be able to eventually remove the mount option, but that is a separate project altogether.
> I don't, actually. I'm just mentioning that it's possible if we find the > mount option to be unpalatable. Ok. > > > @Jan, what do you think? > > > > > My plan was to take a stab at doing this for a later kernel release. > > > > Ok. > > If it works out, then we may be able to eventually remove the mount > option, but that is a separate project altogether. It would just become a nop for anyone setting it which is fine by me.
On Wed 20-09-23 06:35:18, Jeff Layton wrote: > On Wed, 2023-09-20 at 12:17 +0200, Jan Kara wrote: > > If I were a sysadmin, I'd rather opt for something like > > finegrained timestamps + lazytime (if I needed the finegrained timestamps > > functionality). That should avoid the IO overhead of finegrained timestamps > > as well and I'd know I can have problems with timestamps only after a > > system crash. > > > I've just got another idea how we could solve the problem: Couldn't we > > always just report coarsegrained timestamp to userspace and provide access > > to finegrained value only to NFS which should know what it's doing? > > > > I think that'd be hard. First of all, where would we store the second > timestamp? We can't just truncate the fine-grained ones to come up with > a coarse-grained one. It might also be confusing having nfsd and local > filesystems present different attributes. So what I had in mind (and I definitely miss all the NFS intricacies so the idea may be bogus) was that inode->i_ctime would be maintained exactly as is now. There will be new (kernel internal at least for now) STATX flag STATX_MULTIGRAIN_TS. fill_mg_cmtime() will return timestamp truncated to sb->s_time_gran unless STATX_MULTIGRAIN_TS is set. Hence unless you set STATX_MULTIGRAIN_TS, there is no difference in the returned timestamps compared to the state before multigrain timestamps were introduced. With STATX_MULTIGRAIN_TS we return full precision timestamp as stored in the inode. Then NFS in fh_fill_pre_attrs() and fh_fill_post_attrs() needs to make sure STATX_MULTIGRAIN_TS is set when calling vfs_getattr() to get multigrain time. I agree nfsd may now be presenting slightly different timestamps than user is able to see with stat(2) directly on the filesystem. But is that a problem? Essentially it is a similar solution as the mgtime mount option but now sysadmin doesn't have to decide on filesystem mount how to report timestamps but the stat caller knowingly opts into possibly inconsistent (among files) but high precision timestamps. And in the particular NFS usecase where stat is called all the time anyway, timestamps will likely even be consistent among files. Honza
On Wed 20-09-23 12:30:52, Christian Brauner wrote: > On Wed, Sep 20, 2023 at 12:17:31PM +0200, Jan Kara wrote: > > On Wed 20-09-23 10:41:30, Christian Brauner wrote: > > > > > f1 was last written to *after* f2 was last written to. If the timestamp of f1 > > > > > is then lower than the timestamp of f2, timestamps are fundamentally broken. > > > > > > > > > > Many things in user-space depend on timestamps, such as build system > > > > > centered around 'make', but also 'find ... -newer ...'. > > > > > > > > > > > > > > > > > What does breakage with make look like in this situation? The "fuzz" > > > > here is going to be on the order of a jiffy. The typical case for make > > > > timestamp comparisons is comparing source files vs. a build target. If > > > > those are being written nearly simultaneously, then that could be an > > > > issue, but is that a typical behavior? It seems like it would be hard to > > > > rely on that anyway, esp. given filesystems like NFS that can do lazy > > > > writeback. > > > > > > > > One of the operating principles with this series is that timestamps can > > > > be of varying granularity between different files. Note that Linux > > > > already violates this assumption when you're working across filesystems > > > > of different types. > > > > > > > > As to potential fixes if this is a real problem: > > > > > > > > I don't really want to put this behind a mount or mkfs option (a'la > > > > relatime, etc.), but that is one possibility. > > > > > > > > I wonder if it would be feasible to just advance the coarse-grained > > > > current_time whenever we end up updating a ctime with a fine-grained > > > > timestamp? It might produce some inode write amplification. Files that > > > > > > Less than ideal imho. > > > > > > If this risks breaking existing workloads by enabling it unconditionally > > > and there isn't a clear way to detect and handle these situations > > > without risk of regression then we should move this behind a mount > > > option. > > > > > > So how about the following: > > > > > > From cb14add421967f6e374eb77c36cc4a0526b10d17 Mon Sep 17 00:00:00 2001 > > > From: Christian Brauner <brauner@kernel.org> > > > Date: Wed, 20 Sep 2023 10:00:08 +0200 > > > Subject: [PATCH] vfs: move multi-grain timestamps behind a mount option > > > > > > While we initially thought we can do this unconditionally it turns out > > > that this might break existing workloads that rely on timestamps in very > > > specific ways and we always knew this was a possibility. Move > > > multi-grain timestamps behind a vfs mount option. > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > > Surely this is a safe choice as it moves the responsibility to the sysadmin > > and the cases where finegrained timestamps are required. But I kind of > > wonder how is the sysadmin going to decide whether mgtime is safe for his > > system or not? Because the possible breakage needn't be obvious at the > > first sight... If I were a sysadmin, I'd rather opt for something like > > I think you'll basically enable this because you want to export a > filesystem via NFS. OK, that's what I thought but then you have to make a tough choice between: 1) Possibly inconsistent NFS caches on frequent changes. 2) Possibly broken builds on NFS. Pick your poison ;) > > finegrained timestamps + lazytime (if I needed the finegrained timestamps > > functionality). That should avoid the IO overhead of finegrained timestamps > > That would work with this patch, no? Or are you saying it would need > something else? Sorry, I was not really precise here. What I meant was that instead of having multigrain timestamps, I (as a sysadmin) would want the filesystem to set sb->s_time_gran to 1 ns and use lazytime to remove the IO overhead of the frequent timestamp updates. But that is just me brainstorming possible solutions of the original NFS problem. > > as well and I'd know I can have problems with timestamps only after a > > system crash. > > > > I've just got another idea how we could solve the problem: Couldn't we > > always just report coarsegrained timestamp to userspace and provide access > > to finegrained value only to NFS which should know what it's doing? > > What would changes would be involved for that? See my other email. It should be fairly small... > If this is invasive work and we decide this is something that we want to > do then we should remove FS_MGTIME from btrfs, xfs, ext4, and tmpfs for > v6.6. .. but let's see what Jeff thinks. I can miss some problem with the solution. Honza
> On Sep 20, 2023, at 7:48 AM, Christian Brauner <brauner@kernel.org> wrote: > >>>> While we initially thought we can do this unconditionally it turns out >>>> that this might break existing workloads that rely on timestamps in very >>>> specific ways and we always knew this was a possibility. Move >>>> multi-grain timestamps behind a vfs mount option. >>> >>> Surely this is a safe choice as it moves the responsibility to the sysadmin >>> and the cases where finegrained timestamps are required. But I kind of >>> wonder how is the sysadmin going to decide whether mgtime is safe for his >>> system or not? Because the possible breakage needn't be obvious at the >>> first sight... >>> >> >> That's the main reason I really didn't want to go with a mount option. >> Documenting that may be difficult. While there is some pessimism around >> it, I may still take a stab at just advancing the coarse clock whenever >> we fetch a fine-grained timestamp. It'd be nice to remove this option in >> the future if that turns out to be feasible. >> >>> If I were a sysadmin, I'd rather opt for something like >>> finegrained timestamps + lazytime (if I needed the finegrained timestamps >>> functionality). That should avoid the IO overhead of finegrained timestamps >>> as well and I'd know I can have problems with timestamps only after a >>> system crash. >> >>> I've just got another idea how we could solve the problem: Couldn't we >>> always just report coarsegrained timestamp to userspace and provide access >>> to finegrained value only to NFS which should know what it's doing? >>> >> >> I think that'd be hard. First of all, where would we store the second >> timestamp? We can't just truncate the fine-grained ones to come up with >> a coarse-grained one. It might also be confusing having nfsd and local >> filesystems present different attributes. > > As far as I can tell we have two options. The first one is to make this > into a mount option which I really think isn't a big deal and lets us > avoid this whole problem while allowing filesytems exposed via NFS to > make use of this feature for change tracking. A mount option isn't hard to implement, but I think it would be a mistake. As Jan pointed out, the two alternative compromises are often very difficult to choose between. Tossing this decision to administrators doesn't seem like a responsible way to handle a question that might result in, at the least, unexpected behavior, and at worst, data corruption. Plus, on Linux, often times files are accessed locally on NFS servers as well as remotely -- how does the server's administrator pick the correct setting in that case? > The second option is that we turn off fine-grained finestamps for v6.6 > and you get to explore other options. You could put it behind an EXPERIMENTAL Kconfig option so that the code stays in and can be used by the brave or foolish while it is still being refined. > It isn't a big deal regressions like this were always to be expected but > v6.6 needs to stabilize so anything that requires more significant work > is not an option. -- Chuck Lever
On Wed, 2023-09-20 at 14:48 +0200, Jan Kara wrote: > On Wed 20-09-23 06:35:18, Jeff Layton wrote: > > On Wed, 2023-09-20 at 12:17 +0200, Jan Kara wrote: > > > If I were a sysadmin, I'd rather opt for something like > > > finegrained timestamps + lazytime (if I needed the finegrained timestamps > > > functionality). That should avoid the IO overhead of finegrained timestamps > > > as well and I'd know I can have problems with timestamps only after a > > > system crash. > > > > > I've just got another idea how we could solve the problem: Couldn't we > > > always just report coarsegrained timestamp to userspace and provide access > > > to finegrained value only to NFS which should know what it's doing? > > > > > > > I think that'd be hard. First of all, where would we store the second > > timestamp? We can't just truncate the fine-grained ones to come up with > > a coarse-grained one. It might also be confusing having nfsd and local > > filesystems present different attributes. > > So what I had in mind (and I definitely miss all the NFS intricacies so the > idea may be bogus) was that inode->i_ctime would be maintained exactly as > is now. There will be new (kernel internal at least for now) STATX flag > STATX_MULTIGRAIN_TS. fill_mg_cmtime() will return timestamp truncated to > sb->s_time_gran unless STATX_MULTIGRAIN_TS is set. Hence unless you set > STATX_MULTIGRAIN_TS, there is no difference in the returned timestamps > compared to the state before multigrain timestamps were introduced. With > STATX_MULTIGRAIN_TS we return full precision timestamp as stored in the > inode. Then NFS in fh_fill_pre_attrs() and fh_fill_post_attrs() needs to > make sure STATX_MULTIGRAIN_TS is set when calling vfs_getattr() to get > multigrain time. > I agree nfsd may now be presenting slightly different timestamps than user > is able to see with stat(2) directly on the filesystem. But is that a > problem? Essentially it is a similar solution as the mgtime mount option > but now sysadmin doesn't have to decide on filesystem mount how to report > timestamps but the stat caller knowingly opts into possibly inconsistent > (among files) but high precision timestamps. And in the particular NFS > usecase where stat is called all the time anyway, timestamps will likely > even be consistent among files. > I like this idea... Would we also need to raise sb->s_time_gran to something corresponding to HZ on these filesystems? If we truncate the timestamps at a granularity corresponding to HZ before presenting them via statx and the like then that should work around the problem with programs that compare timestamps between inodes. With NFSv4, when a filesystem doesn't report a STATX_CHANGE_COOKIE, nfsd will fake one up using the ctime. It's fine for that to use a full fine- grained timestamp since we don't expect to be able to compare that value with one of a different inode. I think we'd want nfsd to present the mtime/ctime values as truncated, just like we would with a local fs. We could hit the same problem of an earlier-looking timestamp with NFS if we try to present the actual fine- grained values to the clients. IOW, I'm convinced that we need to avoid this behavior in most situations. If we do this, then we technically don't need the mount option either. We could still add it though, and have it govern whether fill_mg_cmtime truncates the timestamps before storing them in the kstat.
> You could put it behind an EXPERIMENTAL Kconfig option so that the > code stays in and can be used by the brave or foolish while it is > still being refined. Given that the discussion has now fully gone back to the drawing board and this is a regression the honest thing to do is to revert the five patches that introduce the infrastructure: ffb6cf19e063 ("fs: add infrastructure for multigrain timestamps") d48c33972916 ("tmpfs: add support for multigrain timestamps") e44df2664746 ("xfs: switch to multigrain timestamps") 0269b585868e ("ext4: switch to multigrain timestamps") 50e9ceef1d4f ("btrfs: convert to multigrain timestamps") The conversion to helpers and cleanups are sane and should stay and can be used for any solution that gets built on top of it. I'd appreciate a look at the branch here: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.ctime.revert survives xfstests.
On Wed, 2023-09-20 at 16:53 +0200, Christian Brauner wrote: > > You could put it behind an EXPERIMENTAL Kconfig option so that the > > code stays in and can be used by the brave or foolish while it is > > still being refined. > > Given that the discussion has now fully gone back to the drawing board > and this is a regression the honest thing to do is to revert the five > patches that introduce the infrastructure: > > ffb6cf19e063 ("fs: add infrastructure for multigrain timestamps") > d48c33972916 ("tmpfs: add support for multigrain timestamps") > e44df2664746 ("xfs: switch to multigrain timestamps") > 0269b585868e ("ext4: switch to multigrain timestamps") > 50e9ceef1d4f ("btrfs: convert to multigrain timestamps") > > The conversion to helpers and cleanups are sane and should stay and can > be used for any solution that gets built on top of it. > > I'd appreciate a look at the branch here: > git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.ctime.revert > > survives xfstests. I think that's probably the wisest course of action. I need some time to ponder the options for this series anyway, and another cycle in next wouldn't hurt. The branch itself looks fine, but you might want to reverse the order of the patches in case someone lands there in the middle of a bisect. IOW, I think you want to revert the "convert to multigrain" patches before you revert the infrastructure.
On Wed 20-09-23 16:53:26, Christian Brauner wrote: > > You could put it behind an EXPERIMENTAL Kconfig option so that the > > code stays in and can be used by the brave or foolish while it is > > still being refined. > > Given that the discussion has now fully gone back to the drawing board > and this is a regression the honest thing to do is to revert the five > patches that introduce the infrastructure: > > ffb6cf19e063 ("fs: add infrastructure for multigrain timestamps") > d48c33972916 ("tmpfs: add support for multigrain timestamps") > e44df2664746 ("xfs: switch to multigrain timestamps") > 0269b585868e ("ext4: switch to multigrain timestamps") > 50e9ceef1d4f ("btrfs: convert to multigrain timestamps") > > The conversion to helpers and cleanups are sane and should stay and can > be used for any solution that gets built on top of it. > > I'd appreciate a look at the branch here: > git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.ctime.revert > > survives xfstests. Agreed. I think most of ffb6cf19e063 ("fs: add infrastructure for multigrain timestamps") will be needed anyway but there's no problem in reintroducing it in the new solution. I've checked the branch and the reverts look good to me. Feel free to add: Acked-by: Jan Kara <jack@suse.cz> Honza
On Wed 20-09-23 10:12:03, Jeff Layton wrote: > On Wed, 2023-09-20 at 14:48 +0200, Jan Kara wrote: > > On Wed 20-09-23 06:35:18, Jeff Layton wrote: > > > On Wed, 2023-09-20 at 12:17 +0200, Jan Kara wrote: > > > > If I were a sysadmin, I'd rather opt for something like > > > > finegrained timestamps + lazytime (if I needed the finegrained timestamps > > > > functionality). That should avoid the IO overhead of finegrained timestamps > > > > as well and I'd know I can have problems with timestamps only after a > > > > system crash. > > > > > > > I've just got another idea how we could solve the problem: Couldn't we > > > > always just report coarsegrained timestamp to userspace and provide access > > > > to finegrained value only to NFS which should know what it's doing? > > > > > > > > > > I think that'd be hard. First of all, where would we store the second > > > timestamp? We can't just truncate the fine-grained ones to come up with > > > a coarse-grained one. It might also be confusing having nfsd and local > > > filesystems present different attributes. > > > > So what I had in mind (and I definitely miss all the NFS intricacies so the > > idea may be bogus) was that inode->i_ctime would be maintained exactly as > > is now. There will be new (kernel internal at least for now) STATX flag > > STATX_MULTIGRAIN_TS. fill_mg_cmtime() will return timestamp truncated to > > sb->s_time_gran unless STATX_MULTIGRAIN_TS is set. Hence unless you set > > STATX_MULTIGRAIN_TS, there is no difference in the returned timestamps > > compared to the state before multigrain timestamps were introduced. With > > STATX_MULTIGRAIN_TS we return full precision timestamp as stored in the > > inode. Then NFS in fh_fill_pre_attrs() and fh_fill_post_attrs() needs to > > make sure STATX_MULTIGRAIN_TS is set when calling vfs_getattr() to get > > multigrain time. > > > I agree nfsd may now be presenting slightly different timestamps than user > > is able to see with stat(2) directly on the filesystem. But is that a > > problem? Essentially it is a similar solution as the mgtime mount option > > but now sysadmin doesn't have to decide on filesystem mount how to report > > timestamps but the stat caller knowingly opts into possibly inconsistent > > (among files) but high precision timestamps. And in the particular NFS > > usecase where stat is called all the time anyway, timestamps will likely > > even be consistent among files. > > > > I like this idea... > > Would we also need to raise sb->s_time_gran to something corresponding > to HZ on these filesystems? I was actually confused a bit about how timestamp_truncate() works. The jiffie granularity is just direct consequence of current_time() using ktime_get_coarse_real_ts64() and not of timestamp_truncate(). sb->s_time_gran seems to be more about the on-disk format so it doesn't seem like a great idea to touch it. So probably we can just truncate timestamps in generic_fillattr() to HZ granularity unconditionally. > If we truncate the timestamps at a granularity corresponding to HZ before > presenting them via statx and the like then that should work around the > problem with programs that compare timestamps between inodes. Exactly. > With NFSv4, when a filesystem doesn't report a STATX_CHANGE_COOKIE, nfsd > will fake one up using the ctime. It's fine for that to use a full fine- > grained timestamp since we don't expect to be able to compare that value > with one of a different inode. Yes. > I think we'd want nfsd to present the mtime/ctime values as truncated, > just like we would with a local fs. We could hit the same problem of an > earlier-looking timestamp with NFS if we try to present the actual fine- > grained values to the clients. IOW, I'm convinced that we need to avoid > this behavior in most situations. I wasn't sure if there's a way to do this within NFS - i.e., if the value communicated via NFSv3 protocol (I know v4 has a special change cookie field for it) that gets used for detecting need to revalidate file contents isn't the one presented to client's userspace as ctime. If there's a way to do this then great, I'm all for presenting truncated timestamps even for NFS. > If we do this, then we technically don't need the mount option either. Yes, that was my hope. > We could still add it though, and have it govern whether fill_mg_cmtime > truncates the timestamps before storing them in the kstat. Well, if we decide these timestamps are useful for userspace as well, I'd rather make that a userspace visible STATX flag than a mount option. So applications aware of the pitfalls can get high precision timestamps without possibly breaking unaware applications. Honza
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index b54c70e1a74e..cb1ff47af156 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -7279,7 +7279,7 @@ static struct file_system_type ext4_fs_type = { .init_fs_context = ext4_init_fs_context, .parameters = ext4_param_specs, .kill_sb = kill_block_super, - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP | FS_MGTIME, }; MODULE_ALIAS_FS("ext4");