Skip to content

mds/client: return -ENODATA when xattr doesn't exist for removexattr#55945

Merged
vshankar merged 4 commits intoceph:mainfrom
lxbsz:wip-64679-new
Jun 28, 2024
Merged

mds/client: return -ENODATA when xattr doesn't exist for removexattr#55945
vshankar merged 4 commits intoceph:mainfrom
lxbsz:wip-64679-new

Conversation

@lxbsz
Copy link
Member

@lxbsz lxbsz commented Mar 5, 2024 •
edited by dparmar18
Loading

The POSIX says we should return -ENODATA when the corresponding
attribute doesn't exist when removing it. While there is one
exception for the acl ones in the local filesystems, for exmaple
for xfs, which will treat it as success.

While in the MDS side there have two ways to remove the xattr:
sending a CEPH_MDS_OP_SETXATTR request by setting the 'flags' with
CEPH_XATTR_REMOVE and just issued a CEPH_MDS_OP_RMXATTR request
directly.

For the first one it will always return 0 when the corresponding
xattr doesn't exist, while for the later one it will return
-ENODATA instead, this should be fixed in MDS to make them to be
consistent.

Fixes: https://tracker.ceph.com/issues/64679
Signed-off-by: Xiubo Li xiubli@redhat.com

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. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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)
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
  • jenkins test rook e2e

@lxbsz lxbsz requested a review from a team March 5, 2024 07:19
@github-actions github-actions bot added build/ops cephfs Ceph File System tests labels Mar 5, 2024
@lxbsz lxbsz force-pushed the wip-64679-new branch 3 times, most recently from 39b15b8 to 417fae6 Compare March 5, 2024 11:18
dparmar18
dparmar18 reviewed Mar 5, 2024
View reviewed changes
dparmar18
dparmar18 reviewed Mar 5, 2024
View reviewed changes
dparmar18
dparmar18 reviewed Mar 6, 2024
View reviewed changes
@dparmar18
Copy link
Contributor

dparmar18 commented Mar 6, 2024

why did we separate out vxattr.cc bf55551?

@lxbsz
Copy link
Member Author

lxbsz commented Mar 7, 2024

why did we separate out vxattr.cc bf55551?

It's not a good idea to pile all the test in a single ceph_test_libcephfs binary, especially when we only want to run vxattr relevant tests like this. It will save a lot of time.

@dparmar18
Copy link
Contributor

dparmar18 commented Mar 7, 2024

why did we separate out vxattr.cc bf55551?

It's not a good idea to pile all the test in a single ceph_test_libcephfs binary, especially when we only want to run vxattr relevant tests like this. It will save a lot of time.

got it

dparmar18
dparmar18 reviewed Mar 7, 2024
View reviewed changes
vshankar
vshankar reviewed Mar 11, 2024
View reviewed changes
vshankar
vshankar reviewed Mar 13, 2024
View reviewed changes
@vshankar
Copy link
Contributor

vshankar commented Mar 21, 2024

jenkins test make check

@vshankar
Copy link
Contributor

vshankar commented Apr 4, 2024

https://pulpito.ceph.com/?branch=wip-vshankar-testing-20240330.170739

@vshankar
Copy link
Contributor

vshankar commented May 9, 2024

This PR is under test in https://tracker.ceph.com/issues/65874.

vshankar added a commit to vshankar/ceph that referenced this pull request May 9, 2024
@vshankar
Merge PR ceph#55945 into wip-vshankar-testing-20240509.053109-debug
1759761 
* refs/pull/55945/head:
	test/libcephfs: add ceph_test_libcephfs_vxattr
	test/libcephfs: add removexattr test support
	test/libcephfs: move FsCrypt xattr test to dedicated test file
	mds/client: return -ENODATA when xattr doesn't exist for removexattr
@vshankar
Copy link
Contributor

vshankar commented May 28, 2024

@lxbsz I believe pjd test failures are related to this change. See: https://pulpito.ceph.com/vshankar-2024-05-27_06:46:53-fs-wip-vshankar-testing-20240521.105740-debug-testing-default-smithi/7727715/

2024-05-27T10:25:47.216 INFO:tasks.workunit.client.0.smithi044.stderr:++ /home/ubuntu/cephtest/mnt.0/client.0/tmp/tmp/../pjd-fstest-20090130-RC/tests/xacl/../../fstest -u 65533 -g 65533 setfacl fstest_f6bd9e3b079accf3b499c26a4a391e28/fstest_2472357a9410ec5786c6f0423e521
e86 m u::rw-,g::r--,o::r--
2024-05-27T10:25:47.216 INFO:tasks.workunit.client.0.smithi044.stderr:++ tail -1
2024-05-27T10:25:47.223 INFO:tasks.workunit.client.0.smithi044.stderr:+ r=61
2024-05-27T10:25:47.223 INFO:tasks.workunit.client.0.smithi044.stderr:+ echo 61
2024-05-27T10:25:47.224 INFO:tasks.workunit.client.0.smithi044.stderr:+ egrep '^0$'
2024-05-27T10:25:47.227 INFO:tasks.workunit.client.0.smithi044.stderr:+ '[' 1 -eq 0 ']'
2024-05-27T10:25:47.227 INFO:tasks.workunit.client.0.smithi044.stderr:+ echo 'not ok 39'

Would you mind checking?

@lxbsz
Copy link
Member Author

lxbsz commented May 29, 2024

@lxbsz I believe pjd test failures are related to this change. See: https://pulpito.ceph.com/vshankar-2024-05-27_06:46:53-fs-wip-vshankar-testing-20240521.105740-debug-testing-default-smithi/7727715/

2024-05-27T10:25:47.216 INFO:tasks.workunit.client.0.smithi044.stderr:++ /home/ubuntu/cephtest/mnt.0/client.0/tmp/tmp/../pjd-fstest-20090130-RC/tests/xacl/../../fstest -u 65533 -g 65533 setfacl fstest_f6bd9e3b079accf3b499c26a4a391e28/fstest_2472357a9410ec5786c6f0423e521
e86 m u::rw-,g::r--,o::r--
2024-05-27T10:25:47.216 INFO:tasks.workunit.client.0.smithi044.stderr:++ tail -1
2024-05-27T10:25:47.223 INFO:tasks.workunit.client.0.smithi044.stderr:+ r=61
2024-05-27T10:25:47.223 INFO:tasks.workunit.client.0.smithi044.stderr:+ echo 61
2024-05-27T10:25:47.224 INFO:tasks.workunit.client.0.smithi044.stderr:+ egrep '^0$'
2024-05-27T10:25:47.227 INFO:tasks.workunit.client.0.smithi044.stderr:+ '[' 1 -eq 0 ']'
2024-05-27T10:25:47.227 INFO:tasks.workunit.client.0.smithi044.stderr:+ echo 'not ok 39'

Would you mind checking?

Okay, let me have a look this week or next.

@lxbsz
Copy link
Member Author

lxbsz commented May 30, 2024

@vshankar Fixed it.

@vshankar
Copy link
Contributor

vshankar commented Jun 3, 2024

@vshankar Fixed it.

Could you elaborate a bit on the fix please?

@lxbsz
Copy link
Member Author

lxbsz commented Jun 3, 2024

@vshankar Fixed it.

Could you elaborate a bit on the fix please?

Sure, just fixed it here https://github.com/ceph/ceph/pull/55945/files#diff-7a3052fe46aebfed0382c9d0bb9880ea1328add824e0b10c5d551ddfee282cd1R13979-R13983. The ACL_EA_ACCESS and ACL_EA_DEFAULT are special and should treat as success if not exist.

@vshankar
Copy link
Contributor

vshankar commented Jun 3, 2024

This PR is under test in https://tracker.ceph.com/issues/66327.

@vshankar
Copy link
Contributor

vshankar commented Jun 17, 2024

This PR is under test in https://tracker.ceph.com/issues/66522.

vshankar
vshankar approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://tracker.ceph.com/projects/cephfs/wiki/Main#wip-vshankar-testing-20240617094253-debug

(https://tracker.ceph.com/issues/66522)

@vshankar
Copy link
Contributor

vshankar commented Jun 25, 2024

jenkins retest this please

@vshankar
Copy link
Contributor

vshankar commented Jun 27, 2024

jenkins test api

@vshankar
Copy link
Contributor

vshankar commented Jun 28, 2024

@lxbsz could you check the api test failures? Those seem to be consistently failing for this change.

lxbsz added 4 commits June 28, 2024 13:01
@lxbsz
mds/client: return -ENODATA when xattr doesn't exist for removexattr
797e926 
The POSIX says we should return -ENODATA when the corresponding
attribute doesn't exist when removing it. While there is one
exception for the acl ones in the local filesystems, for exmaple
for xfs, which will treat it as success.

While in the MDS side there have two ways to remove the xattr:
sending a CEPH_MDS_OP_SETXATTR request by setting the 'flags' with
CEPH_XATTR_REMOVE and just issued a CEPH_MDS_OP_RMXATTR request
directly.

For the first one it will always return 0 when the corresponding
xattr doesn't exist, while for the later one it will return
-ENODATA instead, this should be fixed in MDS to make them to be
consistent.

Added a CEPH_XATTR_REMOVE2 new flags and will return -ENODATA errno
directly when the crresponding xattr doesn't exist. Just keeps the
old CEPH_XATTR_REMOVE flags to make it to be compatible with old
clients.

Fixes: https://tracker.ceph.com/issues/64679
Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
test/libcephfs: move FsCrypt xattr test to dedicated test file
ec30c46 
Switch to use XATTR_CREATE.

Fixes: https://tracker.ceph.com/issues/64679
Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
test/libcephfs: add removexattr test support
1a27670 
There have two ways to remove xattrs, removexattr() and setxattr()
with the XATTR_REPLACE flags set.

Fixes: https://tracker.ceph.com/issues/64679
Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
test/libcephfs: add ceph_test_libcephfs_vxattr
b2a07f8 
Fixes: https://tracker.ceph.com/issues/64679
Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
Copy link
Member Author

lxbsz commented Jun 28, 2024

@lxbsz could you check the api test failures? Those seem to be consistently failing for this change.

Rebased it.

@vshankar vshankar merged commit cf51417 into ceph:main Jun 28, 2024
@joscollin joscollin mentioned this pull request Jul 11, 2024
Merged
This was referenced Jul 24, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dparmar18 dparmar18 dparmar18 left review comments

@vshankar vshankar vshankar approved these changes

Assignees

No one assigned

Labels

build/ops cephfs Ceph File System tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lxbsz @dparmar18 @vshankar