-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Do calls to storage Load need to be Lock protected? #296
Comments
Filestore uses os.WriteFile
if reader reads while writefile is running. it could read after open file, which truncates the file, so it will read empty. so I think there is a race and protection is needed. within a process, read will force page cache flush if read is run after write before close, however between multiple processes. the file is empty from the opening of the file to fsync on close. within the process, an rwmutex is needed likely. however between multiple processes, it may be prudent to require a global lock on read. if so, it may be correct to switch to a locking system with rw support in the long term (advisory lock, etc) |
https://github.com/containerd/containerd/blob/main/pkg%2Fatomicfile%2Ffile.go |
No; this synchronization should be handled primitively, natively/internally, within the storage mechanism. The user shouldn't have to lock for every other storage operation.
Locking is for user-scope purposes, like renewing certificates or something. So it can protect reads and writes, but what it's really for is to sync a certain operation or task that may involve multiple reads or writes.
It sounds like you know a lot more about the primitives of file systems than I do 😅 Especially after our conversation in slack. We can implement improvements to the FileStorage if needed! Is it a matter of calling Sync() in more places? |
i think the easiest solution is to do what containerd is doing. instead of writing over a file, we should write a new file and attempt to copy over it. i will try to replicate the race condition, create a fix, and show that the race is resolved. the comment in their code describes:
the lock existing lock can deal with #2, we just need a mechanism to deal with #1 |
That would be a great help, thank you! 🙌 |
yessir |
by contract, do calls to load need to be protected by the storage lock?
from what I see, the lock is used only to protect for simultaneous writes. it's not used to lock/protect the reader if a writer is currently writing.
is the caller supposed to take the Lock before reading the certificate? if not, are storage Load implementations supposed to protect for concurrent read/write access?
The text was updated successfully, but these errors were encountered: