Skip to content

NSFS | content dir, ignore failure to delete xattr from latest, if already deleted #8964

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

Merged
merged 1 commit into from
Apr 23, 2025

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented Apr 9, 2025

Describe the Problem

when adding to content dir at the same time after enabling versioning. both of them might try to remove the xattr from the directory. one of them will fail with ENODATA (The named attribute does not exist). ignore this error because attribute already deleted. see removexattr

Explain the Changes

  1. ignore xattr clear error for data already deleted

Issues: Fixed #8846

NOTE - issue doesn't replicate on my environment. ran jest tests on CI 15 times without test failing but since it is a concurrency test, there may be other problems that didn't happen by chance

Testing Instructions:

  1. run sudo npx jest test_versioning_concurrency -t "content dir multiple puts of the same key"
  • Doc added/updated
  • Tests added

@nadavMiz nadavMiz self-assigned this Apr 9, 2025
@nadavMiz nadavMiz force-pushed the xattr-concurrency branch from 8b2b8a6 to 83289f7 Compare April 9, 2025 15:14
@nadavMiz nadavMiz requested review from shirady and romayalon April 10, 2025 10:23
@shirady
Copy link
Contributor

shirady commented Apr 10, 2025

@nadavMiz.

  • If I understand right the underlying system call is fremovexattr man (you referred to removexattr), and there the the error code is ENOATTR, but:

ENOATTR
The named attribute does not exist. (ENOATTR is defined to be a synonym for ENODATA in <attr/xattr.h>.)
If you can explain what I missed.

  • Anyway, I saw from the logs that you attached in the issue that it is:
code: ENODATA, context: FileReplacexattr
  • I was wondering if we want what you created as a function that we can use in other places in NSFS, but is out of the scope of this PR... (just raising as a thought).

@nadavMiz
Copy link
Contributor Author

nadavMiz commented Apr 10, 2025

@shirady regarding your comment. in _clear_user_xattr we clear the user xattr from the directory. we check that the xattr exists before clearing them. the issue is that two threads can get to this function together and then one thread deletes the xattr while the second fails with this error because the first already deleted the xattr. and so the The named attribute does not exist. regarding creating a function for this: I am not sure I should do it at this point because its a very specific scenario. but maybe we should consider modifying the existing function so it will not fail in case there are no xattr on the object?

@shirady
Copy link
Contributor

shirady commented Apr 10, 2025

@nadavMiz thank you for the explanation.
I was trying to understand which system cal exactly fails it - fremovexattr or removexattr, if you can show a reference from the code.

@nadavMiz nadavMiz force-pushed the xattr-concurrency branch from 83289f7 to 9d7e284 Compare April 10, 2025 13:47
@nadavMiz nadavMiz force-pushed the xattr-concurrency branch from 9d7e284 to 821ad37 Compare April 23, 2025 10:41
@nadavMiz
Copy link
Contributor Author

@shirady we are using fremovexattr. see clear_xattr in fs_nappi. however both fremovexattr and removexattr have the same errors

@nadavMiz nadavMiz requested review from romayalon and shirady April 23, 2025 10:44
@nadavMiz nadavMiz force-pushed the xattr-concurrency branch from 821ad37 to 9cb1a03 Compare April 23, 2025 10:44
@nadavMiz nadavMiz force-pushed the xattr-concurrency branch from 9cb1a03 to 06e71fb Compare April 23, 2025 10:45
@nadavMiz nadavMiz merged commit 923e708 into noobaa:master Apr 23, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI test fail - content dir multiple puts of the same key - enabled
3 participants