Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cephfs: expose snapshot creation time as new ceph.snap.btime vxattr #27077

Merged
merged 7 commits into from Apr 23, 2019

Conversation

@ddiss
Copy link
Contributor

commented Mar 20, 2019

Samba requires the snapshot creation time for the purpose of tracking and identifying Previous Versions for files and directories.

Fixes: https://tracker.ceph.com/issues/38838

Copy link
Member

left a comment

Implementation LGTM.

Reviewed-by: Greg Farnum gfarnum@redhat.com

src/test/libcephfs/test.cc Show resolved Hide resolved
@ddiss ddiss force-pushed the ddiss:snap_btime_xattr branch from 6a13412 to f733b6b Mar 21, 2019
@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

The updated test compares the ceph.snap.btime between an older and newer snapshot.

Copy link
Member

left a comment

Also, please make a tracker feature ticket and add the fixes line to your commits. Do you want this backported?

src/client/Client.cc Show resolved Hide resolved
src/mds/CInode.cc Outdated Show resolved Hide resolved
src/test/libcephfs/test.cc Show resolved Hide resolved
src/test/libcephfs/test.cc Show resolved Hide resolved
@ddiss ddiss force-pushed the ddiss:snap_btime_xattr branch from f733b6b to 988723c Mar 22, 2019
@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

New version adds Fixes: https://tracker.ceph.com/issues/38838 tags and reworks the infomap.size() == 1 logic.

@ukernel

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

LGTM

@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Thanks for the feedback so far - the corresponding kernel changes were proposed (and subsequently acked) via https://www.spinics.net/lists/ceph-devel/msg44894.html .

@batrick any further comment on the proposed $secs.$nsecs format?

@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

I've posted the Samba changes to https://lists.samba.org/archive/samba-technical/2019-March/133089.html - would be good to get final feedback on the time format soon if possible, as that appears to be the only thing holding up all three patch-sets.

batrick added a commit to batrick/ceph that referenced this pull request Mar 30, 2019
* refs/pull/27077/head:
	test: add libcephfs snap.btime xattr test
	client: add ceph.snap.btime vxattr
	mds: carry snapshot creation time with InodeStat

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
Reviewed-by: Greg Farnum <gfarnum@redhat.com>
batrick added a commit to batrick/ceph that referenced this pull request Apr 2, 2019
* refs/pull/27077/head:
	test: add libcephfs snap.btime xattr test
	client: add ceph.snap.btime vxattr
	mds: carry snapshot creation time with InodeStat

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
Reviewed-by: Greg Farnum <gfarnum@redhat.com>
src/test/libcephfs/test.cc Show resolved Hide resolved
@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

I think I've finally worked out what's going on here:

Client::_listxattr() returns a length based on the static _vxattrs_name_size() value. Client::_vxattrs_calcu_name_size() only takes into account whether the vxattr is hidden, not whether the vxattr.exists_cb() returns true/false.
When filling the xattr buffer, Client::_listxattr() checks vxattr.hidden and vxattr.exists_cb(). When the latter returns false (as is the case for ceph.snap.btime on non-snapshots), we return a length which is larger than the amount that we wrote to the buffer.

@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

It seems that "ceph.snap.btime" is the only vxattr to set hidden=false and fill the exists_cb(), so I don't think this client bug affects existing releases.
IMO there are a couple of options to fix this:

  1. drop _dir_vxattrs_name_size / _file_vxattrs_name_size and always calculate these lengths based on whether exists_cb() returns true in the Client::_listxattr() code-path.
  2. flag the "ceph.snap.btime" vxattr as hidden and add some sort of assert to ensure that hidden is always set to true alongside an exists_cb() callback.

I'll proceed with option (1), but any other feedback would still be appreciated.

@ddiss ddiss force-pushed the ddiss:snap_btime_xattr branch from 988723c to 6cde3ab Apr 15, 2019
@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Changes since previous version:

  • rebase against master
  • add four new commits (on top) which:
    • fix Client::_listxattr() vxattr length calculations
    • extend listxattr test coverage
Copy link
Member

left a comment

otherwise LGTM

src/client/Client.cc Outdated Show resolved Hide resolved
src/client/Client.cc Outdated Show resolved Hide resolved
src/client/Client.cc Outdated Show resolved Hide resolved
ddiss added 4 commits Mar 20, 2019
This allows for a future vxattr exposing the snapshot creation time to
users. The InodeStat encoding version is bumped to account for the new
snap_btime member.

Fixes: https://tracker.ceph.com/issues/38838
Signed-off-by: David Disseldorp <ddiss@suse.de>
The ceph.snap.btime vxattr carries the snapshot creation time for files
and directories residing within a snapshot.

Fixes: https://tracker.ceph.com/issues/38838
Signed-off-by: David Disseldorp <ddiss@suse.de>
Test that the new "ceph.snap.btime" xattr is present following
snapshot, and that the corresponding $secs.$nsecs value changes between
old and new snapshots.

Fixes: https://tracker.ceph.com/issues/38838
Signed-off-by: David Disseldorp <ddiss@suse.de>
Client::_listxattr() incorrectly returns a length based on the static
_vxattrs_name_size() value. _vxattrs_calcu_name_size() only takes into
account whether vxattrs are hidden, ignoring vxattr.exists_cb().

When filling the xattr buffer, Client::_listxattr() checks vxattr.hidden
and vxattr.exists_cb(). When the latter returns false (as is the case
for ceph.snap.btime on non-snapshots), we return a length which is
larger than the amount that we wrote to the buffer.

Fix this behaviour by always calculating the vxattrs length at runtime,
taking both vxattr.hidden and vxattr.exists_cb() into account.

Signed-off-by: David Disseldorp <ddiss@suse.de>
ddiss added 3 commits Apr 15, 2019
_listxattr() now calculates the length of vxattrs dynamically, so these
helpers, which incorrectly ignore vxattr.exists_cb(), can be removed.

Signed-off-by: David Disseldorp <ddiss@suse.de>
Additionally check that:
- a zero buflen returns the needed length
- a non-zero buflen less than needed fails
- the list-length is non-zero following setxattr

Signed-off-by: David Disseldorp <ddiss@suse.de>
Extend LibCephFS.SnapXattrs to also test ceph_listxattr() against
snapshot and non-snapshot files.

Signed-off-by: David Disseldorp <ddiss@suse.de>
@ddiss ddiss force-pushed the ddiss:snap_btime_xattr branch from 6cde3ab to 8fd9620 Apr 15, 2019
@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Changes since previous version:

  • address Patrick's "fix _listxattr() vxattr buffer length calculation" comments
  • rebase against master again
@ddiss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Client::_listxattr() returns a length based on the static _vxattrs_name_size() value. Client::_vxattrs_calcu_name_size() only takes into account whether the vxattr is hidden, not whether the vxattr.exists_cb() returns true/false.
When filling the xattr buffer, Client::_listxattr() checks vxattr.hidden and vxattr.exists_cb(). When the latter returns false (as is the case for ceph.snap.btime on non-snapshots), we return a length which is larger than the amount that we wrote to the buffer.

FWIW, it looks like the kernel ceph_listxattr() function has similar behaviour, so I'll respin my kernel patchset with a corresponding fix once this gets merged.

@batrick batrick merged commit 8fd9620 into ceph:master Apr 23, 2019
4 of 5 checks passed
4 of 5 checks passed
make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
batrick added a commit that referenced this pull request Apr 23, 2019
* refs/pull/27077/head:
	test: check listattr for snapshot btime entry
	test: extend LibCephFS.Xattrs test
	client: remove unused vxattr length helpers
	client: fix _listxattr() vxattr buffer length calculation
	test: add libcephfs snap.btime xattr test
	client: add ceph.snap.btime vxattr
	mds: carry snapshot creation time with InodeStat

Reviewed-by: Patrick Donnelly <pdonnell@redhat.com>
Reviewed-by: Greg Farnum <gfarnum@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.