src/mds: increment directory inode's change attr by one #48241

ajarr · 2022-09-26T00:59:05Z

... whenever the mtime or ctime of the directory inode is modified.

In CephFS subvolume clones exported using NFS-Ganesha, newly created
files using `touch` were not being listed. It was identified that the
create request sent to the Ceph MDS via NFS-Ganesha's libcephfs client
modified the mtime and ctime of the parent directory, but did not modify
the change_attr of the parent directory. Since the NFS client
didn't see a modification of the change attribute in the reply, it
didn't invalidate its readdir cache. The subsequent directory `ls` was
satisfied from the NFS client's stale readdir cache.

Whenever parent directory inode's  mtime was modified in
MDCache::predirty_journal_parents(), the parent inode's change_attr
was set to its dirstat->change_attr. The parent inode's
dirstat->change_attr doesn't track changes to parent's *ctime only*
changes such as setattr, setvxattr, etc. on the parent
directory. See commit 0d441dcd6af553d11d6be6df56d577c5659904a0 for more
details. This caused the directory inode's change_attr to not be updated
when an operation to change only its ctime was followed by an operation
to change its mtime and ctime.

Fix this by making changes to MDCache::predirty_journal_parents() and
CInode::finish_scatter_gather_update() to increment the directory
inode's change_attr by one instead of setting it to its
dirstat->change_attr.

Fixes: https://tracker.ceph.com/issues/57210
Signed-off-by: Ramana Raja <rraja@redhat.com>

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 · 2022-09-26T15:42:22Z

Nice find tracking down the cause of this bug!

The biggest concern here is just making sure we don't have any outstanding places where we set the change_attr to the value of another dirstat or something, because that could rewind us back to having this bug again. Sounds like that's been verified @ajarr? :)

ajarr · 2022-09-27T03:10:48Z

Nice find tracking down the cause of this bug!

The biggest concern here is just making sure we don't have any outstanding places where we set the change_attr to the value of another dirstat or something, because that could rewind us back to having this bug again. Sounds like that's been verified @ajarr? :)

I tracked down the two instances where directory inode's change_attr was set to its dirstat's change_attr, and made the required changes in this PR. Not much has changed around change_attr since the PR #10922 that introduced it.

ajarr · 2022-09-27T15:46:14Z

@jtlayton I looked at your PR that introduced change_attr #10922. I don't see much code changes around change_attr tracking since then in Ceph. In this PR, I make changes in the MDS code where directory inode's change_attr were set to its dirstat's change_attr (by 0d441dc), and instead increment the directory inode's change_attr by one as explained in the commit message.

ajarr · 2022-09-27T16:35:57Z

@vshankar, these are operations that happen on a FS subvolume clone export directory (/volumes///UUID/) when you issue fs subvolume snaphsot clone command.

mkdir of export dir
setxattr 'ceph.dir.layout.pool' on export dir
setattr mode=0755 on export dir
files copied over from snapshot created in export dir
setattr mtime, atime on export dir

jtlayton

This all looks good to me. Nice catch @ajarr !

jtlayton · 2022-09-27T17:01:31Z

src/mds/CInode.cc

@@ -2543,7 +2543,8 @@ void CInode::finish_scatter_gather_update(int type, MutationRef& mut)
      if (touched_mtime)
 	pi->mtime = pi->ctime = pi->dirstat.mtime;
      if (touched_chattr)
-	pi->change_attr = pi->dirstat.change_attr;
+	pi->change_attr++;


I don't know what I was thinking here. Your fix seems obviously correct.

vshankar

Great catch!

gouthampacha · 2022-09-29T17:29:52Z

This is great! Thank you @ajarr - looking forward to backports when you have them; and i'll be happy to test with OpenStack in the picture!

vshankar · 2022-10-10T06:09:07Z

@ajarr you mentioned about an alternate fix. Do you plan to work on it and discuss it here? (A separate PR for that would work too).

ajarr · 2022-10-10T16:57:24Z

@vshankar I plan to work on it in a separate PR. This PR can be merged if the tests passed.

batrick

Great work!

src/test/libcephfs/test.cc

ajarr · 2022-10-13T19:50:09Z

In the latest patchset, only made changes to the test to break it down into smaller ones.

vshankar · 2022-10-14T15:13:34Z

jenkins test api

src/test/libcephfs/test.cc

Alternate operations that only change directory's ctime (setattr/setxattr/removexattr on directory) with those that change directory's mtime and ctime (create/rename/remove a file within directory). Check that directory's change_attr is updated everytime ctime changes. Signed-off-by: Ramana Raja <rraja@redhat.com>

... whenever the mtime or ctime of the directory inode is modified. In CephFS subvolume clones exported using NFS-Ganesha, newly created files using `touch` were not being listed. It was identified that the create request sent to the Ceph MDS via NFS-Ganesha's libcephfs client modified the mtime and ctime of the parent directory, but did not modify the change_attr of the parent directory. Since the NFS client didn't see a modification of the change attribute in the reply, it didn't invalidate its readdir cache. The subsequent directory `ls` was satisfied from the NFS client's stale readdir cache. Whenever parent directory inode's mtime was modified in MDCache::predirty_journal_parents(), the parent inode's change_attr was set to its dirstat->change_attr. The parent inode's dirstat->change_attr doesn't track changes to parent's *ctime only* changes such as setattr, setvxattr, etc. on the parent directory. See commit 0d441dc for more details. This caused the directory inode's change_attr to not be updated when an operation to change only its ctime was followed by an operation to change its mtime and ctime. Fix this by making changes to MDCache::predirty_journal_parents() and CInode::finish_scatter_gather_update() to increment the directory inode's change_attr by one instead of setting it to its dirstat->change_attr. Fixes: https://tracker.ceph.com/issues/57210 Signed-off-by: Ramana Raja <rraja@redhat.com>

Signed-off-by: Ramana Raja <rraja@redhat.com>

ajarr · 2022-10-14T23:33:01Z

In the latest patchset made the following changes to the tests:

improved readability of the code comments per Patrick's suggestion
checked that change_attr increases everytime ctime is updated. So used ASSERT_GT instead of ASSERT_NE

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/Main#14-Oct-2022

vshankar · 2022-10-17T05:27:27Z

jenkins test api

vshankar · 2022-10-17T08:52:34Z

jenkins test api

github-actions bot added cephfs Ceph File System tests labels Sep 26, 2022

ajarr mentioned this pull request Sep 27, 2022

[FSAL Ceph] NFS client unable to see newly created files when listing directory contents in a FS subvolume clone nfs-ganesha/nfs-ganesha#852

Closed

ajarr requested a review from jtlayton September 27, 2022 14:16

jtlayton approved these changes Sep 27, 2022

View reviewed changes

vshankar requested a review from a team September 28, 2022 03:53

vshankar approved these changes Sep 28, 2022

View reviewed changes

vshankar added the wip-vshankar-testing label Sep 28, 2022

kotreshhr approved these changes Sep 28, 2022

View reviewed changes

batrick requested changes Oct 10, 2022

View reviewed changes

src/test/libcephfs/test.cc Outdated Show resolved Hide resolved

ajarr force-pushed the fix-57210 branch from 27ad63e to 5a08ab4 Compare October 13, 2022 18:22

ajarr requested a review from batrick October 13, 2022 18:23

batrick approved these changes Oct 14, 2022

View reviewed changes

batrick reviewed Oct 14, 2022

View reviewed changes

src/test/libcephfs/test.cc Outdated Show resolved Hide resolved

ajarr added 3 commits October 14, 2022 19:25

cephfs: add additional logging to track change_attr of inodes

f02b777

Signed-off-by: Ramana Raja <rraja@redhat.com>

ajarr force-pushed the fix-57210 branch from 5a08ab4 to f02b777 Compare October 14, 2022 23:26

vshankar approved these changes Oct 17, 2022

View reviewed changes

vshankar merged commit 9f56295 into ceph:main Oct 17, 2022

vshankar removed the wip-vshankar-testing label Oct 17, 2022

This was referenced Oct 18, 2022

quincy: src/mds: increment directory inode's change attr by one #48520

Merged

pacific: src/mds: increment directory inode's change attr by one #48521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src/mds: increment directory inode's change attr by one #48241

src/mds: increment directory inode's change attr by one #48241

ajarr commented Sep 26, 2022 •

edited

gregsfortytwo commented Sep 26, 2022

ajarr commented Sep 27, 2022 •

edited

ajarr commented Sep 27, 2022

ajarr commented Sep 27, 2022 •

edited

jtlayton left a comment

jtlayton Sep 27, 2022

vshankar left a comment

gouthampacha commented Sep 29, 2022

vshankar commented Oct 10, 2022

ajarr commented Oct 10, 2022

batrick left a comment

ajarr commented Oct 13, 2022

vshankar commented Oct 14, 2022

ajarr commented Oct 14, 2022

vshankar left a comment

vshankar commented Oct 17, 2022

vshankar commented Oct 17, 2022

src/mds: increment directory inode's change attr by one #48241

src/mds: increment directory inode's change attr by one #48241

Conversation

ajarr commented Sep 26, 2022 • edited

Checklist

gregsfortytwo commented Sep 26, 2022

ajarr commented Sep 27, 2022 • edited

ajarr commented Sep 27, 2022

ajarr commented Sep 27, 2022 • edited

jtlayton left a comment

Choose a reason for hiding this comment

jtlayton Sep 27, 2022

Choose a reason for hiding this comment

vshankar left a comment

Choose a reason for hiding this comment

gouthampacha commented Sep 29, 2022

vshankar commented Oct 10, 2022

ajarr commented Oct 10, 2022

batrick left a comment

Choose a reason for hiding this comment

ajarr commented Oct 13, 2022

vshankar commented Oct 14, 2022

ajarr commented Oct 14, 2022

vshankar left a comment

Choose a reason for hiding this comment

vshankar commented Oct 17, 2022

vshankar commented Oct 17, 2022

ajarr commented Sep 26, 2022 •

edited

ajarr commented Sep 27, 2022 •

edited

ajarr commented Sep 27, 2022 •

edited