Conversation
client/client.c
Outdated
| "set KEY VALUE [ EX seconds | PX milliseconds ]\tset key:value to priskv\n"}, | ||
| {"alloc_set", alloc_set_handler, | ||
| "alloc set KEY VALUE [ EX seconds | PX milliseconds ]\tset key:value to priskv\n"}, | ||
| "alloc set KEY VALUE [ EX seconds | PX milliseconds ] [ --pin-on-seal ]\tzero-copy set with optional pin on seal\n"}, |
There was a problem hiding this comment.
let's follow the redis command convention for pin flags, e.g, using "PIN" "UNPIN" instead of "--pin-on-seal" "--pin-on-acquire" and "--unpin-on-release".
There was a problem hiding this comment.
changed to redis command convention, please take a look
| status = priskv_publish_node(conn->kv, keynode); | ||
| /* Optional: pin-on-seal if requested */ | ||
| if (status == PRISKV_RESP_STATUS_OK && (flags & PRISKV_REQ_FLAG_PIN_ON_SEAL)) { | ||
| (void)priskv_key_pin(conn->kv, keynode); |
There was a problem hiding this comment.
what if an eviction happen in the middle of priskv_publish_node and priskv_key_pin? we need publish and pin to be atomic therefore this key is prevented from being kick off.
| old_keynode = priskv_find_key(kv, (uint8_t *)keynode->key, keynode->keylen, | ||
| PRISKV_KEY_MAX_TIMEOUT, true, NULL); | ||
| if (old_keynode) { | ||
| /* inherit pin_count from old version to keep lifecycle semantics */ |
There was a problem hiding this comment.
There appears to be a race condition in both the original priskv_publish_node function and the updated version with pin support. Consider the following scenario with thread A and thread B both calling the original priskv_publish_node on the same key (key-0):
- Both threads execute
old_keynode = priskv_find_key(kv, (uint8_t *)keynode->key, keynode->keylen, ...)nearly simultaneously - Since neither has inserted their keynode yet, both find no existing node and receive NULL for old_keynode
- Consequently, both threads proceed to insert their respective keynodes into kv and LRU
- This results in two different keynodes with identical keys coexisting in the KV store, which is invalid.
For the updated version, uint32_t old_pins = old_keynode->pin_count; and keynode->pin_count = old_pins; need to be atomic, otherwise, the value of old_keynode->pin_count could be updated before the old_keynode being deleted.
2 participants
Pull Request Description
[Please provide a clear and concise description of your changes here]
Related Issues
Resolves: #[Insert issue number(s)]
Important: Before submitting, please complete the description above and review the checklist below.
Contribution Guidelines (Expand for Details)
We appreciate your contribution to PrisKV! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:
Pull Request Title Format
Your PR title should start with one of these prefixes to indicate the nature of the change:
[Bug]: Corrections to existing functionality[CI]: Changes to build process or CI pipeline[Docs]: Updates or additions to documentation[API]: Modifications to PrisKV's API or interface[CLI]: Changes or additions to the Command Line Interface[Misc]: For changes not covered above (use sparingly)Note: For changes spanning multiple categories, use multiple prefixes in order of importance.
Submission Checklist
By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.