Repository navigation
Conversation
Three tests for writes and reads that race on one object: - 16 create-if-absent writers: exactly one may succeed, and the stored checksum and content type must belong to the stored body. - 16 If-Match writers holding the same ETag: exactly one may succeed. - 8 unconditional writers per round over 40 rounds, with two readers running throughout: every read must pair a body with its own checksum and its own content type. A gated body holds each writer's last chunk until every writer has streamed its first, so all of them pass the early condition check before any of them stores the object. That makes the first two tests fail every time on the current code, which checks the conditions only once, before the body streams. Each test body is also written as its content type, so a read that mixes the body of one write with the attributes or the checksum of another fails the pairing check. Each writer is bounded by a 60 s timeout, since a writer that fails before the gate would leave the others waiting at it. Refs: NOFireAI/ravel#3652 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
An object is three files: the body, its attributes and its internal info, which holds the stored checksums and ETag. put_object and complete_multipart_upload checked If-None-Match and If-Match once, before streaming the body, then renamed the body into place and wrote the two sidecars afterwards, each with its own rename. Writers that arrived together all passed the check, so several create-if-absent or If-Match writers could each get 200, and the last body rename and the last internal-info rename could come from different writers. A read could also open a new body and load the old sidecars. Ravel's benchmark hit this: concurrent create-if-absent writes of one key left a body whose stored CRC64-NVME belonged to another write, and every later GET of it failed the client's checksum check (NOFireAI/ravel#3652). Every object now maps by hash onto one of 1024 read-write locks. - put_object, copy_object and complete_multipart_upload write all three files as flushed temporary files first. They then take the object's write lock, check their conditions again, and rename the files in one blocking task that owns the lock. A full disk fails the write before anything is published, and a dropped request cannot stop the renames half way. The early condition check stays as a fast refusal. POST Object goes through put_object, and a directory key takes the same lock for its check and its directory. - copy_object takes the destination's write lock and the source's read lock in index order, so two copies in opposite directions cannot deadlock. It no longer copies over the destination in place, which truncated a body a reader could still be streaming. When the source has no stored ETag, the destination's ETag is now the digest of the source instead of a re-read of the destination. - get_object, head_object and upload_part_copy open the body and load the sidecars under the read lock. The body then streams from the open file, so it stays the version the checksums describe. - delete_object and delete_objects hold the write lock while they remove an object. - An ETag computed for an object with none stored is written back to its internal info. That write used to run without any ordering against a writer, so it could put the old body's ETag and checksums beside a new body. Every caller now holds the object's lock, including the early condition check, which takes the read lock. The lock is per process, which matches s3s-fs's one-process model. Listings do not read the sidecars, so they stay unlocked. Refs: NOFireAI/ravel#3652 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
Two tests for copy_object under concurrency: - One task copies two sources, with different bodies and content types, onto one destination in turn, 40 times, while two readers read the destination. Every read must pair a body with its own checksum and its own content type. - Two tasks copy A to B and B to A, 40 times each, at the same time. Both must finish within 60 s, and both objects must read back paired. The second test catches a copy that takes the locks of its two objects in an order that can deadlock. The reader loop moves into helpers shared with the overwrite test, and every copy and every task is bounded by 60 s. Refs: NOFireAI/ravel#3652 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
copy_object held the destination's write lock and the source's read lock for the whole copy: the condition checks, any ETag computation, the byte copy and the publish. Copying a large body blocked every read and write of the destination, and of every object sharing its lock, for as long as the copy ran. It also copied the bytes in user space instead of with fs::copy, which the kernel can speed up. A copy now takes only the source's read lock. Under it, the copy checks the source conditions, loads the source's internal info and attributes, computes a missing ETag, copies the body with fs::copy into a temporary file and stages both sidecars. It then releases the source lock and takes the destination's write lock only to publish the staged files. A copy never holds two locks at once, so the ordered locking of source and destination is gone. Every staged file is removed if the copy fails before it publishes. A copy onto itself still updates the object in place, so it holds the object's write lock throughout, as before. Refs: NOFireAI/ravel#3652 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
put_object and complete_multipart_upload took the object's read lock for their early condition check even when the request carried no If-None-Match or If-Match. With no condition the check does nothing, so an unconditional write now skips it and that lock. Refs: NOFireAI/ravel#3652 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>
Member
Author
|
Validation done; proposed upstream as s3s-project#1018 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Validation only; upstream port of #2.