client, mds: update mtime and change attr for snapdir when snaps are created, deleted and renamed

[vshankar]

Changes depend on https://github.com/ceph/ceph/pull/48086 - commits are included in this PR and would be removed when the dependent PR gets merged.

Contribution Guidelines

• To sign and title your commits, please refer to Submitting Patches to Ceph.

• If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

• Tracker (select at least one)
  • [ ] References tracker ticket
  • [ ] Very recent bug; references commit where it was introduced
  • [ ] New feature (ticket optional)
  • [ ] Doc update (no ticket needed)
  • [ ] Code cleanup (no ticket needed)
• Component impact
  • [ ] Affects Dashboard, opened tracker ticket
  • [ ] Affects Orchestrator, opened tracker ticket
  • [ ] No impact that needs to be tracked
• Documentation (select at least one)
  • [ ] Updates relevant documentation
  • [ ] No doc update is appropriate
• Tests (select at least one)
  • [ ] Includes unit test(s)
  • [ ] Includes integration test(s)
  • [ ] Includes bug reproducer
  • [ ] No tests

Show available Jenkins commands

• jenkins retest this please
• jenkins test classic perf
• jenkins test crimson perf
• jenkins test signed
• jenkins test make check
• jenkins test make check arm64
• jenkins test submodules
• jenkins test dashboard
• jenkins test dashboard cephadm
• jenkins test api
• jenkins test docs
• jenkins render docs
• jenkins test ceph-volume all
• jenkins test ceph-volume tox
• jenkins test windows

[gregsfortytwo]

Hm, I don't have any comments on the details, but at a bigger picture: why store the snap_mtime in the parent inode? Alternatives:

1. store the mtime in the SnapRealm instead. It's a piece of snapshot metadata, and we store most of the snapshot metadata there, so it seems more natural? (Also: avoids inflating inodes.)
2. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

[vshankar]

Hm, I don't have any comments on the details, but at a bigger picture: why store the snap_mtime in the parent inode? Alternatives:

1. store the mtime in the SnapRealm instead. It's a piece of snapshot metadata, and we store most of the snapshot metadata there, so it seems more natural? (Also: avoids inflating inodes.)

I didn't think about that frankly. I think doing it this way would be feasible.

2. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

Do you mean parent dirs mtime/change_attr? I.e., for /a/b/.snap/ use mtime/change_attr from directory b when a snap is created/deleted/renamed under .snap?

[gregsfortytwo]

2. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

Do you mean parent dirs mtime/change_attr? I.e., for /a/b/.snap/ use mtime/change_attr from directory b when a snap is created/deleted/renamed under .snap?

Yeah exactly. I'm not sure what the implications are, or if that's actually any simpler than what's in this PR, but if we could do it all on the client without any protocol changes at all that's often a win.

[vshankar]

2. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

Do you mean parent dirs mtime/change_attr? I.e., for /a/b/.snap/ use mtime/change_attr from directory b when a snap is created/deleted/renamed under .snap?

Yeah exactly. I'm not sure what the implications are, or if that's actually any simpler than what's in this PR, but if we could do it all on the client without any protocol changes at all that's often a win.

Creating a snapshot should really not change mtime of its non-snapdir parent, isn't it?

[gregsfortytwo]

3. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

Do you mean parent dirs mtime/change_attr? I.e., for /a/b/.snap/ use mtime/change_attr from directory b when a snap is created/deleted/renamed under .snap?

Yeah exactly. I'm not sure what the implications are, or if that's actually any simpler than what's in this PR, but if we could do it all on the client without any protocol changes at all that's often a win.

Creating a snapshot should really not change mtime of its non-snapdir parent, isn't it?

Hmm, I guess I'm not sure what the behavior is or should be. I assumed it changed the mtime since it's changing a piece of information attached to the inode. But I guess that would interact poorly with the mirror daemon or other backup software, and we may not update it since it's not an update which posix would talk about.

[vshankar]

3. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

Do you mean parent dirs mtime/change_attr? I.e., for /a/b/.snap/ use mtime/change_attr from directory b when a snap is created/deleted/renamed under .snap?

Yeah exactly. I'm not sure what the implications are, or if that's actually any simpler than what's in this PR, but if we could do it all on the client without any protocol changes at all that's often a win.

Creating a snapshot should really not change mtime of its non-snapdir parent, isn't it?

Hmm, I guess I'm not sure what the behavior is or should be. I assumed it changed the mtime since it's changing a piece of information attached to the inode. But I guess that would interact poorly with the mirror daemon or other backup software, and we may not update it since it's not an update which posix would talk about.

Right. Its only the immediate parent (snapdir) whose mtime/change_attr needs updation.

[gregsfortytwo]

On Mon, Oct 10, 2022 at 10:48 PM Venky Shankar @.***> wrote:

1. Do we actually need snapdir-specific metadata? What are the downsides of just using the CInode's mtime and change attr?

Do you mean parent dirs mtime/change_attr? I.e., for /a/b/.snap/ use mtime/change_attr from directory b when a snap is created/deleted/renamed under .snap?

Yeah exactly. I'm not sure what the implications are, or if that's actually any simpler than what's in this PR, but if we could do it all on the client without any protocol changes at all that's often a win.

Creating a snapshot should really not change mtime of its non-snapdir parent, isn't it?

Hmm, does it not change the mtime? It's certainly changing characteristics of the directory, so I'd think it would change that. But perhaps it doesn't do so now — changing mtimes on snapshot creates would interact a bit strangely with backup systems and the mirror daemon.

Message ID: @.***>

[github-actions[bot]]

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

[vshankar]

jenkins test make check

[mchangir]

I wonder if the missing *times/rstats for the .snap dir is the reason rctime is going haywire for .snap dir in general. This popped up in my mind since we'd planned to use rctime as the deciding factor to optimize file-system crawl for ceph-mirroring.

[vshankar]

I wonder if the missing *times/rstats for the .snap dir is the reason rctime is going haywire for .snap dir in general. This popped up in my mind since we'd planned to use rctime as the deciding factor to optimize file-system crawl for ceph-mirroring.

I have no idea. From whatever I recall, an mds failover caused rstats for inodes (not only for snapdir) mess up all over the place.

[vshankar]

Fixed and updated. @lxbsz please take a look.

[vshankar]

jenkins test make check

[vshankar]

@lxbsz @ajarr fixed and updated.

[vshankar]

jenkins test api

[vshankar]

https://tracker.ceph.com/projects/cephfs/wiki/Main#08-Dec-2022

[vshankar]

https://tracker.ceph.com/projects/cephfs/wiki/Main#08-Dec-2022

Sorry - wrong branch/link. This is still under test.

[vshankar]

Fixed and updated. I also switched to using std::nullopt for std::optional.

[github-actions[bot]]

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved