-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Handle truncation while mmap-ing (esp. on Windows) mmap on Windows works differntly from mmap on Linux. * We cannot mmap to a size beyond the size of the file. So we need to truncate to that size and then map. * We cannot truncate a file while a section of it has been mapped. This has implications for how we manage value logs. This change fixes mmap-ing on Windows with the following changes: * When badger.NewKV() function is called, which in turn calls valueLog.Open() method, we don’t immediately mmap the writable log files (read-only log files are not a problem). We mmap the writable log after replay has happened. * Whenever the valueLog.iterate() method is called, and it determines that a file needs to be truncated - we need to either make sure that there are no open mmaps, or we need to munmap and then re-map the file after truncation. * Whenever a writable log file is mmap-ed, its size increases to ValueLogFileSize * 2 (because of truncation in y.Mmap() function). We need to ensure that once it has filled up, it is truncated back to the actual size before it is closed and opened as a read-only file. We also need to truncate if back if the value log is closed before it fills up. * Fix a few tests on AppVeyor that were running out of memory by reducing ValueLogFileSize option value. Fixes #212. * Only truncate if the file isn't mmapped. Fix mmap-ing on Windows. The previous code we checked in for mmap on Windows was broken. Cleaned up the code after looking at the way BoltDB does mmap-ing on Windows. The most important thing to note is that in Windows, unlike Linux we cannot map beyond the size of the file. So the workaround is to truncate the file to a bigger size before mmap-ing it. Special care needs to be taken when calling this function, because read-only file descriptors cannot be truncated.
- Loading branch information
1 parent
8de624f
commit 273f40c
Showing
4 changed files
with
75 additions
and
29 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
273f40c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question regarding the changes made in
kv.go
. I'm not familiar enough with the logic behind the code, but I'm now getting theUnable to mmap RDWR log file
error even when I'm using the optionOpt.TableLoadingMode = options.FileIO
. I have to use that option, since in one scenario where I'm using Badger, I have a very small memory footprint, and the Go application is being called via FastCGI with some restrictions — one of them being 'no memory mapping allowed'. Well, sort of: it can be used, so long as the actual memory being allocated is infinitesimally small (but the smallest possible size is 1 MByte). This does not seem to be the case, since, as far as I can understand the code, this is some memory allocated for the log and not the database.I wonder if there could be an extra check to see what options have been set and avoid mmap if FileIO has been selected?
273f40c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... Our testing showed that this fixes the issue.
So, the issue you're seeing is related to value log memory map. The option that we have is for LSM tree. So, changing that option wouldn't change this behavior.
Can you please create a Github issue adding the panic / error that you're getting, the Badger commit you're at, which OS etc.? The engineer working on this is currently on vacation, but we'll try and get to this as soon as we can.
Update: On re-reading your description, seems like you're in an environment where memory mapping isn't allowed? So, this is not a windows issue, but a specific issue with the env you're running in?
273f40c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're quite right. I'm sorry, I'm not using Windows at all (but a customised Debian Linux setup over which I have no control). However, before this patch was committed, I had no issues with memory allocation; I wrongly interpreted it to be related to something here. It must be a different issue, and so I've filed it separately as per your suggestion: #246