Add lock for updating file size in OS memory provider #622
Conversation
Add lock for updating file size in OS memory provider, because umfMemoryProviderAlloc() has to be MT-safe. Signed-off-by: Lukasz Dorau <[email protected]>
| if (util_mutex_lock(lock_fd)) { | ||
| LOG_ERR("locking file size failed"); | ||
| return -1; | ||
| } | ||
|
|
||
| if (*fd_size + extended_length > max_fd_size) { | ||
| util_mutex_unlock(lock_fd); | ||
| LOG_ERR("cannot grow a file size beyond %zu", max_fd_size); | ||
| return -1; | ||
| } | ||
|
|
||
| fd_offset = *fd_size; | ||
| *fd_size += extended_length; | ||
| util_mutex_unlock(lock_fd); |
There was a problem hiding this comment.
What about something like this? It probably should have better performance, and we don't need extra locks.
I'm not very proficient with atomics, so it would be good if @pbalcer or @igchor check if my proposal is valid .
| if (util_mutex_lock(lock_fd)) { | |
| LOG_ERR("locking file size failed"); | |
| return -1; | |
| } | |
| if (*fd_size + extended_length > max_fd_size) { | |
| util_mutex_unlock(lock_fd); | |
| LOG_ERR("cannot grow a file size beyond %zu", max_fd_size); | |
| return -1; | |
| } | |
| fd_offset = *fd_size; | |
| *fd_size += extended_length; | |
| util_mutex_unlock(lock_fd); | |
| do { | |
| size_t size = *fd_size; | |
| if (size + extended_length > max_fd_size) { | |
| LOG_ERR("cannot grow a file size beyond %zu", max_fd_size); | |
| return -1; | |
| } | |
| fd_offset = *fd_size; | |
| } while(!__sync_bool_compare_and_swap(fd_size, size, size + extended_lenght)); |
There was a problem hiding this comment.
It does not work - another thread can update fd_size with exactly the same value ...
There was a problem hiding this comment.
It should not be a problem - as compare_and_swap checks for old value, and them updates it to new value. So even if one thread updates this value, second thread will detect it
There was a problem hiding this comment.
Right, it should work.
But how would you do that on Windows? I do not know any equivalent of __sync_bool_compare_and_swap on Windows ...
There was a problem hiding this comment.
All atomic functions are available on windows - just they have stupid names.
https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64
There was a problem hiding this comment.
my 2 cents: since this isn't perf critical, I'd just leave it as is with the mutex. Atomics can be tricky to get right and make things more difficult to modify.
There was a problem hiding this comment.
I agree with Piotr - since this is not the allocation path we should use the simplest solution
There was a problem hiding this comment.
I agree with Piotr - since this is not the allocation path we should use the simplest solution
This is an allocation path
Description
Add lock for updating file size in OS memory provider, because
umfMemoryProviderAlloc()has to be MT-safe.Checklist