mbox series

[v2,00/11] fs: multigrain timestamp redux

Message ID 20240701-mgtime-v2-0-19d412a940d9@kernel.org (mailing list archive)
Headers show
Series fs: multigrain timestamp redux | expand

Message

Jeff Layton July 1, 2024, 10:26 a.m. UTC
This set is essentially unchanged from the last one, aside from the
new file in Documentation/. I had a review comment from Andi Kleen
suggesting that the ctime_floor should be per time_namespace, but I
think that's incorrect as the realtime clock is not namespaced.

At LSF/MM this year, we had a discussion about the inode change
attribute. At the time I mentioned that I thought I could salvage the
multigrain timestamp work that had to be reverted last year [1].  That
version had to be reverted because it was possible for a file to get a
coarse grained timestamp that appeared to be earlier than another file
that had recently gotten a fine-grained stamp.

This version corrects the problem by establishing a per-time_namespace
ctime_floor value that should prevent this from occurring. In the above
situation that was problematic before, the two files might end up with
the same timestamp value, but they won't appear to have been modified in
the wrong order.

That problem was discovered by the test-stat-time gnulib test. Note that
that test still fails on multigrain timestamps, but that's because its
method of determining the minimum delay that will show a timestamp
change will no longer work with multigrain timestamps. I have a patch to
change the testcase to use a different method that I've posted to the
bug-gnulib mailing list.

The big question with this set is whether the performance will be
suitable. The testing I've done seems to show performance parity with
multigrain timestamps enabled, but it's hard to rule this out regressing
some workload.

This set is based on top of Christian's vfs.misc branch (which has the
earlier change to track inode timestamps as discrete integers). If there
are no major objections, I'd like to let this soak in linux-next for a
bit to see if any problems shake out.

[1]: https://lore.kernel.org/linux-fsdevel/20230807-mgctime-v7-0-d1dec143a704@kernel.org/

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- Added Documentation file
- Link to v1: https://lore.kernel.org/r/20240626-mgtime-v1-0-a189352d0f8f@kernel.org

---
Jeff Layton (11):
      fs: turn inode ctime fields into a single ktime_t
      fs: uninline inode_get_ctime and inode_set_ctime_to_ts
      fs: tracepoints for inode_needs_update_time and inode_set_ctime_to_ts
      fs: add infrastructure for multigrain timestamps
      fs: add percpu counters to count fine vs. coarse timestamps
      fs: have setattr_copy handle multigrain timestamps appropriately
      xfs: switch to multigrain timestamps
      ext4: switch to multigrain timestamps
      btrfs: convert to multigrain timestamps
      tmpfs: add support for multigrain timestamps
      Documentation: add a new file documenting multigrain timestamps

 Documentation/filesystems/multigrain-ts.rst | 126 ++++++++++++++++
 fs/attr.c                                   |  52 ++++++-
 fs/btrfs/file.c                             |  25 +---
 fs/btrfs/super.c                            |   3 +-
 fs/ext4/super.c                             |   2 +-
 fs/inode.c                                  | 221 +++++++++++++++++++++++++---
 fs/stat.c                                   |  39 ++++-
 fs/xfs/libxfs/xfs_trans_inode.c             |   6 +-
 fs/xfs/xfs_iops.c                           |   6 +-
 fs/xfs/xfs_super.c                          |   2 +-
 include/linux/fs.h                          |  61 +++++---
 include/trace/events/timestamp.h            | 173 ++++++++++++++++++++++
 mm/shmem.c                                  |   2 +-
 13 files changed, 639 insertions(+), 79 deletions(-)
---
base-commit: 2e8c78ef85682671dae2ac3a5aa039b07be0fc0b
change-id: 20240626-mgtime-5cd80b18d810

Best regards,

Comments

Josef Bacik July 1, 2024, 1:53 p.m. UTC | #1
On Mon, Jul 01, 2024 at 06:26:36AM -0400, Jeff Layton wrote:
> This set is essentially unchanged from the last one, aside from the
> new file in Documentation/. I had a review comment from Andi Kleen
> suggesting that the ctime_floor should be per time_namespace, but I
> think that's incorrect as the realtime clock is not namespaced.
> 
> At LSF/MM this year, we had a discussion about the inode change
> attribute. At the time I mentioned that I thought I could salvage the
> multigrain timestamp work that had to be reverted last year [1].  That
> version had to be reverted because it was possible for a file to get a
> coarse grained timestamp that appeared to be earlier than another file
> that had recently gotten a fine-grained stamp.
> 
> This version corrects the problem by establishing a per-time_namespace
> ctime_floor value that should prevent this from occurring. In the above
> situation that was problematic before, the two files might end up with
> the same timestamp value, but they won't appear to have been modified in
> the wrong order.
> 
> That problem was discovered by the test-stat-time gnulib test. Note that
> that test still fails on multigrain timestamps, but that's because its
> method of determining the minimum delay that will show a timestamp
> change will no longer work with multigrain timestamps. I have a patch to
> change the testcase to use a different method that I've posted to the
> bug-gnulib mailing list.
> 
> The big question with this set is whether the performance will be
> suitable. The testing I've done seems to show performance parity with
> multigrain timestamps enabled, but it's hard to rule this out regressing
> some workload.
> 
> This set is based on top of Christian's vfs.misc branch (which has the
> earlier change to track inode timestamps as discrete integers). If there
> are no major objections, I'd like to let this soak in linux-next for a
> bit to see if any problems shake out.
> 
> [1]: https://lore.kernel.org/linux-fsdevel/20230807-mgctime-v7-0-d1dec143a704@kernel.org/
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

I have a few nits that need to be addressed, but you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the series once they're addressed.  Thanks,

Josef
Jeff Layton July 1, 2024, 2:12 p.m. UTC | #2
On Mon, 2024-07-01 at 09:53 -0400, Josef Bacik wrote:
> On Mon, Jul 01, 2024 at 06:26:36AM -0400, Jeff Layton wrote:
> > This set is essentially unchanged from the last one, aside from the
> > new file in Documentation/. I had a review comment from Andi Kleen
> > suggesting that the ctime_floor should be per time_namespace, but I
> > think that's incorrect as the realtime clock is not namespaced.
> > 
> > At LSF/MM this year, we had a discussion about the inode change
> > attribute. At the time I mentioned that I thought I could salvage the
> > multigrain timestamp work that had to be reverted last year [1].  That
> > version had to be reverted because it was possible for a file to get a
> > coarse grained timestamp that appeared to be earlier than another file
> > that had recently gotten a fine-grained stamp.
> > 
> > This version corrects the problem by establishing a per-time_namespace
> > ctime_floor value that should prevent this from occurring. In the above
> > situation that was problematic before, the two files might end up with
> > the same timestamp value, but they won't appear to have been modified in
> > the wrong order.
> > 
> > That problem was discovered by the test-stat-time gnulib test. Note that
> > that test still fails on multigrain timestamps, but that's because its
> > method of determining the minimum delay that will show a timestamp
> > change will no longer work with multigrain timestamps. I have a patch to
> > change the testcase to use a different method that I've posted to the
> > bug-gnulib mailing list.
> > 
> > The big question with this set is whether the performance will be
> > suitable. The testing I've done seems to show performance parity with
> > multigrain timestamps enabled, but it's hard to rule this out regressing
> > some workload.
> > 
> > This set is based on top of Christian's vfs.misc branch (which has the
> > earlier change to track inode timestamps as discrete integers). If there
> > are no major objections, I'd like to let this soak in linux-next for a
> > bit to see if any problems shake out.
> > 
> > [1]: https://lore.kernel.org/linux-fsdevel/20230807-mgctime-v7-0-d1dec143a704@kernel.org/
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> I have a few nits that need to be addressed, but you can add
> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> 
> to the series once they're addressed.  Thanks,
> 

Thanks! Fixed them up in my tree. I left the IS_I_VERSION check out as
well, and added a note to the changelog on the btrfs patch.